- Revision
- 247119
- Author
- cdu...@apple.com
- Date
- 2019-07-03 17:23:25 -0700 (Wed, 03 Jul 2019)
Log Message
Clarify threading model for WebResourceLoadStatisticsStore::dumpResourceLoadStatistics()
https://bugs.webkit.org/show_bug.cgi?id=199468
Reviewed by Youenn Fablet.
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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (247118 => 247119)
--- trunk/Source/WebKit/ChangeLog 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/ChangeLog 2019-07-04 00:23:25 UTC (rev 247119)
@@ -1,3 +1,32 @@
+2019-07-03 Chris Dumez <cdu...@apple.com>
+
+ Clarify threading model for WebResourceLoadStatisticsStore::dumpResourceLoadStatistics()
+ https://bugs.webkit.org/show_bug.cgi?id=199468
+
+ Reviewed by Youenn Fablet.
+
+ 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 Jonathan Bedard <jbed...@apple.com>
[Catalina] Enable WebKit build
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp 2019-07-04 00:23:25 UTC (rev 247119)
@@ -1099,13 +1099,13 @@
}
}
-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
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h 2019-07-04 00:23:25 UTC (rev 247119)
@@ -73,7 +73,7 @@
bool isRegisteredAsRedirectingTo(const RedirectedFromDomain&, const RedirectedToDomain&) const override;
void clearPrevalentResource(const RegistrableDomain&) override;
- String dumpResourceLoadStatistics() const override;
+ void dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&&) final;
bool isPrevalentResource(const RegistrableDomain&) const override;
bool isVeryPrevalentResource(const RegistrableDomain&) const override;
void setPrevalentResource(const RegistrableDomain&) override;
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp 2019-07-04 00:23:25 UTC (rev 247119)
@@ -462,15 +462,21 @@
}
}
-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
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h 2019-07-04 00:23:25 UTC (rev 247119)
@@ -79,7 +79,7 @@
bool isRegisteredAsRedirectingTo(const RedirectedFromDomain&, const RedirectedToDomain&) const override;
void clearPrevalentResource(const RegistrableDomain&) override;
- String dumpResourceLoadStatistics() const override;
+ void dumpResourceLoadStatistics(CompletionHandler<void(const String&)>&&) final;
bool isPrevalentResource(const RegistrableDomain&) const override;
bool isVeryPrevalentResource(const RegistrableDomain&) const override;
void setPrevalentResource(const RegistrableDomain&) override;
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp 2019-07-04 00:23:25 UTC (rev 247119)
@@ -214,8 +214,13 @@
}
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.");
});
});
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h 2019-07-04 00:23:25 UTC (rev 247119)
@@ -119,7 +119,7 @@
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,8 +183,6 @@
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);
@@ -191,6 +189,8 @@
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 @@
bool debugModeEnabled() const { return m_debugModeEnabled; }
static constexpr unsigned maxNumberOfRecursiveCallsInRedirectTraceBack { 50 };
+
+ Vector<CompletionHandler<void()>> m_dataRecordRemovalCompletionHandlers;
private:
bool shouldRemoveDataRecords() const;
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2019-07-04 00:23:25 UTC (rev 247119)
@@ -625,27 +625,19 @@
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();
+ }
+ m_statisticsStore->dumpResourceLoadStatistics(WTFMove(innerCompletionHandler));
});
}
-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);
- });
-}
-
void WebResourceLoadStatisticsStore::isPrevalentResource(const RegistrableDomain& domain, CompletionHandler<void(bool)>&& completionHandler)
{
ASSERT(RunLoop::isMain());
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h (247118 => 247119)
--- trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2019-07-04 00:08:00 UTC (rev 247118)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2019-07-04 00:23:25 UTC (rev 247119)
@@ -126,7 +126,6 @@
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 @@
bool m_hasScheduledProcessStats { false };
bool m_firstNetworkProcessCreated { false };
-
- CompletionHandler<void(String)> m_dumpResourceLoadStatisticsCompletionHandler;
};
} // namespace WebKit
