Attachment 425748 Details for Bug 224305 – Patch

[patch] Patch

bug-224305-20210412082840.patch (text/plain), 18.67 KB, created by Sihui Liu on 2021-04-12 08:28:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2021-04-12 08:28:41 PDT

Size: 18.67 KB

patch

obsolete

>Subversion Revision: 275801
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 7b554b81b3506d6f516417996aba222efe7696e2..4c772704eaf07668ae9170c9f51a34aba5bf6389 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,53 @@
>+2021-04-12  Sihui Liu  <sihui_liu@apple.com>
>+
>+        Create WebIDBServer only when it is needed
>+        https://bugs.webkit.org/show_bug.cgi?id=224305
>+        rdar://71962196
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Currently each WebIDBServer has a separate thread, so we don't want to create or keep WebIDBServer if it's not 
>+        in use. There are two cases where network process needs a WebIDBServer:
>+        1. handle requests from UI process to collect or remove data
>+        2. handle requests from Web process to perform IDB operations 
>+
>+        Previously, we created a WebIDBServer when network process connects to a web process, but that does not mean web
>+        process will perform IDB operations and we may create a thread that's not used. To avoid this, add a new message
>+        AddIDBConnection for web process to ensure network process has WebIDBServer when it's about to perform operation.
>+
>+        Also, previously network process removes a WebIDBServer when session is removed and WebIDBServer is not binded 
>+        with any web process connection. Now we remove WebIDBServer when it's done handling requests, that is count of
>+        pending requests from UI process is 0 and WebIDBServer is not binded with web process connection. We also remove
>+        WebIDBServer at when network process is about to be destroyed (NetworkProcess::didClose) so we can break the 
>+        reference cycle of NetworkProcess-WebIDBServer-IDBServer, and make sure thread exits.
>+
>+        * NetworkProcess/IndexedDB/WebIDBServer.cpp:
>+        (WebKit::WebIDBServer::create):
>+        (WebKit::WebIDBServer::WebIDBServer):
>+        (WebKit::m_closeCallback):
>+        (WebKit::WebIDBServer::~WebIDBServer):
>+        (WebKit::WebIDBServer::getOrigins):
>+        (WebKit::WebIDBServer::closeAndDeleteDatabasesModifiedSince):
>+        (WebKit::WebIDBServer::closeAndDeleteDatabasesForOrigins):
>+        (WebKit::WebIDBServer::renameOrigin):
>+        (WebKit::WebIDBServer::removeConnection):
>+        (WebKit::WebIDBServer::close):
>+        (WebKit::WebIDBServer::tryClose):
>+        * NetworkProcess/IndexedDB/WebIDBServer.h:
>+        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
>+        (WebKit::NetworkConnectionToWebProcess::addIDBConnection):
>+        * NetworkProcess/NetworkConnectionToWebProcess.h:
>+        * NetworkProcess/NetworkConnectionToWebProcess.messages.in:
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::NetworkProcess::didClose):
>+        (WebKit::NetworkProcess::createNetworkConnectionToWebProcess):
>+        (WebKit::NetworkProcess::destroySession):
>+        (WebKit::NetworkProcess::createWebIDBServer):
>+        (WebKit::NetworkProcess::connectionToWebProcessClosed):
>+        (WebKit::NetworkProcess::removeWebIDBServerIfPossible): Deleted. Move the removal code to WebIDBServer.
>+        * WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:
>+        (WebKit::WebIDBConnectionToServer::WebIDBConnectionToServer):
>+
> 2021-04-10  Sihui Liu  <sihui_liu@apple.com>
> 
>         Unreviewed, reverting r275794, r275784, r275779
>diff --git a/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp b/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp
>index d21e571eb17e1e737372b945360e95a5939065f7..6af595ffc106590da817d7154624fb1ff6ee6597 100644
>--- a/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp
>+++ b/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp
>@@ -34,13 +34,15 @@
> 
> namespace WebKit {
> 
>-Ref<WebIDBServer> WebIDBServer::create(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester)
>+Ref<WebIDBServer> WebIDBServer::create(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester, CompletionHandler<void()>&& closeCallback)
> {
>-    return adoptRef(*new WebIDBServer(sessionID, directory, WTFMove(spaceRequester)));
>+    return adoptRef(*new WebIDBServer(sessionID, directory, WTFMove(spaceRequester), WTFMove(closeCallback)));
> }
> 
>-WebIDBServer::WebIDBServer(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester)
>+WebIDBServer::WebIDBServer(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester, CompletionHandler<void()>&& closeCallback)
>     : CrossThreadTaskHandler("com.apple.WebKit.IndexedDBServer", WTF::CrossThreadTaskHandler::AutodrainedPoolForRunLoop::Use)
>+    , m_dataTaskCounter([this](RefCounterEvent) { tryClose(); })
>+    , m_closeCallback(WTFMove(closeCallback))
> {
>     ASSERT(RunLoop::isMain());
> 
>@@ -55,17 +57,19 @@ WebIDBServer::WebIDBServer(PAL::SessionID sessionID, const String& directory, We
> WebIDBServer::~WebIDBServer()
> {
>     ASSERT(RunLoop::isMain());
>+    // close() has to be called to make sure thread exits.
>+    ASSERT(!m_closeCallback);
> }
> 
> void WebIDBServer::getOrigins(CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&& callback)
> {
>     ASSERT(RunLoop::isMain());
> 
>-    postTask([this, protectedThis = makeRef(*this), callback = WTFMove(callback)]() mutable {
>+    postTask([this, protectedThis = makeRef(*this), callback = WTFMove(callback), token = m_dataTaskCounter.count()]() mutable {
>         ASSERT(!RunLoop::isMain());
> 
>         LockHolder locker(m_server->lock());
>-        postTaskReply(CrossThreadTask([callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable {
>+        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token), origins = crossThreadCopy(m_server->getOrigins())]() mutable {
>             callback(WTFMove(origins));
>         }));
>     });
>@@ -75,12 +79,12 @@ void WebIDBServer::closeAndDeleteDatabasesModifiedSince(WallTime modificationTim
> {
>     ASSERT(RunLoop::isMain());
> 
>-    postTask([this, protectedThis = makeRef(*this), modificationTime, callback = WTFMove(callback)]() mutable {
>+    postTask([this, protectedThis = makeRef(*this), modificationTime, callback = WTFMove(callback), token = m_dataTaskCounter.count()]() mutable {
>         ASSERT(!RunLoop::isMain());
> 
>         LockHolder locker(m_server->lock());
>         m_server->closeAndDeleteDatabasesModifiedSince(modificationTime);
>-        postTaskReply(CrossThreadTask([callback = WTFMove(callback)]() mutable {
>+        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
>             callback();
>         }));
>     });
>@@ -90,12 +94,12 @@ void WebIDBServer::closeAndDeleteDatabasesForOrigins(const Vector<WebCore::Secur
> {
>     ASSERT(RunLoop::isMain());
> 
>-    postTask([this, protectedThis = makeRef(*this), originDatas = originDatas.isolatedCopy(), callback = WTFMove(callback)] () mutable {
>+    postTask([this, protectedThis = makeRef(*this), originDatas = originDatas.isolatedCopy(), callback = WTFMove(callback), token = m_dataTaskCounter.count()] () mutable {
>         ASSERT(!RunLoop::isMain());
> 
>         LockHolder locker(m_server->lock());
>         m_server->closeAndDeleteDatabasesForOrigins(originDatas);
>-        postTaskReply(CrossThreadTask([callback = WTFMove(callback)]() mutable {
>+        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
>             callback();
>         }));
>     });
>@@ -105,12 +109,14 @@ void WebIDBServer::renameOrigin(const WebCore::SecurityOriginData& oldOrigin, co
> {
>     ASSERT(RunLoop::isMain());
> 
>-    postTask([this, protectedThis = makeRef(*this), oldOrigin = oldOrigin.isolatedCopy(), newOrigin = newOrigin.isolatedCopy(), callback = WTFMove(callback)] () mutable {
>+    postTask([this, protectedThis = makeRef(*this), oldOrigin = oldOrigin.isolatedCopy(), newOrigin = newOrigin.isolatedCopy(), callback = WTFMove(callback), token = m_dataTaskCounter.count()] () mutable {
>         ASSERT(!RunLoop::isMain());
> 
>         LockHolder locker(m_server->lock());
>         m_server->renameOrigin(oldOrigin, newOrigin);
>-        postTaskReply(CrossThreadTask(WTFMove(callback)));
>+        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
>+            callback();
>+        }));
>     });
> }
> 
>@@ -370,8 +376,11 @@ void WebIDBServer::removeConnection(IPC::Connection& connection)
> {
>     ASSERT(RunLoop::isMain());
> 
>-    m_connections.remove(&connection);
>-    connection.removeThreadMessageReceiver(Messages::WebIDBServer::messageReceiverName());
>+    auto* takenConnection = m_connections.take(&connection);
>+    if (!takenConnection)
>+        return;
>+
>+    takenConnection->removeThreadMessageReceiver(Messages::WebIDBServer::messageReceiverName());
>     postTask([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()] {
>         auto connection = m_connectionMap.take(connectionID);
> 
>@@ -380,6 +389,8 @@ void WebIDBServer::removeConnection(IPC::Connection& connection)
>         LockHolder locker(m_server->lock());
>         m_server->unregisterConnection(connection->connectionToClient());
>     });
>+
>+    tryClose();
> }
> 
> void WebIDBServer::postTask(Function<void()>&& task)
>@@ -397,6 +408,8 @@ void WebIDBServer::dispatchToThread(Function<void()>&& task)
> void WebIDBServer::close()
> {
>     ASSERT(RunLoop::isMain());
>+    if (!m_closeCallback)
>+        return;
> 
>     // Remove the references held by IPC::Connection.
>     for (auto* connection : m_connections)
>@@ -413,6 +426,16 @@ void WebIDBServer::close()
> 
>         CrossThreadTaskHandler::kill();
>     });
>+
>+    m_closeCallback();
>+}
>+
>+void WebIDBServer::tryClose()
>+{
>+    if (!m_connections.isEmpty() || m_dataTaskCounter.value())
>+        return;
>+
>+    close();
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h b/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h
>index 256e295650a620d83cf5ca87a8bc0cc3114a1d2e..62fe7008a0ca9ac0c1f2bd1d7a4917276231ee34 100644
>--- a/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h
>+++ b/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h
>@@ -31,6 +31,7 @@
> #include <WebCore/IDBServer.h>
> #include <WebCore/StorageQuotaManager.h>
> #include <wtf/CrossThreadTaskHandler.h>
>+#include <wtf/RefCounter.h>
> 
> namespace WebCore {
> class StorageQuotaManager;
>@@ -43,7 +44,7 @@ namespace WebKit {
> 
> class WebIDBServer final : public CrossThreadTaskHandler, public IPC::Connection::ThreadMessageReceiverRefCounted {
> public:
>-    static Ref<WebIDBServer> create(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&);
>+    static Ref<WebIDBServer> create(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&, CompletionHandler<void()>&&);
> 
>     void getOrigins(CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&&);
>     void closeAndDeleteDatabasesModifiedSince(WallTime, CompletionHandler<void()>&& callback);
>@@ -88,18 +89,25 @@ public:
>     void dispatchToThread(WTF::Function<void()>&&);
>     void close();
> 
>-    bool hasConnection() const { return !m_connections.isEmpty(); }
> private:
>-    WebIDBServer(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&);
>+    WebIDBServer(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&, CompletionHandler<void()>&&);
>     ~WebIDBServer();
> 
>     void postTask(WTF::Function<void()>&&);
> 
>+    void tryClose();
>+
>     std::unique_ptr<WebCore::IDBServer::IDBServer> m_server;
>     bool m_isSuspended { false };
> 
>     HashMap<IPC::Connection::UniqueID, std::unique_ptr<WebIDBConnectionToClient>> m_connectionMap;
>     HashSet<IPC::Connection*> m_connections;
>+
>+    enum DataTaskCounterType { };
>+    using DataTaskCounter = RefCounter<DataTaskCounterType>;
>+    using DataTaskCounterToken = DataTaskCounter::Token;
>+    DataTaskCounter m_dataTaskCounter;
>+    CompletionHandler<void()> m_closeCallback;
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>index 051550e777e507a3ada73c1be9298275017a20bc..79a3dd786cfd9099ce253c796d3e8807f7547336 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>@@ -1241,6 +1241,10 @@ void NetworkConnectionToWebProcess::prioritizeResourceLoads(Vector<ResourceLoadI
>     session->networkLoadScheduler().prioritizeLoads(loads);
> }
> 
>+void NetworkConnectionToWebProcess::addIDBConnection()
>+{
>+    m_networkProcess->webIDBServer(m_sessionID).addConnection(m_connection.get(), m_webProcessIdentifier);
>+}
> 
> } // namespace WebKit
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>index 2ec7d11ca0ea9402a4ca7a928c73d83b1e6ef2f6..2815edec73db5b9c74ee28641fba3fb47246fdd2 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>@@ -186,6 +186,8 @@ public:
> 
>     void broadcastConsoleMessage(JSC::MessageSource, JSC::MessageLevel, const String& message);
> 
>+    void addIDBConnection();
>+
> private:
>     NetworkConnectionToWebProcess(NetworkProcess&, WebCore::ProcessIdentifier, PAL::SessionID, IPC::Connection::Identifier);
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>index a098412a5f3d55fe3848b223f9dffa98c36614c6..07bfce4491cd5306b4faa28bc4c85a53cef0eab7 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>@@ -107,4 +107,6 @@ messages -> NetworkConnectionToWebProcess LegacyReceiver {
> #endif
>     SetResourceLoadSchedulingMode(WebCore::PageIdentifier webPageID, enum:uint8_t WebCore::LoadSchedulingMode mode)
>     PrioritizeResourceLoads(Vector<uint64_t> loadIdentifiers)
>+
>+    AddIDBConnection()
> }
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 03f29512036f9bd3657a873cae8848fa6847a7ff..126e02566c6a838bfe43871aca97ae0be89f7a6b 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -272,6 +272,10 @@ void NetworkProcess::didClose(IPC::Connection&)
>     forEachNetworkSession([&] (auto& networkSession) {
>         platformFlushCookies(networkSession.sessionID(), [callbackAggregator] { });
>     });
>+
>+    // Make sure references to NetworkProcess in spaceRequester and closeHandler is removed.
>+    for (auto& server : m_webIDBServers.values())
>+        server->close();
> }
> 
> void NetworkProcess::didCreateDownload()
>@@ -391,8 +395,6 @@ void NetworkProcess::createNetworkConnectionToWebProcess(ProcessIdentifier ident
>     connection.setOnLineState(NetworkStateNotifier::singleton().onLine());
> 
>     m_storageManagerSet->addConnection(connection.connection());
>-
>-    webIDBServer(sessionID).addConnection(connection.connection(), identifier);
> }
> 
> void NetworkProcess::clearCachedCredentials(PAL::SessionID sessionID)
>@@ -552,8 +554,6 @@ void NetworkProcess::destroySession(PAL::SessionID sessionID)
> #endif
> 
>     m_storageManagerSet->remove(sessionID);
>-
>-    removeWebIDBServerIfPossible(sessionID);
> }
> 
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>@@ -2330,10 +2330,13 @@ Ref<WebIDBServer> NetworkProcess::createWebIDBServer(PAL::SessionID sessionID)
>         path = m_idbDatabasePaths.get(sessionID);
>     }
> 
>-    return WebIDBServer::create(sessionID, path, [this, weakThis = makeWeakPtr(this), sessionID](const auto& origin, uint64_t spaceRequested) {
>-        RefPtr<StorageQuotaManager> storageQuotaManager = weakThis ? this->storageQuotaManager(sessionID, origin) : nullptr;
>-        return storageQuotaManager ? storageQuotaManager->requestSpaceOnBackgroundThread(spaceRequested) : StorageQuotaManager::Decision::Deny;
>-    });
>+    auto spaceRequester = [protectedThis = makeRef(*this), sessionID](const auto& origin, uint64_t spaceRequested) {
>+        return protectedThis->storageQuotaManager(sessionID, origin)->requestSpaceOnBackgroundThread(spaceRequested);
>+    };
>+    auto closeHandler = [protectedThis = makeRef(*this), sessionID]() {
>+        protectedThis->m_webIDBServers.remove(sessionID);
>+    };
>+    return WebIDBServer::create(sessionID, path, WTFMove(spaceRequester), WTFMove(closeHandler));
> }
> 
> WebIDBServer& NetworkProcess::webIDBServer(PAL::SessionID sessionID)
>@@ -2363,24 +2366,6 @@ void NetworkProcess::setSessionStorageQuotaManagerIDBRootPath(PAL::SessionID ses
>     sessionStorageQuotaManager->setIDBRootPath(idbRootPath);
> }
> 
>-void NetworkProcess::removeWebIDBServerIfPossible(PAL::SessionID sessionID)
>-{
>-    ASSERT(RunLoop::isMain());
>-
>-    auto iterator = m_webIDBServers.find(sessionID);
>-    if (iterator == m_webIDBServers.end())
>-        return;
>-
>-    if (m_networkSessions.contains(sessionID))
>-        return;
>-
>-    if (iterator->value->hasConnection())
>-        return;
>-
>-    iterator->value->close();
>-    m_webIDBServers.remove(iterator);
>-}
>-
> void NetworkProcess::syncLocalStorage(CompletionHandler<void()>&& completionHandler)
> {
>     m_storageManagerSet->waitUntilSyncingLocalStorageFinished();
>@@ -2667,10 +2652,8 @@ void NetworkProcess::connectionToWebProcessClosed(IPC::Connection& connection, P
> {
>     m_storageManagerSet->removeConnection(connection);
> 
>-    auto* webIDBServer = m_webIDBServers.get(sessionID);
>-    ASSERT(webIDBServer);
>-    webIDBServer->removeConnection(connection);
>-    removeWebIDBServerIfPossible(sessionID);
>+    if (auto* server = m_webIDBServers.get(sessionID))
>+        server->removeConnection(connection);
> }
> 
> NetworkConnectionToWebProcess* NetworkProcess::webProcessConnection(ProcessIdentifier identifier) const
>diff --git a/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp b/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp
>index f062975e430525c9ad9fbdc35ef60597d6aff058..e39e1c452bed69cc14af4cca77a204638709f04e 100644
>--- a/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp
>+++ b/Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp
>@@ -59,6 +59,7 @@ Ref<WebIDBConnectionToServer> WebIDBConnectionToServer::create()
> WebIDBConnectionToServer::WebIDBConnectionToServer()
>     : m_connectionToServer(IDBClient::IDBConnectionToServer::create(*this))
> {
>+    send(Messages::NetworkConnectionToWebProcess::AddIDBConnection());
> }
> 
> WebIDBConnectionToServer::~WebIDBConnectionToServer()

Actions: View | Formatted Diff | Diff

Attachments on bug 224305: 425445 | 425475 | 425521 | 425542 | 425555 | 425650 | 425652 | 425653 | 425666 | 425680 | 425681 | 425685 | 425686 | 425748