Attachment 373422 Details for Bug 199468 – Patch

[patch] Patch

bug-199468-20190703152945.patch (text/plain), 13.11 KB, created by Chris Dumez on 2019-07-03 15:29:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-03 15:29:45 PDT

Size: 13.11 KB

patch

obsolete

>Subversion Revision: 247103
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 7a391906332cdb13c01dccb2bcc8ec8fcbbcc242..9562d1a748ea95770ceda37ef1fb029ccaa147dc 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,32 @@
>+2019-07-03  Chris Dumez  <cdumez@apple.com>
>+
>+        Clarify threading model for WebResourceLoadStatisticsStore::dumpResourceLoadStatistics()
>+        https://bugs.webkit.org/show_bug.cgi?id=199468
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Our convention is that the WebResourceLoadStatisticsStore is always created, used and
>+        destroyed on the main thread, while the ResourceLoadStatisticsStore is always created,
>+        used and destroyed on the background queue.
>+
>+        r245517 broke this convention by introducing a tryDumpResourceLoadStatistics() method
>+        to WebResourceLoadStatisticsStore which gets called on the background queue. This patch
>+        fixes this since this has been a huge source of thread-safety bugs in the past.
>+
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:
>+        (WebKit::ResourceLoadStatisticsDatabaseStore::dumpResourceLoadStatistics):
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:
>+        (WebKit::ResourceLoadStatisticsMemoryStore::dumpResourceLoadStatistics):
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:
>+        (WebKit::ResourceLoadStatisticsStore::removeDataRecords):
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:
>+        (WebKit::ResourceLoadStatisticsStore::dataRecordsBeingRemoved const):
>+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
>+        (WebKit::WebResourceLoadStatisticsStore::dumpResourceLoadStatistics):
>+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
>+
> 2019-07-03  Chris Dumez  <cdumez@apple.com>
> 
>         Fix a couple of thread safety issues in ResourceLoadStatisticsStore
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
>index 3c2ecd5bc8ce5869f40b6c436bb5dd13e674765f..04923ee433fc12f56c128f70ac782364e356bea2 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
>@@ -1099,13 +1099,13 @@ void ResourceLoadStatisticsDatabaseStore::setDomainsAsPrevalent(StdSet<unsigned>
>     }
> }
> 
>-String ResourceLoadStatisticsDatabaseStore::dumpResourceLoadStatistics() const
>+void ResourceLoadStatisticsDatabaseStore::dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&& completionHandler)
> {
>     ASSERT(!RunLoop::isMain());
> 
>     // FIXME(195088): Implement SQLite-based dumping routines.
>     ASSERT_NOT_REACHED();
>-    return { };
>+    completionHandler({ });
> }
> 
> bool ResourceLoadStatisticsDatabaseStore::predicateValueForDomain(WebCore::SQLiteStatement& predicateStatement, const RegistrableDomain& domain) const
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h
>index bfc2339685332002626789431ae073b2d3975f43..1ba9343b6460c6d04e8065e3d1b1183da11071ba 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h
>@@ -73,7 +73,7 @@ public:
>     bool isRegisteredAsRedirectingTo(const RedirectedFromDomain&, const RedirectedToDomain&) const override;
> 
>     void clearPrevalentResource(const RegistrableDomain&) override;
>-    String dumpResourceLoadStatistics() const override;
>+    void dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&&) override;
>     bool isPrevalentResource(const RegistrableDomain&) const override;
>     bool isVeryPrevalentResource(const RegistrableDomain&) const override;
>     void setPrevalentResource(const RegistrableDomain&) override;
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp
>index 35241748de43613ef03023b3e6576358b6efffc4..7305c510236a4c8a3a36f60505f961182e4ee736 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp
>@@ -462,15 +462,21 @@ void ResourceLoadStatisticsMemoryStore::setPrevalentResource(ResourceLoadStatist
>     }
> }
>     
>-String ResourceLoadStatisticsMemoryStore::dumpResourceLoadStatistics() const
>+void ResourceLoadStatisticsMemoryStore::dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&& completionHandler)
> {
>     ASSERT(!RunLoop::isMain());
>+    if (dataRecordsBeingRemoved()) {
>+        m_dataRecordRemovalCompletionHandlers.append([this, completionHandler = WTFMove(completionHandler)]() mutable {
>+            dumpResourceLoadStatistics(WTFMove(completionHandler));
>+        });
>+        return;
>+    }
> 
>     StringBuilder result;
>     result.appendLiteral("Resource load statistics:\n\n");
>     for (auto& mapEntry : m_resourceStatisticsMap.values())
>         result.append(mapEntry.toString());
>-    return result.toString();
>+    completionHandler(result.toString());
> }
> 
> bool ResourceLoadStatisticsMemoryStore::isPrevalentResource(const RegistrableDomain& domain) const
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h
>index 45f7e1b759693c1103a3d6627316d6fce02e4ce9..5ffed2545430c6fb3e8d87c759b0626e8d1080db 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h
>@@ -79,7 +79,7 @@ public:
>     bool isRegisteredAsRedirectingTo(const RedirectedFromDomain&, const RedirectedToDomain&) const override;
> 
>     void clearPrevalentResource(const RegistrableDomain&) override;
>-    String dumpResourceLoadStatistics() const override;
>+    void dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&&) override;
>     bool isPrevalentResource(const RegistrableDomain&) const override;
>     bool isVeryPrevalentResource(const RegistrableDomain&) const override;
>     void setPrevalentResource(const RegistrableDomain&) override;
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>index 753c294373bc4b7b8c69883c8c9c1b085de91424..00b4fa4dfd8c7bc8a0c41e6dc8ca7e3973cfc0fd 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>@@ -214,8 +214,13 @@ void ResourceLoadStatisticsStore::removeDataRecords(CompletionHandler<void()>&&
>                 }
>                 weakThis->incrementRecordsDeletedCountForDomains(WTFMove(domainsWithDeletedWebsiteData));
>                 weakThis->setDataRecordsBeingRemoved(false);
>-                weakThis->m_store.tryDumpResourceLoadStatistics();
>+                
>+                auto dataRecordRemovalCompletionHandlers = WTFMove(weakThis->m_dataRecordRemovalCompletionHandlers);
>                 completionHandler();
>+                
>+                for (auto& dataRecordRemovalCompletionHandler : dataRecordRemovalCompletionHandlers)
>+                    dataRecordRemovalCompletionHandler();
>+
>                 RELEASE_LOG_INFO_IF(weakThis->m_debugLoggingEnabled, ITPDebug, "Done removing data records.");
>             });
>         });
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h
>index d7798b25d1e3bbbb78a87ab3e1a5a3139f14ba82..adec14cf9ccd964c2901ce2d84ea356df6db0ff2 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h
>@@ -119,7 +119,7 @@ public:
>     virtual bool isRegisteredAsRedirectingTo(const RedirectedFromDomain&, const RedirectedToDomain&) const = 0;
> 
>     virtual void clearPrevalentResource(const RegistrableDomain&) = 0;
>-    virtual String dumpResourceLoadStatistics() const = 0;
>+    virtual void dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&&) = 0;
>     virtual bool isPrevalentResource(const RegistrableDomain&) const = 0;
>     virtual bool isVeryPrevalentResource(const RegistrableDomain&) const = 0;
>     virtual void setPrevalentResource(const RegistrableDomain&) = 0;
>@@ -183,14 +183,14 @@ public:
>     virtual bool isMemoryStore() const { return false; }
>     virtual bool isDatabaseStore()const { return false; }
> 
>-    bool dataRecordsBeingRemoved() const { return m_dataRecordsBeingRemoved; }
>-
> protected:
>     static unsigned computeImportance(const WebCore::ResourceLoadStatistics&);
>     static Vector<OperatingDate> mergeOperatingDates(const Vector<OperatingDate>& existingDates, Vector<OperatingDate>&& newDates);
>     static void debugLogDomainsInBatches(const char* action, const Vector<RegistrableDomain>& domains);
> 
>     ResourceLoadStatisticsStore(WebResourceLoadStatisticsStore&, WorkQueue&, ShouldIncludeLocalhost);
>+    
>+    bool dataRecordsBeingRemoved() const { return m_dataRecordsBeingRemoved; }
> 
>     bool hasStatisticsExpired(const ResourceLoadStatistics&, OperatingDatesWindow) const;
>     bool hasStatisticsExpired(WallTime mostRecentUserInteractionTime, OperatingDatesWindow) const;
>@@ -234,6 +234,8 @@ protected:
>     bool debugModeEnabled() const { return m_debugModeEnabled; }
> 
>     static constexpr unsigned maxNumberOfRecursiveCallsInRedirectTraceBack { 50 };
>+    
>+    Vector<CompletionHandler<void()>> m_dataRecordRemovalCompletionHandlers;
> 
> private:
>     bool shouldRemoveDataRecords() const;
>diff --git a/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp b/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp
>index b72d8ebab0c4d0b43e73bb42053f24403ea90642..26c25b9ecc89f7fa053ab1c0fe006350a1196b07 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp
>@@ -625,24 +625,16 @@ void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandle
>     ASSERT(RunLoop::isMain());
> 
>     postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
>-        ASSERT(!m_dumpResourceLoadStatisticsCompletionHandler);
>-        m_dumpResourceLoadStatisticsCompletionHandler = WTFMove(completionHandler);
>-        if (m_statisticsStore && m_statisticsStore->dataRecordsBeingRemoved())
>+        auto innerCompletionHandler = [completionHandler = WTFMove(completionHandler)](const String& result) mutable {
>+            postTaskReply([result = result.isolatedCopy(), completionHandler = WTFMove(completionHandler)]() mutable {
>+                completionHandler(result);
>+            });
>+        };
>+        if (!m_statisticsStore) {
>+            innerCompletionHandler(emptyString());
>             return;
>-        tryDumpResourceLoadStatistics();
>-    });
>-}
>-
>-void WebResourceLoadStatisticsStore::tryDumpResourceLoadStatistics()
>-{
>-    ASSERT(!RunLoop::isMain());
>-
>-    if (!m_dumpResourceLoadStatisticsCompletionHandler)
>-        return;
>-
>-    String result = m_statisticsStore ? m_statisticsStore->dumpResourceLoadStatistics() : emptyString();
>-    postTaskReply([result = result.isolatedCopy(), completionHandler = WTFMove(m_dumpResourceLoadStatisticsCompletionHandler)]() mutable {
>-        completionHandler(result);
>+        }
>+        m_statisticsStore->dumpResourceLoadStatistics(WTFMove(innerCompletionHandler));
>     });
> }
> 
>diff --git a/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h b/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h
>index f19a73d6fbc2ea258bc998ead5d2275c2fca09a1..1fc48a2fb983503828b8f74a5b199b6ee27d4168 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h
>+++ b/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h
>@@ -126,7 +126,6 @@ public:
>     void setPrevalentResource(const RegistrableDomain&, CompletionHandler<void()>&&);
>     void setVeryPrevalentResource(const RegistrableDomain&, CompletionHandler<void()>&&);
>     void dumpResourceLoadStatistics(CompletionHandler<void(String)>&&);
>-    void tryDumpResourceLoadStatistics();
>     void isPrevalentResource(const RegistrableDomain&, CompletionHandler<void(bool)>&&);
>     void isVeryPrevalentResource(const RegistrableDomain&, CompletionHandler<void(bool)>&&);
>     void isRegisteredAsSubresourceUnder(const SubResourceDomain&, const TopFrameDomain&, CompletionHandler<void(bool)>&&);
>@@ -206,8 +205,6 @@ private:
>     bool m_hasScheduledProcessStats { false };
> 
>     bool m_firstNetworkProcessCreated { false };
>-    
>-    CompletionHandler<void(String)> m_dumpResourceLoadStatisticsCompletionHandler;
> };
> 
> } // namespace WebKit

Actions: View | Formatted Diff | Diff

Attachments on bug 199468: 373422 | 373434