Attachment 343301 Details for Bug 186905 – Patch

[patch] Patch

bug-186905-20180621192627.patch (text/plain), 17.07 KB, created by Chris Dumez on 2018-06-21 19:26:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-21 19:26:28 PDT

Size: 17.07 KB

patch

obsolete

>Subversion Revision: 233068
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 02a8eef5f3c7331e09e0e2fc07ff98b021be720c..04ce755f22496a8d59c2aff320b984f6f8282f62 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,47 @@
>+2018-06-21  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under WebResourceLoadStatisticsStore::mergeStatistics(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)
>+        https://bugs.webkit.org/show_bug.cgi?id=186905
>+        <rdar://problem/41266775>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        I believe the crash was caused by the WebResourceLoadStatisticsStore object being dead
>+        when mergeStatistics() is called. In particular, the crash was happening when the
>+        ResourceLoadStatisticsPersistentStorage's FileMonitor would detect a file change and
>+        we would re-sync statistics from the disk. The FileMonitor's lambda function was
>+        capturing |this| without ref'ing it, and the FileMonitor monitors the disk and
>+        calls the lambda on the background queue, while it gets destroyed on the main thread.
>+
>+        To make lifetime management less complex, the following changes were made:
>+        - The ResourceLoadStatisticsPersistentStorage object is now always constructed / used
>+          and destroyed on the background queue. We no longer have to worry about being on
>+          the right thread in a given method.
>+        - Now that ResourceLoadStatisticsPersistentStorage is always used from the background
>+          queue and no longer needs to be thread-safe, drop its ref() / deref() methods and
>+          use weak pointers instead to make sure the ResourceLoadStatisticsPersistentStorage
>+          is still alive when a lamdba gets called on the background queue.
>+        - For write scheduling use WorkQueue::dispatchAfter() and a WeakPtr instead of a
>+          RunLoop::Timer. This is more convenient to use as the RunLoop::Timer has to be used
>+          on the main thread.
>+
>+        * UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:
>+        (WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
>+        (WebKit::ResourceLoadStatisticsPersistentStorage::~ResourceLoadStatisticsPersistentStorage):
>+        (WebKit::ResourceLoadStatisticsPersistentStorage::startMonitoringDisk):
>+        (WebKit::ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics):
>+        (WebKit::ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore):
>+        * UIProcess/ResourceLoadStatisticsPersistentStorage.h:
>+        * UIProcess/WebResourceLoadStatisticsStore.cpp:
>+        (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
>+        (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
>+        (WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
>+        (WebKit::WebResourceLoadStatisticsStore::processStatisticsAndDataRecords):
>+        (WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData):
>+        (WebKit::WebResourceLoadStatisticsStore::applicationWillTerminate):
>+        (WebKit::WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent):
>+        * UIProcess/WebResourceLoadStatisticsStore.h:
>+
> 2018-06-21  Chris Dumez  <cdumez@apple.com>
> 
>         Unreviewed, rolling out r232995.
>diff --git a/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp b/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp
>index b67cade25dfcc9bacf579c66b5507fef0accbb7c..5fc1de35523e0c33e29fe8422638fbd61d051f30 100644
>--- a/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp
>+++ b/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp
>@@ -34,7 +34,6 @@
> #include <WebCore/SharedBuffer.h>
> #include <wtf/RunLoop.h>
> #include <wtf/WorkQueue.h>
>-#include <wtf/threads/BinarySemaphore.h>
> 
> namespace WebKit {
> 
>@@ -85,21 +84,20 @@ static std::unique_ptr<KeyedDecoder> createDecoderForFile(const String& path)
> ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage(WebResourceLoadStatisticsStore& store, const String& storageDirectoryPath, IsReadOnly isReadOnly)
>     : m_memoryStore(store)
>     , m_storageDirectoryPath(storageDirectoryPath)
>-    , m_asyncWriteTimer(RunLoop::main(), this, &ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired)
>     , m_isReadOnly(isReadOnly)
>-{
>-}
>-
>-void ResourceLoadStatisticsPersistentStorage::initialize()
> {
>     ASSERT(!RunLoop::isMain());
>+
>     populateMemoryStoreFromDisk();
>     startMonitoringDisk();
> }
> 
> ResourceLoadStatisticsPersistentStorage::~ResourceLoadStatisticsPersistentStorage()
> {
>-    ASSERT(!m_hasPendingWrite);
>+    ASSERT(!RunLoop::isMain());
>+
>+    if (m_hasPendingWrite)
>+        writeMemoryStoreToDisk();
> }
> 
> String ResourceLoadStatisticsPersistentStorage::storageDirectoryPath() const
>@@ -126,8 +124,11 @@ void ResourceLoadStatisticsPersistentStorage::startMonitoringDisk()
>     if (resourceLogPath.isEmpty())
>         return;
> 
>-    m_fileMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_memoryStore.statisticsQueue(), [this] (FileMonitor::FileChangeType type) {
>+    m_fileMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_memoryStore.statisticsQueue(), [this, weakThis = makeWeakPtr(*this)] (FileMonitor::FileChangeType type) {
>         ASSERT(!RunLoop::isMain());
>+        if (!weakThis)
>+            return;
>+
>         switch (type) {
>         case FileMonitor::FileChangeType::Modification:
>             refreshMemoryStoreFromDisk();
>@@ -143,6 +144,8 @@ void ResourceLoadStatisticsPersistentStorage::startMonitoringDisk()
> 
> void ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics()
> {
>+    ASSERT(!RunLoop::isMain());
>+
>     String storagePath = storageDirectoryPath();
>     ASSERT(!storagePath.isEmpty());
> 
>@@ -237,15 +240,6 @@ void ResourceLoadStatisticsPersistentStorage::populateMemoryStoreFromDisk()
>     m_memoryStore.logTestingEvent(ASCIILiteral("PopulatedWithoutGrandfathering"));
> }
> 
>-void ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired()
>-{
>-    ASSERT(RunLoop::isMain());
>-    RELEASE_ASSERT(m_isReadOnly != IsReadOnly::Yes);
>-    m_memoryStore.statisticsQueue().dispatch([this] () mutable {
>-        writeMemoryStoreToDisk();
>-    });
>-}
>-
> void ResourceLoadStatisticsPersistentStorage::writeMemoryStoreToDisk()
> {
>     ASSERT(!RunLoop::isMain());
>@@ -292,8 +286,9 @@ void ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore(ForceIm
>         if (!m_hasPendingWrite) {
>             m_hasPendingWrite = true;
>             Seconds delay = minimumWriteInterval - timeSinceLastWrite + 1_s;
>-            RunLoop::main().dispatch([this, protectedThis = makeRef(*this), delay] {
>-                m_asyncWriteTimer.startOneShot(delay);
>+            m_memoryStore.statisticsQueue().dispatchAfter(delay, [weakThis = makeWeakPtr(*this)] {
>+                if (weakThis)
>+                    weakThis->writeMemoryStoreToDisk();
>             });
>         }
>         return;
>@@ -315,36 +310,6 @@ void ResourceLoadStatisticsPersistentStorage::clear()
>         RELEASE_LOG_ERROR(ResourceLoadStatistics, "ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file: %s", filePath.utf8().data());
> }
> 
>-void ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously()
>-{
>-    if (m_isReadOnly == IsReadOnly::Yes) {
>-        RELEASE_ASSERT(!m_asyncWriteTimer.isActive());
>-        return;
>-    }
>-
>-    m_asyncWriteTimer.stop();
>-
>-    BinarySemaphore semaphore;
>-    // Make sure any pending work in our queue is finished before we terminate.
>-    m_memoryStore.statisticsQueue().dispatch([&semaphore, this] {
>-        // Write final file state to disk.
>-        if (m_hasPendingWrite)
>-            writeMemoryStoreToDisk();
>-        semaphore.signal();
>-    });
>-    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/ResourceLoadStatisticsPersistentStorage.h b/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h
>index b8d12f1717ddc29096188f96ed7803d7c3156daf..b58de8771b7d5f496b42504420c627665c7602e1 100644
>--- a/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h
>+++ b/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h
>@@ -29,6 +29,7 @@
> #include <wtf/MonotonicTime.h>
> #include <wtf/RunLoop.h>
> #include <wtf/WallTime.h>
>+#include <wtf/WeakPtr.h>
> #include <wtf/text/WTFString.h>
> 
> namespace WebCore {
>@@ -39,24 +40,16 @@ namespace WebKit {
> 
> class WebResourceLoadStatisticsStore;
> 
>-class ResourceLoadStatisticsPersistentStorage {
>+// Can only be constructed / destroyed / used from the WebResourceLoadStatisticsStore's statistic queue.
>+class ResourceLoadStatisticsPersistentStorage : public CanMakeWeakPtr<ResourceLoadStatisticsPersistentStorage> {
> public:
>     enum class IsReadOnly { No, Yes };
>     ResourceLoadStatisticsPersistentStorage(WebResourceLoadStatisticsStore&, const String& storageDirectoryPath, IsReadOnly);
>     ~ResourceLoadStatisticsPersistentStorage();
> 
>-    void initialize();
>     void clear();
> 
>-    void finishAllPendingWorkSynchronously();
>-
>-    void ref();
>-    void deref();
>-
>-    enum class ForceImmediateWrite {
>-        No,
>-        Yes,
>-    };
>+    enum class ForceImmediateWrite { No, Yes, };
>     void scheduleOrWriteMemoryStore(ForceImmediateWrite);
> 
> private:
>@@ -71,11 +64,9 @@ 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;
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>index 6dab2637b9a8857c8db23714453167d8ef6dbc09..73e76ac3efeee145c271ce1d6130832c58756ffc 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>@@ -46,6 +46,7 @@
> #if !RELEASE_LOG_DISABLED
> #include <wtf/text/StringBuilder.h>
> #endif
>+#include <wtf/threads/BinarySemaphore.h>
> 
> namespace WebKit {
> using namespace WebCore;
>@@ -165,7 +166,6 @@ static Vector<OperatingDate> mergeOperatingDates(const Vector<OperatingDate>& ex
> 
> WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& resourceLoadStatisticsDirectory, Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&& updatePrevalentDomainsToPartitionOrBlockCookiesHandler, HasStorageAccessForFrameHandler&& hasStorageAccessForFrameHandler, GrantStorageAccessHandler&& grantStorageAccessHandler, RemoveAllStorageAccessHandler&& removeAllStorageAccessHandler, RemovePrevalentDomainsHandler&& removeDomainsHandler)
>     : m_statisticsQueue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
>-    , m_persistentStorage(*this, resourceLoadStatisticsDirectory, isEphemeral ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes : ResourceLoadStatisticsPersistentStorage::IsReadOnly::No)
>     , m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(WTFMove(updatePrevalentDomainsToPartitionOrBlockCookiesHandler))
>     , m_hasStorageAccessForFrameHandler(WTFMove(hasStorageAccessForFrameHandler))
>     , m_grantStorageAccessHandler(WTFMove(grantStorageAccessHandler))
>@@ -180,8 +180,8 @@ WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& res
>     registerUserDefaultsIfNeeded();
> #endif
> 
>-    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this)] {
>-        m_persistentStorage.initialize();
>+    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), isEphemeral, resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory.isolatedCopy()] {
>+        m_persistentStorage = std::make_unique<ResourceLoadStatisticsPersistentStorage>(*this, resourceLoadStatisticsDirectory, isEphemeral ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes : ResourceLoadStatisticsPersistentStorage::IsReadOnly::No);
>         includeTodayAsOperatingDateIfNecessary();
>     });
> 
>@@ -195,7 +195,22 @@ WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& res
> 
> WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> {
>-    m_persistentStorage.finishAllPendingWorkSynchronously();
>+    flushAndDestroyPersistentStore();
>+}
>+
>+void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
>+{
>+    if (!m_persistentStorage)
>+        return;
>+
>+    // Make sure we destroy the persistent store on the background queue and wait for it to die
>+    // synchronously since it has a C++ reference to us.
>+    BinarySemaphore semaphore;
>+    m_statisticsQueue->dispatch([&semaphore, this] {
>+        m_persistentStorage = nullptr;
>+        semaphore.signal();
>+    });
>+    semaphore.wait(WallTime::infinity());
> }
> 
> #if !RELEASE_LOG_DISABLED
>@@ -341,7 +356,8 @@ void WebResourceLoadStatisticsStore::processStatisticsAndDataRecords()
>             ASSERT(!RunLoop::isMain());
> 
>             pruneStatisticsIfNeeded();
>-            m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
>+            if (m_persistentStorage)
>+                m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
> 
>             RunLoop::main().dispatch([] {
>                 WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed();
>@@ -352,7 +368,8 @@ void WebResourceLoadStatisticsStore::processStatisticsAndDataRecords()
>             ASSERT(!RunLoop::isMain());
> 
>             pruneStatisticsIfNeeded();
>-            m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
>+            if (m_persistentStorage)
>+                m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
>         });
>     }
> }
>@@ -534,7 +551,8 @@ void WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData(CompletionHa
>                     statistic.grandfathered = true;
>                 }
>                 m_endOfGrandfatheringTimestamp = WallTime::now() + m_parameters.grandfatheringTime;
>-                m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
>+                if (m_persistentStorage)
>+                    m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
>                 callback();
>                 logTestingEvent(ASCIILiteral("Grandfathered"));
>             });
>@@ -554,7 +572,7 @@ void WebResourceLoadStatisticsStore::processDidCloseConnection(WebProcessProxy&,
> 
> void WebResourceLoadStatisticsStore::applicationWillTerminate()
> {
>-    m_persistentStorage.finishAllPendingWorkSynchronously();
>+    flushAndDestroyPersistentStore();
> }
> 
> void WebResourceLoadStatisticsStore::performDailyTasks()
>@@ -1001,7 +1019,8 @@ void WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent(ShouldGr
>     ASSERT(RunLoop::isMain());
>     m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), shouldGrandfather, callback = WTFMove(callback)] () mutable {
>         clearInMemory();
>-        m_persistentStorage.clear();
>+        if (m_persistentStorage)
>+            m_persistentStorage->clear();
>         
>         if (shouldGrandfather == ShouldGrandfather::Yes)
>             grandfatherExistingWebsiteData([protectedThis = WTFMove(protectedThis), callback = WTFMove(callback)]() {
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>index 4f116244ba1e4c0dcf404648aecebb3931f54450..f70f46fe813d70c247df2b23c2b21fe653cc6a76 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>@@ -200,6 +200,8 @@ private:
>     void setDebugLogggingEnabled(bool enabled) { m_debugLoggingEnabled  = enabled; }
>     void setStorageAccessPromptsEnabled(bool enabled) { m_storageAccessPromptsEnabled  = enabled; }
> 
>+    void flushAndDestroyPersistentStore();
>+
> #if PLATFORM(COCOA)
>     void registerUserDefaultsIfNeeded();
> #endif
>@@ -225,7 +227,7 @@ private:
>     ResourceLoadStatisticsClassifier m_resourceLoadStatisticsClassifier;
> #endif
>     Ref<WTF::WorkQueue> m_statisticsQueue;
>-    ResourceLoadStatisticsPersistentStorage m_persistentStorage;
>+    std::unique_ptr<ResourceLoadStatisticsPersistentStorage> m_persistentStorage;
>     Vector<OperatingDate> m_operatingDates;
> 
>     UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler;

Actions: View | Formatted Diff | Diff

Attachments on bug 186905: 343301