Attachment 345889 Details for Bug 188084 – Patch

[patch] Patch

bug-188084-20180726164704.patch (text/plain), 12.86 KB, created by Chris Dumez on 2018-07-26 16:47:04 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-07-26 16:47:04 PDT

Size: 12.86 KB

patch

obsolete

>Subversion Revision: 234279
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 5274849870080d35f19a637fa216002adb3f776f..8e0c2686901b3488c234993b3fb671125d02bb50 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,24 @@
>+2018-07-26  Chris Dumez  <cdumez@apple.com>
>+
>+        It should be possible to use WTF::CallbackAggregator from a background thread
>+        https://bugs.webkit.org/show_bug.cgi?id=188084
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make WTF::CallbackAggregator usable on background threads by updating its destructor to call the
>+        completion handler on whatever thread it is destroyed on, instead of always dispatching to the
>+        main thread before calling the completion handler.
>+
>+        Existing CallbackAggregator users that deal with threads have been updated to make sure they
>+        destroy the CallbackAggregator on the expected thread.
>+
>+        This will allow us to use CallbackAggregator in the ResourceLoadStatisticsMemoryStore, which lives
>+        on a background WorkQueue.
>+
>+        * wtf/CallbackAggregator.h:
>+        (WTF::CallbackAggregator::~CallbackAggregator):
>+        (WTF::CallbackAggregator::CallbackAggregator):
>+
> 2018-07-26  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         String(View) should have a splitAllowingEmptyEntries function instead of a flag parameter
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index e4b95a9787aafa447ed4ff94a5d75d517e480118..7f7e755e20263a6b97664ddec10b476f7bf7b038 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-07-26  Chris Dumez  <cdumez@apple.com>
>+
>+        It should be possible to use WTF::CallbackAggregator from a background thread
>+        https://bugs.webkit.org/show_bug.cgi?id=188084
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This code was passing CallbackAggregator objects for background thread and not being
>+        careful on which thread the CallbackAggregator gets destroyed. This patch fixes this
>+        since the CallbackAggregator destructor no longer takes care of dispatching to the
>+        main thread.
>+
>+        * NetworkProcess/cache/CacheStorageEngine.cpp:
>+        (WebKit::CacheStorage::Engine::clearAllCaches):
>+        (WebKit::CacheStorage::Engine::clearCachesForOrigin):
>+
> 2018-07-26  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         String(View) should have a splitAllowingEmptyEntries function instead of a flag parameter
>diff --git a/Source/WTF/wtf/CallbackAggregator.h b/Source/WTF/wtf/CallbackAggregator.h
>index 553bd459e1ec234dd7ad5e66c8972519cbfc078d..4eb60ea3c6083042ee18a7bf9ee16bd453dcc3ac 100644
>--- a/Source/WTF/wtf/CallbackAggregator.h
>+++ b/Source/WTF/wtf/CallbackAggregator.h
>@@ -37,24 +37,24 @@ public:
> 
>     ~CallbackAggregator()
>     {
>-        if (!m_callback)
>-            return;
>-        if (isMainThread()) {
>+        ASSERT(m_wasConstructedOnMainThread == isMainThread());
>+        if (m_callback)
>             m_callback();
>-            return;
>-        }
>-        callOnMainThread([callback = WTFMove(m_callback)] () mutable {
>-            callback();
>-        });
>     }
> 
> private:
>     explicit CallbackAggregator(CompletionHandler<void()>&& callback)
>         : m_callback(WTFMove(callback))
>+#if !ASSERT_DISABLED
>+        , m_wasConstructedOnMainThread(isMainThread())
>+#endif
>     {
>     }
> 
>     CompletionHandler<void()> m_callback;
>+#if !ASSERT_DISABLED
>+    bool m_wasConstructedOnMainThread;
>+#endif
> };
> 
> } // namespace WTF
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index ac02e9a2e15585ae0211cce744afdf747bea7766..8bfefe362a85bf2594845ac28f640933ca689669 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -578,7 +578,7 @@ void NetworkProcess::deleteWebsiteData(PAL::SessionID sessionID, OptionSet<Websi
>     });
> 
>     if (websiteDataTypes.contains(WebsiteDataType::DOMCache))
>-        CacheStorage::Engine::clearAllCaches(sessionID, clearTasksHandler.copyRef());
>+        CacheStorage::Engine::clearAllCaches(sessionID, [clearTasksHandler = clearTasksHandler.copyRef()] { });
> 
>     if (websiteDataTypes.contains(WebsiteDataType::DiskCache) && !sessionID.isEphemeral())
>         clearDiskCache(modifiedSince, [clearTasksHandler = WTFMove(clearTasksHandler)] { });
>@@ -622,7 +622,7 @@ void NetworkProcess::deleteWebsiteDataForOrigins(PAL::SessionID sessionID, Optio
> 
>     if (websiteDataTypes.contains(WebsiteDataType::DOMCache)) {
>         for (auto& originData : originDatas)
>-            CacheStorage::Engine::clearCachesForOrigin(sessionID, SecurityOriginData { originData }, clearTasksHandler.copyRef());
>+            CacheStorage::Engine::clearCachesForOrigin(sessionID, SecurityOriginData { originData }, [clearTasksHandler = clearTasksHandler.copyRef()] { });
>     }
> 
>     if (websiteDataTypes.contains(WebsiteDataType::DiskCache) && !sessionID.isEphemeral())
>diff --git a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>index 4946f396123ec166a6c477214f4bd1fdb8633500..b99b8884e8603c62425494a242b014d624290069 100644
>--- a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>+++ b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp
>@@ -183,17 +183,17 @@ void Engine::representation(PAL::SessionID sessionID, CompletionHandler<void(Str
>     });
> }
> 
>-void Engine::clearAllCaches(PAL::SessionID sessionID, Ref<WTF::CallbackAggregator>&& aggregator)
>+void Engine::clearAllCaches(PAL::SessionID sessionID, CompletionHandler<void()>&& completionHandler)
> {
>-    from(sessionID, [aggregator = WTFMove(aggregator)](auto& engine) mutable {
>-        engine.clearAllCaches(aggregator);
>+    from(sessionID, [completionHandler = WTFMove(completionHandler)](auto& engine) mutable {
>+        engine.clearAllCaches(WTFMove(completionHandler));
>     });
> }
> 
>-void Engine::clearCachesForOrigin(PAL::SessionID sessionID, WebCore::SecurityOriginData&& originData, Ref<WTF::CallbackAggregator>&& aggregator)
>+void Engine::clearCachesForOrigin(PAL::SessionID sessionID, WebCore::SecurityOriginData&& originData, CompletionHandler<void()>&& completionHandler)
> {
>-    from(sessionID, [originData = WTFMove(originData), aggregator = WTFMove(aggregator)](auto& engine) mutable {
>-        engine.clearCachesForOrigin(originData, aggregator);
>+    from(sessionID, [originData = WTFMove(originData), completionHandler = WTFMove(completionHandler)](auto& engine) mutable {
>+        engine.clearCachesForOrigin(originData, WTFMove(completionHandler));
>     });
> }
> 
>@@ -516,49 +516,87 @@ void Engine::fetchEntries(bool shouldComputeSize, WTF::CompletionHandler<void(Ve
>     }
> }
> 
>-void Engine::clearAllCaches(CallbackAggregator& taskHandler)
>+void Engine::clearAllCaches(CompletionHandler<void()>&& completionHandler)
> {
>+    ASSERT(RunLoop::isMain());
>+
>+    auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler));
>+
>     for (auto& caches : m_caches.values())
>-        caches->clear([taskHandler = makeRef(taskHandler)] { });
>+        caches->clear([callbackAggregator = callbackAggregator.copyRef()] { });
> 
>     if (!shouldPersist())
>         return;
> 
>-    m_ioQueue->dispatch([path = m_rootPath.isolatedCopy(), taskHandler = makeRef(taskHandler)] {
>+    clearAllCachesFromDisk([callbackAggregator = WTFMove(callbackAggregator)] { });
>+}
>+
>+void Engine::clearAllCachesFromDisk(CompletionHandler<void()>&& completionHandler)
>+{
>+    ASSERT(RunLoop::isMain());
>+
>+    m_ioQueue->dispatch([path = m_rootPath.isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable {
>         for (auto& filename : WebCore::FileSystem::listDirectory(path, "*")) {
>             if (WebCore::FileSystem::fileIsDirectory(filename, WebCore::FileSystem::ShouldFollowSymbolicLinks::No))
>                 deleteDirectoryRecursively(filename);
>         }
>+        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
>+            completionHandler();
>+        });
>     });
> }
> 
>-void Engine::clearCachesForOrigin(const WebCore::SecurityOriginData& origin, CallbackAggregator& taskHandler)
>+void Engine::clearCachesForOrigin(const WebCore::SecurityOriginData& origin, CompletionHandler<void()>&& completionHandler)
> {
>+    ASSERT(RunLoop::isMain());
>+
>+    auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler));
>+
>     for (auto& keyValue : m_caches) {
>         if (keyValue.key.topOrigin == origin || keyValue.key.clientOrigin == origin)
>-            keyValue.value->clear([taskHandler = makeRef(taskHandler)] { });
>+            keyValue.value->clear([callbackAggregator = callbackAggregator.copyRef()] { });
>     }
> 
>     if (!shouldPersist())
>         return;
> 
>+    clearCachesForOriginFromDisk(origin, [callbackAggregator = WTFMove(callbackAggregator)] { });
>+}
>+
>+void Engine::clearCachesForOriginFromDisk(const WebCore::SecurityOriginData& origin, CompletionHandler<void()>&& completionHandler)
>+{
>+    ASSERT(RunLoop::isMain());
>+
>+    auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler));
>+
>     for (auto& folderPath : WebCore::FileSystem::listDirectory(m_rootPath, "*")) {
>         if (!WebCore::FileSystem::fileIsDirectory(folderPath, WebCore::FileSystem::ShouldFollowSymbolicLinks::No))
>             continue;
>-        Caches::retrieveOriginFromDirectory(folderPath, *m_ioQueue, [this, protectedThis = makeRef(*this), origin, taskHandler = makeRef(taskHandler), folderPath] (std::optional<WebCore::ClientOrigin>&& folderOrigin) mutable {
>+        Caches::retrieveOriginFromDirectory(folderPath, *m_ioQueue, [this, protectedThis = makeRef(*this), origin, callbackAggregator = callbackAggregator.copyRef(), folderPath] (std::optional<WebCore::ClientOrigin>&& folderOrigin) mutable {
>             if (!folderOrigin)
>                 return;
>             if (folderOrigin->topOrigin != origin && folderOrigin->clientOrigin != origin)
>                 return;
> 
>             ASSERT(folderPath == cachesRootPath(*folderOrigin));
>-            m_ioQueue->dispatch([path = folderPath.isolatedCopy(), taskHandler = WTFMove(taskHandler)] {
>-                deleteDirectoryRecursively(path);
>-            });
>+            deleteDirectoryRecursivelyOnBackgroundThread(folderPath, [callbackAggregator = WTFMove(callbackAggregator)] { });
>         });
>     }
> }
> 
>+void Engine::deleteDirectoryRecursivelyOnBackgroundThread(const String& path, CompletionHandler<void()>&& completionHandler)
>+{
>+    ASSERT(RunLoop::isMain());
>+
>+    m_ioQueue->dispatch([path = path.isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable {
>+        deleteDirectoryRecursively(path);
>+
>+        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
>+            completionHandler();
>+        });
>+    });
>+}
>+
> void Engine::clearMemoryRepresentation(const WebCore::ClientOrigin& origin, WebCore::DOMCacheEngine::CompletionCallback&& callback)
> {
>     readCachesFromDisk(origin, [callback = WTFMove(callback)](CachesOrError&& result) {
>diff --git a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h
>index d5c9419f56c58e70071d29d7e5c416c0d68cdae6..8c2e8cce14d8129097f71ac21e26e361fd6d6833 100644
>--- a/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h
>+++ b/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h
>@@ -75,8 +75,8 @@ public:
>     static void clearMemoryRepresentation(PAL::SessionID, WebCore::ClientOrigin&&, WebCore::DOMCacheEngine::CompletionCallback&&);
>     static void representation(PAL::SessionID, CompletionHandler<void(String&&)>&&);
> 
>-    static void clearAllCaches(PAL::SessionID, Ref<WTF::CallbackAggregator>&&);
>-    static void clearCachesForOrigin(PAL::SessionID, WebCore::SecurityOriginData&&, Ref<WTF::CallbackAggregator>&&);
>+    static void clearAllCaches(PAL::SessionID, CompletionHandler<void()>&&);
>+    static void clearCachesForOrigin(PAL::SessionID, WebCore::SecurityOriginData&&, CompletionHandler<void()>&&);
> 
>     bool shouldPersist() const { return !!m_ioQueue;}
> 
>@@ -95,8 +95,11 @@ private:
>     void remove(uint64_t cacheIdentifier, WebCore::DOMCacheEngine::CacheIdentifierCallback&&);
>     void retrieveCaches(const WebCore::ClientOrigin&, uint64_t updateCounter, WebCore::DOMCacheEngine::CacheInfosCallback&&);
> 
>-    void clearAllCaches(WTF::CallbackAggregator&);
>-    void clearCachesForOrigin(const WebCore::SecurityOriginData&, WTF::CallbackAggregator&);
>+    void clearAllCaches(CompletionHandler<void()>&&);
>+    void clearAllCachesFromDisk(CompletionHandler<void()>&&);
>+    void clearCachesForOrigin(const WebCore::SecurityOriginData&, CompletionHandler<void()>&&);
>+    void clearCachesForOriginFromDisk(const WebCore::SecurityOriginData&, CompletionHandler<void()>&&);
>+    void deleteDirectoryRecursivelyOnBackgroundThread(const String& path, CompletionHandler<void()>&&);
> 
>     void clearMemoryRepresentation(const WebCore::ClientOrigin&, WebCore::DOMCacheEngine::CompletionCallback&&);
>     String representation();

Actions: View | Formatted Diff | Diff

Attachments on bug 188084: 345889