- Revision
- 247975
- Author
- alanc...@apple.com
- Date
- 2019-07-29 20:55:24 -0700 (Mon, 29 Jul 2019)
Log Message
Cherry-pick r247784. rdar://problem/53647468
Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
https://bugs.webkit.org/show_bug.cgi?id=200071
<rdar://problem/53335583>
Reviewed by Brent Fulgham and Youenn Fablet.
The WebResourceLoadStatisticsStore is a main thread object. In its destructor, it was dispatching
to the background queue to destroy the m_statisticsStore / m_persistentStorage data members, which
live on the background queue. It would then synchronously wait for the background queue to finish
destroying them. The idea was to guarantee that the ResourceLoadStatisticsMemoryStore and the
ResourceLoadStatisticsPersistentStorage would never outlive the WebResourceLoadStatisticsStore,
given that they keep a raw pointer back to the WebResourceLoadStatisticsStore (via m_store data
member).
The issue is that *while* the WebResourceLoadStatisticsStore destructor is running on the main
thread, the background queue may be running code in ResourceLoadStatisticsMemoryStore or
ResourceLoadStatisticsPersistentStorage which refs the WebResourceLoadStatisticsStore, even
though its ref count has already reached 0. It is actually a common pattern in
ResourceLoadStatisticsMemoryStore to call RunLoop::main().dispatch() and ref their m_store in
the lambda, so that they can interact with the WebResourceLoadStatisticsStore.
To address the issue, we now destroy m_statisticsStore / m_persistentStorage *before* the
WebResourceLoadStatisticsStore destructor runs. The NetworkSession destructor now calls
WebResourceLoadStatisticsStore::didDestroyNetworkSession() which takes care of destroying
m_statisticsStore / m_persistentStorage on the background queue, synchronously. The
WebResourceLoadStatisticsStore destructor will only run later, once all remaining references
to it are gone.
* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
* NetworkProcess/NetworkSession.cpp:
(WebKit::NetworkSession::~NetworkSession):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247784 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-608-branch/Source/WebKit/ChangeLog (247974 => 247975)
--- branches/safari-608-branch/Source/WebKit/ChangeLog 2019-07-30 03:55:21 UTC (rev 247974)
+++ branches/safari-608-branch/Source/WebKit/ChangeLog 2019-07-30 03:55:24 UTC (rev 247975)
@@ -1,5 +1,84 @@
2019-07-29 Alan Coon <alanc...@apple.com>
+ Cherry-pick r247784. rdar://problem/53647468
+
+ Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
+ https://bugs.webkit.org/show_bug.cgi?id=200071
+ <rdar://problem/53335583>
+
+ Reviewed by Brent Fulgham and Youenn Fablet.
+
+ The WebResourceLoadStatisticsStore is a main thread object. In its destructor, it was dispatching
+ to the background queue to destroy the m_statisticsStore / m_persistentStorage data members, which
+ live on the background queue. It would then synchronously wait for the background queue to finish
+ destroying them. The idea was to guarantee that the ResourceLoadStatisticsMemoryStore and the
+ ResourceLoadStatisticsPersistentStorage would never outlive the WebResourceLoadStatisticsStore,
+ given that they keep a raw pointer back to the WebResourceLoadStatisticsStore (via m_store data
+ member).
+
+ The issue is that *while* the WebResourceLoadStatisticsStore destructor is running on the main
+ thread, the background queue may be running code in ResourceLoadStatisticsMemoryStore or
+ ResourceLoadStatisticsPersistentStorage which refs the WebResourceLoadStatisticsStore, even
+ though its ref count has already reached 0. It is actually a common pattern in
+ ResourceLoadStatisticsMemoryStore to call RunLoop::main().dispatch() and ref their m_store in
+ the lambda, so that they can interact with the WebResourceLoadStatisticsStore.
+
+ To address the issue, we now destroy m_statisticsStore / m_persistentStorage *before* the
+ WebResourceLoadStatisticsStore destructor runs. The NetworkSession destructor now calls
+ WebResourceLoadStatisticsStore::didDestroyNetworkSession() which takes care of destroying
+ m_statisticsStore / m_persistentStorage on the background queue, synchronously. The
+ WebResourceLoadStatisticsStore destructor will only run later, once all remaining references
+ to it are gone.
+
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
+ (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
+ (WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
+ * NetworkProcess/NetworkSession.cpp:
+ (WebKit::NetworkSession::~NetworkSession):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247784 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-07-24 Chris Dumez <cdu...@apple.com>
+
+ Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
+ https://bugs.webkit.org/show_bug.cgi?id=200071
+ <rdar://problem/53335583>
+
+ Reviewed by Brent Fulgham and Youenn Fablet.
+
+ The WebResourceLoadStatisticsStore is a main thread object. In its destructor, it was dispatching
+ to the background queue to destroy the m_statisticsStore / m_persistentStorage data members, which
+ live on the background queue. It would then synchronously wait for the background queue to finish
+ destroying them. The idea was to guarantee that the ResourceLoadStatisticsMemoryStore and the
+ ResourceLoadStatisticsPersistentStorage would never outlive the WebResourceLoadStatisticsStore,
+ given that they keep a raw pointer back to the WebResourceLoadStatisticsStore (via m_store data
+ member).
+
+ The issue is that *while* the WebResourceLoadStatisticsStore destructor is running on the main
+ thread, the background queue may be running code in ResourceLoadStatisticsMemoryStore or
+ ResourceLoadStatisticsPersistentStorage which refs the WebResourceLoadStatisticsStore, even
+ though its ref count has already reached 0. It is actually a common pattern in
+ ResourceLoadStatisticsMemoryStore to call RunLoop::main().dispatch() and ref their m_store in
+ the lambda, so that they can interact with the WebResourceLoadStatisticsStore.
+
+ To address the issue, we now destroy m_statisticsStore / m_persistentStorage *before* the
+ WebResourceLoadStatisticsStore destructor runs. The NetworkSession destructor now calls
+ WebResourceLoadStatisticsStore::didDestroyNetworkSession() which takes care of destroying
+ m_statisticsStore / m_persistentStorage on the background queue, synchronously. The
+ WebResourceLoadStatisticsStore destructor will only run later, once all remaining references
+ to it are gone.
+
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
+ (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
+ (WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
+ * NetworkProcess/NetworkSession.cpp:
+ (WebKit::NetworkSession::~NetworkSession):
+
+2019-07-29 Alan Coon <alanc...@apple.com>
+
Cherry-pick r247782. rdar://problem/53647473
SYS___pthread_markcancel is sometimes used by libwebrtc
Modified: branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp (247974 => 247975)
--- branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2019-07-30 03:55:21 UTC (rev 247974)
+++ branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2019-07-30 03:55:24 UTC (rev 247975)
@@ -180,7 +180,15 @@
WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
{
ASSERT(RunLoop::isMain());
+ ASSERT(!m_statisticsStore);
+ ASSERT(!m_persistentStorage);
+}
+void WebResourceLoadStatisticsStore::didDestroyNetworkSession()
+{
+ ASSERT(RunLoop::isMain());
+
+ m_networkSession = nullptr;
flushAndDestroyPersistentStore();
}
@@ -335,6 +343,9 @@
return;
case StorageAccessStatus::RequiresUserPrompt:
{
+ if (!m_networkSession)
+ return completionHandler(StorageAccessWasGranted::No, StorageAccessPromptWasShown::No);
+
CompletionHandler<void(bool)> requestConfirmationCompletionHandler = [this, protectedThis = protectedThis.copyRef(), subFrameDomain, topFrameDomain, frameID, pageID, completionHandler = WTFMove(completionHandler)] (bool userDidGrantAccess) mutable {
if (userDidGrantAccess)
grantStorageAccess(subFrameDomain, topFrameDomain, frameID, pageID, StorageAccessPromptWasShown::Yes, WTFMove(completionHandler));
@@ -358,13 +369,11 @@
return;
}
- if (m_statisticsStore) {
- m_statisticsStore->requestStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID, pageID, [statusHandler = WTFMove(statusHandler)](StorageAccessStatus status) mutable {
- postTaskReply([statusHandler = WTFMove(statusHandler), status]() mutable {
- statusHandler(status);
- });
+ m_statisticsStore->requestStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID, pageID, [statusHandler = WTFMove(statusHandler)](StorageAccessStatus status) mutable {
+ postTaskReply([statusHandler = WTFMove(statusHandler), status]() mutable {
+ statusHandler(status);
});
- }
+ });
});
}
Modified: branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h (247974 => 247975)
--- branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2019-07-30 03:55:21 UTC (rev 247974)
+++ branches/safari-608-branch/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2019-07-30 03:55:24 UTC (rev 247975)
@@ -94,6 +94,8 @@
~WebResourceLoadStatisticsStore();
+ void didDestroyNetworkSession();
+
static const OptionSet<WebsiteDataType>& monitoredDataTypes();
WorkQueue& statisticsQueue() { return m_statisticsQueue.get(); }
Modified: branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.cpp (247974 => 247975)
--- branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.cpp 2019-07-30 03:55:21 UTC (rev 247974)
+++ branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.cpp 2019-07-30 03:55:24 UTC (rev 247975)
@@ -94,10 +94,25 @@
NetworkSession::~NetworkSession()
{
+#if ENABLE(RESOURCE_LOAD_STATISTICS)
+ destroyResourceLoadStatistics();
+#endif
+
m_storageManager->resume();
m_storageManager->waitUntilTasksFinished();
}
+#if ENABLE(RESOURCE_LOAD_STATISTICS)
+void NetworkSession::destroyResourceLoadStatistics()
+{
+ if (!m_resourceLoadStatistics)
+ return;
+
+ m_resourceLoadStatistics->didDestroyNetworkSession();
+ m_resourceLoadStatistics = nullptr;
+}
+#endif
+
void NetworkSession::invalidateAndCancel()
{
for (auto* task : m_dataTaskSet)
@@ -116,7 +131,7 @@
{
ASSERT(!m_isInvalidated);
if (!enable) {
- m_resourceLoadStatistics = nullptr;
+ destroyResourceLoadStatistics();
return;
}
Modified: branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.h (247974 => 247975)
--- branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.h 2019-07-30 03:55:21 UTC (rev 247974)
+++ branches/safari-608-branch/Source/WebKit/NetworkProcess/NetworkSession.h 2019-07-30 03:55:24 UTC (rev 247975)
@@ -110,6 +110,10 @@
protected:
NetworkSession(NetworkProcess&, PAL::SessionID, String&& localStorageDirectory, SandboxExtension::Handle&);
+#if ENABLE(RESOURCE_LOAD_STATISTICS)
+ void destroyResourceLoadStatistics();
+#endif
+
PAL::SessionID m_sessionID;
Ref<NetworkProcess> m_networkProcess;
HashSet<NetworkDataTask*> m_dataTaskSet;
