Fix lifetime management issue in ResourceLoadStatisticsPersistentStorage::scheduleOrW...

https://bugs.webkit.org/show_bug.cgi?id=174790

Reviewed by Brady Eidson.

Fix lifetime management issue in ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore().
WorkQueue::dispatchAfter() keeps the WorkQueue alive because its implementation keeps a strong ref to
|this|. As a result, the lambda passed to dispatchAfter(), which calls writeMemoryStoreToDisk(), can
get executed after the store is gone.

To address the issue, we now use a RunLoop::Timer to schedule the write, instead of a
WorkQueue::dispatchAfter() call. This way, we are guaranteed that the callback will not get called
after the store has been destroyed.

* UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:
(WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
(WebKit::ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired):
(WebKit::ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore):
(WebKit::ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously):
(WebKit::ResourceLoadStatisticsPersistentStorage::ref):
(WebKit::ResourceLoadStatisticsPersistentStorage::deref):
* UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:

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

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 8978090eb3a66e9b1aebad3aa0d1e1e3b92167e1..a228f5452be2e179d2e0430db5348c9068282b98 100644 (file)
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,28 @@
+2017-07-24  Chris Dumez  <cdumez@apple.com>
+
+        Fix lifetime management issue in ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore()
+        https://bugs.webkit.org/show_bug.cgi?id=174790
+
+        Reviewed by Brady Eidson.
+
+        Fix lifetime management issue in ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore().
+        WorkQueue::dispatchAfter() keeps the WorkQueue alive because its implementation keeps a strong ref to
+        |this|. As a result, the lambda passed to dispatchAfter(), which calls writeMemoryStoreToDisk(), can
+        get executed after the store is gone.
+
+        To address the issue, we now use a RunLoop::Timer to schedule the write, instead of a
+        WorkQueue::dispatchAfter() call. This way, we are guaranteed that the callback will not get called
+        after the store has been destroyed.
+
+        * UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:
+        (WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
+        (WebKit::ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired):
+        (WebKit::ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore):
+        (WebKit::ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously):
+        (WebKit::ResourceLoadStatisticsPersistentStorage::ref):
+        (WebKit::ResourceLoadStatisticsPersistentStorage::deref):
+        * UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:
+
 2017-07-24  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, rolling out r219828.

diff --git a/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp b/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp
index 58ea1aae2f99de5638bfd3aabc4b3a48ef503f86..b4d92beb098d1f49731ae5974ff2aca514866d11 100644 (file)
--- a/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp
+++ b/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp
@@ -85,6 +85,7 @@ static std::unique_ptr<KeyedDecoder> createDecoderForFile(const String& path)
 ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage(WebResourceLoadStatisticsStore& store, const String& storageDirectoryPath)
     : m_memoryStore(store)
     , m_storageDirectoryPath(storageDirectoryPath)
+    , m_asyncWriteTimer(RunLoop::main(), this, &ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired)
 {
 }
 
@@ -237,6 +238,14 @@ void ResourceLoadStatisticsPersistentStorage::populateMemoryStoreFromDisk()
         m_memoryStore.grandfatherExistingWebsiteData();
 }
 
+void ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired()
+{
+    ASSERT(RunLoop::isMain());
+    m_memoryStore.statisticsQueue().dispatch([this] () mutable {
+        writeMemoryStoreToDisk();
+    });
+}
+
 void ResourceLoadStatisticsPersistentStorage::writeMemoryStoreToDisk()
 {
     ASSERT(!RunLoop::isMain());
@@ -280,8 +289,8 @@ void ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore()
         if (!m_hasPendingWrite) {
             m_hasPendingWrite = true;
             Seconds delay = minimumWriteInterval - timeSinceLastWrite + 1_s;
-            m_memoryStore.statisticsQueue().dispatchAfter(delay, [this] () mutable {
-                writeMemoryStoreToDisk();
+            RunLoop::main().dispatch([this, protectedThis = makeRef(*this), delay] {
+                m_asyncWriteTimer.startOneShot(delay);
             });
         }
         return;
@@ -305,6 +314,8 @@ void ResourceLoadStatisticsPersistentStorage::clear()
 
 void ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously()
 {
+    m_asyncWriteTimer.stop();
+
     BinarySemaphore semaphore;
     // Make sure any pending work in our queue is finished before we terminate.
     m_memoryStore.statisticsQueue().dispatch([&semaphore, this] {
@@ -316,6 +327,16 @@ void ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously(
     semaphore.wait(WallTime::infinity());
 }
 
+void ResourceLoadStatisticsPersistentStorage::ref()
+{
+    m_memoryStore.ref();
+}
+
+void ResourceLoadStatisticsPersistentStorage::deref()
+{
+    m_memoryStore.deref();
+}
+
 #if !PLATFORM(IOS)
 void ResourceLoadStatisticsPersistentStorage::excludeFromBackup() const
 {

diff --git a/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h b/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h
index e3544f535be0fc6322163180ed645f079d570f73..81f7ccde34c12ba606691a86dcb4d45528f435a8 100644 (file)
--- a/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h
+++ b/Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h
@@ -27,6 +27,7 @@
 
 #include <wtf/Forward.h>
 #include <wtf/MonotonicTime.h>
+#include <wtf/RunLoop.h>
 #include <wtf/WallTime.h>
 #include <wtf/text/WTFString.h>
 
@@ -49,6 +50,9 @@ public:
 
     void finishAllPendingWorkSynchronously();
 
+    void ref();
+    void deref();
+
 private:
     String storageDirectoryPath() const;
     String resourceLogFilePath() const;
@@ -61,9 +65,11 @@ private:
     void populateMemoryStoreFromDisk();
     void excludeFromBackup() const;
     void refreshMemoryStoreFromDisk();
+    void asyncWriteTimerFired();
 
     WebResourceLoadStatisticsStore& m_memoryStore;
     const String m_storageDirectoryPath;
+    RunLoop::Timer<ResourceLoadStatisticsPersistentStorage> m_asyncWriteTimer;
     std::unique_ptr<WebCore::FileMonitor> m_fileMonitor;
     WallTime m_lastStatisticsFileSyncTime;
     MonotonicTime m_lastStatisticsWriteTime;