Changeset 150537 in webkit

Timestamp:

May 22, 2013 1:50:44 PM (11 years ago)

Author:

ap@apple.com

Message:

Crashes in NetworkProcess due to incorrect private browsing session tracking
​https://bugs.webkit.org/show_bug.cgi?id=116628

Reviewed by Brady Eidson.

The current API for private browsing makes it extremely difficult to track sessions.
Private browsing is enabled via WKPreferences, but the session is shared, so it
has to be maintained while there is any chance that any page group anywhere still
needs it.

This patch fixes some of the issues, but ultimately, I think that we'll just need
to deprecate and replace the API.

• NetworkProcess/NetworkConnectionToWebProcess.cpp: (WebKit::storageSession): There are valid scenarios where privateBrowsingEnabled is true, but there is no private browsing session. Handle that without crashing, although this unfortunately means that it will be harder to spot logic errors when using a wrong session. (WebKit::NetworkConnectionToWebProcess::registerBlobURL): Removed an obsolete FIXME.

• NetworkProcess/mac/RemoteNetworkingContext.h: Changed privateBrowsingSession() to return a pointer, as no caller could know when it was safe to call it.

• NetworkProcess/mac/RemoteNetworkingContext.mm: (WebKit::RemoteNetworkingContext::storageSession): Handle the case where private browsing session is unexpectedly missing without crashing. (WebKit::RemoteNetworkingContext::privateBrowsingSession): Changed to return a pointer.

• UIProcess/WebContext.cpp: (WebKit::WebContext::ensureNetworkProcess): Actually initialize privateBrowsingEnabled creation parameter. It would be very difficult to figure out 100% reliably whether NetworkProcess needs a private browsing session with the current API, but for existing clients, looking at m_privateBrowsingEnterCount is good enough. Certainly better than not initializing.

• WebProcess/InjectedBundle/InjectedBundle.cpp: (WebKit::InjectedBundle::setPrivateBrowsingEnabled): Added a FIXME.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkConnectionToWebProcess.cpp (modified) (2 diffs)
• NetworkProcess/mac/RemoteNetworkingContext.h (modified) (1 diff)
• NetworkProcess/mac/RemoteNetworkingContext.mm (modified) (2 diffs)
• UIProcess/WebContext.cpp (modified) (1 diff)
• WebProcess/InjectedBundle/InjectedBundle.cpp (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2013-05-22  Alexey Proskuryakov  <ap@apple.com>
+
+        Crashes in NetworkProcess due to incorrect private browsing session tracking
+        https://bugs.webkit.org/show_bug.cgi?id=116628
+
+        Reviewed by Brady Eidson.
+
+        The current API for private browsing makes it extremely difficult to track sessions.
+        Private browsing is enabled via WKPreferences, but the session is shared, so it
+        has to be maintained while there is any chance that any page group anywhere still
+        needs it.
+
+        This patch fixes some of the issues, but ultimately, I think that we'll just need
+        to deprecate and replace the API.
+
+        * NetworkProcess/NetworkConnectionToWebProcess.cpp: (WebKit::storageSession):
+        There are valid scenarios where privateBrowsingEnabled is true, but there is no
+        private browsing session. Handle that without crashing, although this unfortunately
+        means that it will be harder to spot logic errors when using a wrong session.
+        (WebKit::NetworkConnectionToWebProcess::registerBlobURL): Removed an obsolete FIXME.
+
+        * NetworkProcess/mac/RemoteNetworkingContext.h: Changed privateBrowsingSession()
+        to return a pointer, as no caller could know when it was safe to call it.
+
+        * NetworkProcess/mac/RemoteNetworkingContext.mm:
+        (WebKit::RemoteNetworkingContext::storageSession): Handle the case where private
+        browsing session is unexpectedly missing without crashing.
+        (WebKit::RemoteNetworkingContext::privateBrowsingSession): Changed to return a pointer.
+
+        * UIProcess/WebContext.cpp: (WebKit::WebContext::ensureNetworkProcess):
+        Actually initialize privateBrowsingEnabled creation parameter. It would be very
+        difficult to figure out 100% reliably whether NetworkProcess needs a private browsing
+        session with the current API, but for existing clients, looking at
+        m_privateBrowsingEnterCount is good enough. Certainly better than not initializing.
+
+        * WebProcess/InjectedBundle/InjectedBundle.cpp:
+        (WebKit::InjectedBundle::setPrivateBrowsingEnabled): Added a FIXME.
+
 2013-05-22  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp

@@ -157 +157 @@
 static NetworkStorageSession& storageSession(bool privateBrowsingEnabled)
 {
-    return privateBrowsingEnabled ? RemoteNetworkingContext::privateBrowsingSession() : NetworkStorageSession::defaultStorageSession();
+    if (privateBrowsingEnabled) {
+        NetworkStorageSession* privateSession = RemoteNetworkingContext::privateBrowsingSession();
+        if (privateSession)
+            return *privateSession;
+        // Some requests with private browsing mode requested may still be coming shortly after NetworkProcess was told to destroy its session.
+        // FIXME: Find a way to track private browsing sessions more rigorously.
+        LOG_ERROR("Private browsing was requested, but there was no session for it. Please file a bug unless you just disabled private browsing, in which case it's an expected race.");
+    }
+    return NetworkStorageSession::defaultStorageSession();
 }
 
@@ -209 +217 @@
 void NetworkConnectionToWebProcess::registerBlobURL(const KURL& url, const BlobRegistrationData& data)
 {
-    // FIXME: unregister all URLs when process connection closes.
-
     Vector<RefPtr<SandboxExtension>> extensions;
     for (size_t i = 0, count = data.sandboxExtensions().size(); i < count; ++i) {

trunk/Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.h

@@ -43 +43 @@
     static void destroyPrivateBrowsingSession();
 
-    static WebCore::NetworkStorageSession& privateBrowsingSession(); // Can only be called when the session exists.
+    static WebCore::NetworkStorageSession* privateBrowsingSession();
 
     virtual bool shouldClearReferrerOnHTTPSToHTTPRedirect() const OVERRIDE;

trunk/Source/WebKit2/NetworkProcess/mac/RemoteNetworkingContext.mm

@@ -79 +79 @@
 {
     if (m_privateBrowsingEnabled) {
-        ASSERT(privateBrowsingStorageSession());
-        return *privateBrowsingStorageSession();
+        NetworkStorageSession* privateSession = privateBrowsingStorageSession().get();
+        if (privateSession)
+            return *privateSession;
+        // Some requests with private browsing mode requested may still be coming shortly after NetworkProcess was told to destroy its session.
+        // FIXME: Find a way to track private browsing sessions more rigorously.
+        LOG_ERROR("Private browsing was requested, but there was no session for it. Please file a bug unless you just disabled private browsing, in which case it's an expected race.");
     }
 
@@ -86 +90 @@
 }
 
-NetworkStorageSession& RemoteNetworkingContext::privateBrowsingSession()
+NetworkStorageSession* RemoteNetworkingContext::privateBrowsingSession()
 {
-    ASSERT(privateBrowsingStorageSession());
-    return *privateBrowsingStorageSession();
+    return privateBrowsingStorageSession().get();
 }
 

trunk/Source/WebKit2/UIProcess/WebContext.cpp

@@ -374 +374 @@
         SandboxExtension::createHandleForReadWriteDirectory(parameters.diskCacheDirectory, parameters.diskCacheDirectoryExtensionHandle);
 
+    // FIXME: We don't account for private browsing mode being enabled due to a persistent preference in any of active page groups.
+    // This means that clients must re-enable private browsing mode through API on each launch, not relying on preferences.
+    // If the client does not re-enable private browsing on next launch, NetworkProcess will crash.
+    parameters.privateBrowsingEnabled = m_privateBrowsingEnterCount;
+
     parameters.cacheModel = m_cacheModel;
 

trunk/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp

@@ -304 +304 @@
 void InjectedBundle::setPrivateBrowsingEnabled(WebPageGroupProxy* pageGroup, bool enabled)
 {
+    // FIXME (NetworkProcess): This test-only function doesn't work with NetworkProcess, <https://bugs.webkit.org/show_bug.cgi?id=115274>.
 #if PLATFORM(MAC) || USE(CFNETWORK)
     if (enabled)