Attachment 341793 Details for Bug 186205 – Patch

[patch] Patch

bug-186205-20180601151601.patch (text/plain), 15.76 KB, created by Chris Dumez on 2018-06-01 15:16:02 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-01 15:16:02 PDT

Size: 15.76 KB

patch

obsolete

>Subversion Revision: 232397
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ff60d3759cdc1236638e66a7ac7578cc06904e6c..a388cfc778a92d0fcd62561aa5b0c1b421f5b698 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] Simplify the formatting class implementation by pushing down some of the logic to the Geometry class
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 84c7e4418b610f40b871bab463d95bfb248b6dc5..8b9cfc4211b3618f204b09c506e14e48f03ba3be 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [GTK] Crash in WebKitFaviconDatabase when pageURL is unset
>diff --git a/Source/WebCore/workers/service/server/SWServer.cpp b/Source/WebCore/workers/service/server/SWServer.cpp
>index dc325ddd9c6ea5db67766abaa843e63fc37d4390..a20822aaa9b1152ca9447a058c31fd57a5fd662a 100644
>--- a/Source/WebCore/workers/service/server/SWServer.cpp
>+++ b/Source/WebCore/workers/service/server/SWServer.cpp
>@@ -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);
> }
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 29d62469d487e02580e35f9d739c657bbc181e3c..db13dbc9201c6f41b2c85ea2b58792a2a3e500f0 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -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)
>diff --git a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>index ddb8c251285b5fbcc7a6c38d35b72e18430bcf72..dc872648467e2619f2e47334e548c2bb57bc6330 100644
>--- a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>+++ b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>@@ -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));
>diff --git a/Source/WebKit/StorageProcess/StorageProcess.cpp b/Source/WebKit/StorageProcess/StorageProcess.cpp
>index 421ce76c65b3c1b7168aa2636b628a05db6ba422..a7903156b0b29aa38e7e6668992b8ab457b0e5fa 100644
>--- a/Source/WebKit/StorageProcess/StorageProcess.cpp
>+++ b/Source/WebKit/StorageProcess/StorageProcess.cpp
>@@ -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);
>diff --git a/Source/WebKit/StorageProcess/StorageProcess.h b/Source/WebKit/StorageProcess/StorageProcess.h
>index ea0f71397767357f89abe5f435bb7a30c2b26907..ab1bb2e097876ee8132636d0a83ccb6561309e30 100644
>--- a/Source/WebKit/StorageProcess/StorageProcess.h
>+++ b/Source/WebKit/StorageProcess/StorageProcess.h
>@@ -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);
>diff --git a/Source/WebKit/StorageProcess/StorageProcess.messages.in b/Source/WebKit/StorageProcess/StorageProcess.messages.in
>index 56aa746f7ebb7a7496391d3edc34f78456345396..31a0507884f82de8a916648ab69612e361d1577b 100644
>--- a/Source/WebKit/StorageProcess/StorageProcess.messages.in
>+++ b/Source/WebKit/StorageProcess/StorageProcess.messages.in
>@@ -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)
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 9b7ad82c194faa4e087f046d03b570a8d4d98fd7..e7e916907d243b2669d6c4be8613d905e27e9b09 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -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);
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>index 548761a3fc493c91fc733acf1b983c50d9d0e44e..68192de1129dc43f736b91f3d370f06b9cb8ff9e 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>@@ -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());
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>index e3d1e908db37dcca391e5c8ab156c38151ec86f5..3abf8ff51f442b8508b376925f5aad3a8b0685ab 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
>@@ -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;

Actions: View | Formatted Diff | Diff

Attachments on bug 186205: 341783 | 341793