Regression(r230567): Unable to log into twitter.com in private sessions
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Jun 2018 22:56:29 +0000 (22:56 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Jun 2018 22:56:29 +0000 (22:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186205
<rdar://problem/40670799>

Reviewed by Youenn Fablet.

We were using the same SWServer for all private sessions and the SWServer's sessionID would
be legacyPrivateSessionID(). As a result, the service worker's sessionID would be legacyPrivateSessionID()
Source/WebCore:

as well and would not match the sessionID of its client pages. This sessionID mismatch was
causing the breakage.

Instead of using the same SWServer of all private sessions, we now go back to using a SWServer
per private session. However, we now make sure that the SWServer gets destroyed whenever its
corresponding session gets destroyed.

* workers/service/server/SWServer.cpp:
(WebCore::SWServer::~SWServer):

Source/WebKit:

as well and would not match the sessionID of its client pages. This sessionID mismatch was
causing the breakage.

Instead of using the same SWServer of all private sessions, we now go back to using a SWServer
per private session. However, we now make sure that the SWServer gets destroyed whenever its
corresponding session gets destroyed.

* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::destroySession):
* NetworkProcess/cache/CacheStorageEngine.cpp:
(WebKit::CacheStorage::Engine::from):
* StorageProcess/StorageProcess.cpp:
(WebKit::StorageProcess::destroySession):
(WebKit::StorageProcess::swServerForSession):
* StorageProcess/StorageProcess.h:
* StorageProcess/StorageProcess.messages.in:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::setAnyPageGroupMightHavePrivateBrowsingEnabled):
* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::~WebsiteDataStore):

(WebKit::WebsiteDataStore::enableResourceLoadStatisticsAndSetTestingCallback):
* UIProcess/WebsiteData/WebsiteDataStore.h:
(WebKit::WebsiteDataStore::weakPtrFactory const):
Fix memory leak caused by a reference cycle between the WebsiteDataStore and its
WebResourceLoadStatisticsStore, by using WeakPtr to break the cycle. This was causing
us to leak WebsiteDataStore objects, which would prevent the destruction of sessions.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232423 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/workers/service/server/SWServer.cpp
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkProcess.cpp
Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
Source/WebKit/StorageProcess/StorageProcess.cpp
Source/WebKit/StorageProcess/StorageProcess.h
Source/WebKit/StorageProcess/StorageProcess.messages.in
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h

index 1cb6e6f..65cf36a 100644 (file)
@@ -1,3 +1,23 @@
+2018-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r230567): Unable to log into twitter.com in private sessions
+        https://bugs.webkit.org/show_bug.cgi?id=186205
+        <rdar://problem/40670799>
+
+        Reviewed by Youenn Fablet.
+
+        We were using the same SWServer for all private sessions and the SWServer's sessionID would
+        be legacyPrivateSessionID(). As a result, the service worker's sessionID would be legacyPrivateSessionID()
+        as well and would not match the sessionID of its client pages. This sessionID mismatch was
+        causing the breakage.
+
+        Instead of using the same SWServer of all private sessions, we now go back to using a SWServer
+        per private session. However, we now make sure that the SWServer gets destroyed whenever its
+        corresponding session gets destroyed.
+
+        * workers/service/server/SWServer.cpp:
+        (WebCore::SWServer::~SWServer):
+
 2018-06-01  Youenn Fablet  <youenn@apple.com>
 
         Add an option to restrict communication to localhost sockets
index dc325dd..a20822a 100644 (file)
@@ -70,10 +70,6 @@ HashSet<SWServer*>& SWServer::allServers()
 
 SWServer::~SWServer()
 {
-    RELEASE_ASSERT(m_connections.isEmpty());
-    RELEASE_ASSERT(m_registrations.isEmpty());
-    RELEASE_ASSERT(m_jobQueues.isEmpty());
-    
     allServers().remove(this);
 }
 
index 04176b1..77ed1bb 100644 (file)
@@ -1,3 +1,42 @@
+2018-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r230567): Unable to log into twitter.com in private sessions
+        https://bugs.webkit.org/show_bug.cgi?id=186205
+        <rdar://problem/40670799>
+
+        Reviewed by Youenn Fablet.
+
+        We were using the same SWServer for all private sessions and the SWServer's sessionID would
+        be legacyPrivateSessionID(). As a result, the service worker's sessionID would be legacyPrivateSessionID()
+        as well and would not match the sessionID of its client pages. This sessionID mismatch was 
+        causing the breakage.
+
+        Instead of using the same SWServer of all private sessions, we now go back to using a SWServer
+        per private session. However, we now make sure that the SWServer gets destroyed whenever its
+        corresponding session gets destroyed.
+
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::destroySession):
+        * NetworkProcess/cache/CacheStorageEngine.cpp:
+        (WebKit::CacheStorage::Engine::from):
+        * StorageProcess/StorageProcess.cpp:
+        (WebKit::StorageProcess::destroySession):
+        (WebKit::StorageProcess::swServerForSession):
+        * StorageProcess/StorageProcess.h:
+        * StorageProcess/StorageProcess.messages.in:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::setAnyPageGroupMightHavePrivateBrowsingEnabled):
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::~WebsiteDataStore):
+
+        (WebKit::WebsiteDataStore::enableResourceLoadStatisticsAndSetTestingCallback):
+        * UIProcess/WebsiteData/WebsiteDataStore.h:
+        (WebKit::WebsiteDataStore::weakPtrFactory const):
+        Fix memory leak caused by a reference cycle between the WebsiteDataStore and its
+        WebResourceLoadStatisticsStore, by using WeakPtr to break the cycle. This was causing
+        us to leak WebsiteDataStore objects, which would prevent the destruction of sessions.
+
+
 2018-06-01  Youenn Fablet  <youenn@apple.com>
 
         Add an option to restrict communication to localhost sockets
index 29d6246..db13dbc 100644 (file)
@@ -363,6 +363,7 @@ void NetworkProcess::destroySession(PAL::SessionID sessionID)
 {
     SessionTracker::destroySession(sessionID);
     m_sessionsControlledByAutomation.remove(sessionID);
+    CacheStorage::Engine::destroyEngine(sessionID);
 }
 
 void NetworkProcess::grantSandboxExtensionsToStorageProcessForBlobs(const Vector<String>& filenames, Function<void ()>&& completionHandler)
index ddb8c25..dc87264 100644 (file)
@@ -82,9 +82,6 @@ Engine::~Engine()
 
 Engine& Engine::from(PAL::SessionID sessionID)
 {
-    if (sessionID.isEphemeral())
-        sessionID = PAL::SessionID::legacyPrivateSessionID();
-
     auto addResult = globalEngineMap().add(sessionID, nullptr);
     if (addResult.isNewEntry)
         addResult.iterator->value = Engine::create(NetworkProcess::singleton().cacheStorageDirectory(sessionID));
index 421ce76..a790315 100644 (file)
@@ -284,6 +284,16 @@ void StorageProcess::createStorageToWebProcessConnection(bool isServiceWorkerPro
 #endif
 }
 
+void StorageProcess::destroySession(PAL::SessionID sessionID)
+{
+#if ENABLE(SERVICE_WORKER)
+    m_swServers.remove(sessionID);
+    m_swDatabasePaths.remove(sessionID);
+#else
+    UNUSED_PARAM(sessionID);
+#endif
+}
+
 void StorageProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, uint64_t callbackID)
 {
     auto websiteData = std::make_unique<WebsiteData>();
@@ -424,10 +434,6 @@ SWServer& StorageProcess::swServerForSession(PAL::SessionID sessionID)
 {
     ASSERT(sessionID.isValid());
 
-    // Use the same SWServer for all ephemeral sessions.
-    if (sessionID.isEphemeral())
-        sessionID = PAL::SessionID::legacyPrivateSessionID();
-
     auto result = m_swServers.add(sessionID, nullptr);
     if (!result.isNewEntry) {
         ASSERT(result.iterator->value);
index ea0f713..ab1bb2e 100644 (file)
@@ -128,6 +128,7 @@ private:
     void initializeWebsiteDataStore(const StorageProcessCreationParameters&);
     void createStorageToWebProcessConnection(bool isServiceWorkerProcess, WebCore::SecurityOriginData&&);
 
+    void destroySession(PAL::SessionID);
     void fetchWebsiteData(PAL::SessionID, OptionSet<WebsiteDataType> websiteDataTypes, uint64_t callbackID);
     void deleteWebsiteData(PAL::SessionID, OptionSet<WebsiteDataType> websiteDataTypes, WallTime modifiedSince, uint64_t callbackID);
     void deleteWebsiteDataForOrigins(PAL::SessionID, OptionSet<WebsiteDataType> websiteDataTypes, const Vector<WebCore::SecurityOriginData>& origins, uint64_t callbackID);
index 56aa746..31a0507 100644 (file)
@@ -35,6 +35,8 @@ messages -> StorageProcess LegacyReceiver {
     DidGetSandboxExtensionsForBlobFiles(uint64_t requestID, WebKit::SandboxExtension::HandleArray extensions)
 #endif
 
+    DestroySession(PAL::SessionID sessionID)
+
 #if ENABLE(SERVICE_WORKER)
     DidNotHandleFetch(WebCore::SWServerConnectionIdentifier serverConnectionIdentifier, WebCore::FetchIdentifier fetchIdentifier)
     DidFailFetch(WebCore::SWServerConnectionIdentifier serverConnectionIdentifier, WebCore::FetchIdentifier fetchIdentifier)
index 9b7ad82..e7e9169 100644 (file)
@@ -721,17 +721,14 @@ void WebProcessPool::windowServerConnectionStateChanged()
 
 void WebProcessPool::setAnyPageGroupMightHavePrivateBrowsingEnabled(bool privateBrowsingEnabled)
 {
-    if (networkProcess()) {
-        if (privateBrowsingEnabled)
-            networkProcess()->send(Messages::NetworkProcess::AddWebsiteDataStore(WebsiteDataStoreParameters::legacyPrivateSessionParameters()), 0);
-        else
-            networkProcess()->send(Messages::NetworkProcess::DestroySession(PAL::SessionID::legacyPrivateSessionID()), 0);
-    }
-
-    if (privateBrowsingEnabled)
+    if (privateBrowsingEnabled) {
+        sendToNetworkingProcess(Messages::NetworkProcess::AddWebsiteDataStore(WebsiteDataStoreParameters::legacyPrivateSessionParameters()));
         sendToAllProcesses(Messages::WebProcess::AddWebsiteDataStore(WebsiteDataStoreParameters::legacyPrivateSessionParameters()));
-    else
+    } else {
+        sendToNetworkingProcess(Messages::NetworkProcess::DestroySession(PAL::SessionID::legacyPrivateSessionID()));
+        sendToStorageProcess(Messages::StorageProcess::DestroySession(PAL::SessionID::legacyPrivateSessionID()));
         sendToAllProcesses(Messages::WebProcess::DestroySession(PAL::SessionID::legacyPrivateSessionID()));
+    }
 }
 
 void (*s_invalidMessageCallback)(WKStringRef messageName);
index 548761a..68192de 100644 (file)
@@ -32,6 +32,7 @@
 #include "NetworkProcessMessages.h"
 #include "StorageManager.h"
 #include "StorageProcessCreationParameters.h"
+#include "StorageProcessMessages.h"
 #include "WebCookieManagerProxy.h"
 #include "WebProcessMessages.h"
 #include "WebProcessPool.h"
@@ -116,8 +117,10 @@ WebsiteDataStore::~WebsiteDataStore()
     if (m_sessionID.isValid() && m_sessionID != PAL::SessionID::defaultSessionID()) {
         ASSERT(nonDefaultDataStores().get(m_sessionID) == this);
         nonDefaultDataStores().remove(m_sessionID);
-        for (auto& processPool : WebProcessPool::allProcessPools())
+        for (auto& processPool : WebProcessPool::allProcessPools()) {
             processPool->sendToNetworkingProcess(Messages::NetworkProcess::DestroySession(m_sessionID));
+            processPool->sendToStorageProcess(Messages::StorageProcess::DestroySession(m_sessionID));
+        }
     }
 }
 
@@ -1462,16 +1465,21 @@ void WebsiteDataStore::enableResourceLoadStatisticsAndSetTestingCallback(Functio
     }
 
 #if HAVE(CFNETWORK_STORAGE_PARTITIONING)
-    m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(m_configuration.resourceLoadStatisticsDirectory, WTFMove(callback), m_sessionID.isEphemeral(), [this, protectedThis = makeRef(*this)] (const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst shouldClearFirst) {
-        updatePrevalentDomainsToPartitionOrBlockCookies(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst);
-    }, [this, protectedThis = makeRef(*this)] (const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void(bool hasAccess)>&& callback) {
-        hasStorageAccessForFrameHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
-    }, [this, protectedThis = makeRef(*this)] (const String& resourceDomain, const String& firstPartyDomain, std::optional<uint64_t> frameID, uint64_t pageID, WTF::CompletionHandler<void(bool wasGranted)>&& callback) {
-        grantStorageAccessHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
-    }, [this, protectedThis = makeRef(*this)] () {
-        removeAllStorageAccessHandler();
-    }, [this, protectedThis = makeRef(*this)] (const Vector<String>& domainsToRemove) {
-        removePrevalentDomains(domainsToRemove);
+    m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(m_configuration.resourceLoadStatisticsDirectory, WTFMove(callback), m_sessionID.isEphemeral(), [weakThis = makeWeakPtr(*this)] (const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst shouldClearFirst) {
+        if (weakThis)
+            weakThis->updatePrevalentDomainsToPartitionOrBlockCookies(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst);
+    }, [weakThis = makeWeakPtr(*this)] (const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void(bool hasAccess)>&& callback) {
+        if (weakThis)
+            weakThis->hasStorageAccessForFrameHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
+    }, [weakThis = makeWeakPtr(*this)] (const String& resourceDomain, const String& firstPartyDomain, std::optional<uint64_t> frameID, uint64_t pageID, WTF::CompletionHandler<void(bool wasGranted)>&& callback) {
+        if (weakThis)
+            weakThis->grantStorageAccessHandler(resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
+    }, [weakThis = makeWeakPtr(*this)] () {
+        if (weakThis)
+            weakThis->removeAllStorageAccessHandler();
+    }, [weakThis = makeWeakPtr(*this)] (const Vector<String>& domainsToRemove) {
+        if (weakThis)
+            weakThis->removePrevalentDomains(domainsToRemove);
     });
 #else
     m_resourceLoadStatistics = WebResourceLoadStatisticsStore::create(m_configuration.resourceLoadStatisticsDirectory, WTFMove(callback), m_sessionID.isEphemeral());
index e3d1e90..3abf8ff 100644 (file)
@@ -37,6 +37,7 @@
 #include <wtf/OptionSet.h>
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
+#include <wtf/WeakPtr.h>
 #include <wtf/WorkQueue.h>
 #include <wtf/text/WTFString.h>
 
@@ -182,6 +183,8 @@ public:
     void addSecKeyProxyStore(Ref<SecKeyProxyStore>&&);
 #endif
 
+    auto& weakPtrFactory() const { return m_weakFactory; }
+
 private:
     explicit WebsiteDataStore(PAL::SessionID);
     explicit WebsiteDataStore(Configuration, PAL::SessionID);
@@ -212,6 +215,7 @@ private:
 
     void maybeRegisterWithSessionIDMap();
 
+    WeakPtrFactory<WebsiteDataStore> m_weakFactory;
     const PAL::SessionID m_sessionID;
 
     const Configuration m_configuration;