Title: [278449] trunk/Source/WebKit
Revision
278449
Author
sihui_...@apple.com
Date
2021-06-03 23:35:37 -0700 (Thu, 03 Jun 2021)

Log Message

Make WebIDBServer use WorkQueue instead of Thread
https://bugs.webkit.org/show_bug.cgi?id=226589

Reviewed by Chris Dumez.

This matches other storage manager classes and makes management of thread lifetime much easier. We used to
destroy WebIDBServer aggressively (when there is no connection or task left) to ensure thread of WebIDBServer
does not stay around idly, and that led us to create new WebIDBServer for new task right after destroying
WebIDBServer and before databases are properly closed in some cases, which can caused issues like database is
locked during new task. With WorkQueue, we don't need to manage the threads, and we can close WebIDBServer
when session is destroyed.

* NetworkProcess/IndexedDB/WebIDBServer.cpp:
(WebKit::WebIDBServer::create):
(WebKit::WebIDBServer::WebIDBServer):
(WebKit::WebIDBServer::~WebIDBServer):
(WebKit::WebIDBServer::getOrigins):
(WebKit::WebIDBServer::closeAndDeleteDatabasesModifiedSince):
(WebKit::WebIDBServer::closeAndDeleteDatabasesForOrigins):
(WebKit::WebIDBServer::renameOrigin):
(WebKit::WebIDBServer::addConnection):
(WebKit::WebIDBServer::removeConnection):
(WebKit::WebIDBServer::postTask):
(WebKit::WebIDBServer::postTaskReply):
(WebKit::WebIDBServer::close):
(WebKit::m_closeCallback): Deleted.
(WebKit::WebIDBServer::dispatchToThread): Deleted.
(WebKit::WebIDBServer::tryClose): Deleted.
* NetworkProcess/IndexedDB/WebIDBServer.h:
* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::didClose):
(WebKit::NetworkProcess::destroySession):
(WebKit::NetworkProcess::createWebIDBServer):

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (278448 => 278449)


--- trunk/Source/WebKit/ChangeLog	2021-06-04 04:16:39 UTC (rev 278448)
+++ trunk/Source/WebKit/ChangeLog	2021-06-04 06:35:37 UTC (rev 278449)
@@ -1,3 +1,39 @@
+2021-06-03  Sihui Liu  <sihui_...@apple.com>
+
+        Make WebIDBServer use WorkQueue instead of Thread
+        https://bugs.webkit.org/show_bug.cgi?id=226589
+
+        Reviewed by Chris Dumez.
+
+        This matches other storage manager classes and makes management of thread lifetime much easier. We used to
+        destroy WebIDBServer aggressively (when there is no connection or task left) to ensure thread of WebIDBServer 
+        does not stay around idly, and that led us to create new WebIDBServer for new task right after destroying 
+        WebIDBServer and before databases are properly closed in some cases, which can caused issues like database is 
+        locked during new task. With WorkQueue, we don't need to manage the threads, and we can close WebIDBServer
+        when session is destroyed.
+
+        * NetworkProcess/IndexedDB/WebIDBServer.cpp:
+        (WebKit::WebIDBServer::create):
+        (WebKit::WebIDBServer::WebIDBServer):
+        (WebKit::WebIDBServer::~WebIDBServer):
+        (WebKit::WebIDBServer::getOrigins):
+        (WebKit::WebIDBServer::closeAndDeleteDatabasesModifiedSince):
+        (WebKit::WebIDBServer::closeAndDeleteDatabasesForOrigins):
+        (WebKit::WebIDBServer::renameOrigin):
+        (WebKit::WebIDBServer::addConnection):
+        (WebKit::WebIDBServer::removeConnection):
+        (WebKit::WebIDBServer::postTask):
+        (WebKit::WebIDBServer::postTaskReply):
+        (WebKit::WebIDBServer::close):
+        (WebKit::m_closeCallback): Deleted.
+        (WebKit::WebIDBServer::dispatchToThread): Deleted.
+        (WebKit::WebIDBServer::tryClose): Deleted.
+        * NetworkProcess/IndexedDB/WebIDBServer.h:
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::didClose):
+        (WebKit::NetworkProcess::destroySession):
+        (WebKit::NetworkProcess::createWebIDBServer):
+
 2021-06-03  Chris Dumez  <cdu...@apple.com>
 
         stopMakingViewBlankDueToLackOfRenderingUpdate logging shows even if we never made the view blank

Modified: trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp (278448 => 278449)


--- trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp	2021-06-04 04:16:39 UTC (rev 278448)
+++ trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp	2021-06-04 06:35:37 UTC (rev 278449)
@@ -28,20 +28,17 @@
 
 #include "WebIDBConnectionToClient.h"
 #include "WebIDBServerMessages.h"
-#include <WebCore/SQLiteDatabaseTracker.h>
 #include <WebCore/StorageQuotaManager.h>
 
 namespace WebKit {
 
-Ref<WebIDBServer> WebIDBServer::create(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester, CompletionHandler<void()>&& closeCallback)
+Ref<WebIDBServer> WebIDBServer::create(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester)
 {
-    return adoptRef(*new WebIDBServer(sessionID, directory, WTFMove(spaceRequester), WTFMove(closeCallback)));
+    return adoptRef(*new WebIDBServer(sessionID, directory, WTFMove(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))
+WebIDBServer::WebIDBServer(PAL::SessionID sessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&& spaceRequester)
+    : m_queue(WorkQueue::create("com.apple.WebKit.IndexedDBServer"))
 {
     ASSERT(RunLoop::isMain());
 
@@ -56,8 +53,6 @@
 WebIDBServer::~WebIDBServer()
 {
     ASSERT(RunLoop::isMain());
-    // close() has to be called to make sure thread exits.
-    ASSERT(!m_closeCallback);
     ASSERT(!m_isSuspended);
 }
 
@@ -65,13 +60,13 @@
 {
     ASSERT(RunLoop::isMain());
 
-    postTask([this, protectedThis = makeRef(*this), callback = WTFMove(callback), token = m_dataTaskCounter.count()]() mutable {
+    postTask([this, protectedThis = makeRef(*this), callback = WTFMove(callback)]() mutable {
         ASSERT(!RunLoop::isMain());
 
         Locker locker { m_serverLock };
-        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token), origins = crossThreadCopy(m_server->getOrigins())]() mutable {
+        postTaskReply([callback = WTFMove(callback), origins = crossThreadCopy(m_server->getOrigins())]() mutable {
             callback(WTFMove(origins));
-        }));
+        });
     });
 }
 
@@ -79,14 +74,14 @@
 {
     ASSERT(RunLoop::isMain());
 
-    postTask([this, protectedThis = makeRef(*this), modificationTime, callback = WTFMove(callback), token = m_dataTaskCounter.count()]() mutable {
+    postTask([this, protectedThis = makeRef(*this), modificationTime, callback = WTFMove(callback)]() mutable {
         ASSERT(!RunLoop::isMain());
 
         Locker locker { m_serverLock };
         m_server->closeAndDeleteDatabasesModifiedSince(modificationTime);
-        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
+        postTaskReply([callback = WTFMove(callback)]() mutable {
             callback();
-        }));
+        });
     });
 }
 
@@ -94,14 +89,14 @@
 {
     ASSERT(RunLoop::isMain());
 
-    postTask([this, protectedThis = makeRef(*this), originDatas = originDatas.isolatedCopy(), callback = WTFMove(callback), token = m_dataTaskCounter.count()] () mutable {
+    postTask([this, protectedThis = makeRef(*this), originDatas = originDatas.isolatedCopy(), callback = WTFMove(callback)] () mutable {
         ASSERT(!RunLoop::isMain());
 
         Locker locker { m_serverLock };
         m_server->closeAndDeleteDatabasesForOrigins(originDatas);
-        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
+        postTaskReply([callback = WTFMove(callback)]() mutable {
             callback();
-        }));
+        });
     });
 }
 
@@ -109,14 +104,14 @@
 {
     ASSERT(RunLoop::isMain());
 
-    postTask([this, protectedThis = makeRef(*this), oldOrigin = oldOrigin.isolatedCopy(), newOrigin = newOrigin.isolatedCopy(), callback = WTFMove(callback), token = m_dataTaskCounter.count()] () mutable {
+    postTask([this, protectedThis = makeRef(*this), oldOrigin = oldOrigin.isolatedCopy(), newOrigin = newOrigin.isolatedCopy(), callback = WTFMove(callback)] () mutable {
         ASSERT(!RunLoop::isMain());
 
         Locker locker { m_serverLock };
         m_server->renameOrigin(oldOrigin, newOrigin);
-        postTaskReply(CrossThreadTask([callback = WTFMove(callback), token = WTFMove(token)]() mutable {
+        postTaskReply([callback = WTFMove(callback)]() mutable {
             callback();
-        }));
+        });
     });
 }
 
@@ -370,7 +365,7 @@
         m_server->registerConnection(iter->value->connectionToClient());
     });
     m_connections.add(connection);
-    connection.addThreadMessageReceiver(Messages::WebIDBServer::messageReceiverName(), this);
+    connection.addWorkQueueMessageReceiver(Messages::WebIDBServer::messageReceiverName(), m_queue.get(), this);
 }
 
 void WebIDBServer::removeConnection(IPC::Connection& connection)
@@ -380,7 +375,7 @@
     if (!m_connections.remove(connection))
         return;
 
-    connection.removeThreadMessageReceiver(Messages::WebIDBServer::messageReceiverName());
+    connection.removeWorkQueueMessageReceiver(Messages::WebIDBServer::messageReceiverName());
     postTask([this, protectedThis = makeRef(*this), connectionID = connection.uniqueID()] {
         auto connection = m_connectionMap.take(connectionID);
 
@@ -389,8 +384,6 @@
         Locker locker { m_serverLock };
         m_server->unregisterConnection(connection->connectionToClient());
     });
-
-    tryClose();
 }
 
 void WebIDBServer::postTask(Function<void()>&& task)
@@ -397,58 +390,37 @@
 {
     ASSERT(RunLoop::isMain());
 
-    CrossThreadTaskHandler::postTask(CrossThreadTask(WTFMove(task)));
+    m_queue->dispatch(WTFMove(task));
 }
 
-void WebIDBServer::dispatchToThread(Function<void()>&& task)
+void WebIDBServer::postTaskReply(Function<void()>&& taskReply)
 {
-    CrossThreadTaskHandler::postTask(CrossThreadTask(WTFMove(task)));
+    ASSERT(!RunLoop::isMain());
+
+    RunLoop::main().dispatch(WTFMove(taskReply));
 }
 
-void WebIDBServer::close()
+void WebIDBServer::close(CompletionHandler<void()>&& completionHandler)
 {
     ASSERT(RunLoop::isMain());
-    if (!m_closeCallback)
-        return;
 
     // Remove the references held by IPC::Connection.
     for (auto& connection : m_connections)
-        connection.removeThreadMessageReceiver(Messages::WebIDBServer::messageReceiverName());
+        connection.removeWorkQueueMessageReceiver(Messages::WebIDBServer::messageReceiverName());
 
-    CrossThreadTaskHandler::setCompletionCallback([this, protectedThis = makeRef(*this)]() mutable {
-        ASSERT(!RunLoop::isMain());
+    // Dispatch last task to clean up.
+   postTask([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {
+        m_connectionMap.clear();
 
-        m_connectionMap.clear();
         {
             Locker locker { m_serverLock };
             m_server = nullptr;
         }
 
-        callOnMainRunLoop([protectedThis = WTFMove(protectedThis)] { });
+        postTaskReply([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {
+            completionHandler();
+        });
     });
-
-#if PLATFORM(IOS_FAMILY)
-    // Network process may become active and close WebIDBServer before receiving resume message.
-    resume();
-#endif
-
-    {
-        Locker locker { m_serverLock };
-        if (m_server)
-            m_server->stopDatabaseActivitiesOnMainThread();
-    }
-
-    CrossThreadTaskHandler::kill();
-
-    m_closeCallback();
 }
 
-void WebIDBServer::tryClose()
-{
-    if (!m_connections.computesEmpty() || m_dataTaskCounter.value())
-        return;
-
-    close();
-}
-
 } // namespace WebKit

Modified: trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h (278448 => 278449)


--- trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h	2021-06-04 04:16:39 UTC (rev 278448)
+++ trunk/Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h	2021-06-04 06:35:37 UTC (rev 278449)
@@ -43,9 +43,9 @@
 
 namespace WebKit {
 
-class WebIDBServer final : public CrossThreadTaskHandler, public IPC::Connection::ThreadMessageReceiverRefCounted {
+class WebIDBServer final : public IPC::Connection::WorkQueueMessageReceiver {
 public:
-    static Ref<WebIDBServer> create(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&, CompletionHandler<void()>&&);
+    static Ref<WebIDBServer> create(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&);
 
     void getOrigins(CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&&);
     void closeAndDeleteDatabasesModifiedSince(WallTime, CompletionHandler<void()>&& callback);
@@ -87,16 +87,16 @@
     void removeConnection(IPC::Connection&);
 
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
-    void dispatchToThread(WTF::Function<void()>&&);
-    void close();
+    void close(CompletionHandler<void()>&& = { });
 
 private:
-    WebIDBServer(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&, CompletionHandler<void()>&&);
+    WebIDBServer(PAL::SessionID, const String& directory, WebCore::IDBServer::IDBServer::StorageQuotaManagerSpaceRequester&&);
     ~WebIDBServer();
 
     void postTask(WTF::Function<void()>&&);
+    void postTaskReply(Function<void()>&&);
 
-    void tryClose();
+    Ref<WorkQueue> m_queue;
 
     Lock m_serverLock;
     std::unique_ptr<WebCore::IDBServer::IDBServer> m_server WTF_GUARDED_BY_LOCK(m_serverLock);
@@ -104,12 +104,6 @@
 
     HashMap<IPC::Connection::UniqueID, std::unique_ptr<WebIDBConnectionToClient>> m_connectionMap;
     WeakHashSet<IPC::Connection> m_connections; // Only used on the main thread.
-
-    enum DataTaskCounterType { };
-    using DataTaskCounter = RefCounter<DataTaskCounterType>;
-    using DataTaskCounterToken = DataTaskCounter::Token;
-    DataTaskCounter m_dataTaskCounter;
-    CompletionHandler<void()> m_closeCallback;
 };
 
 } // namespace WebKit

Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (278448 => 278449)


--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp	2021-06-04 04:16:39 UTC (rev 278448)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp	2021-06-04 06:35:37 UTC (rev 278449)
@@ -273,10 +273,10 @@
         platformFlushCookies(networkSession.sessionID(), [callbackAggregator] { });
     });
 
-    // Make sure references to NetworkProcess in spaceRequester and closeHandler is removed.
+    // Make sure reference to NetworkProcess in spaceRequester is removed.
     auto servers = std::exchange(m_webIDBServers, { });
     for (auto& server : servers.values())
-        server->close();
+        server->close([callbackAggregator] { });
 }
 
 void NetworkProcess::didCreateDownload()
@@ -553,6 +553,8 @@
 #endif
 
     m_storageManagerSet->remove(sessionID);
+    if (auto server = m_webIDBServers.take(sessionID))
+        server->close();
 }
 
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
@@ -2348,10 +2350,7 @@
     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));
+    return WebIDBServer::create(sessionID, path, WTFMove(spaceRequester));
 }
 
 WebIDBServer& NetworkProcess::webIDBServer(PAL::SessionID sessionID)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to