Modified: trunk/Source/WebKit/ChangeLog (233537 => 233538)
--- trunk/Source/WebKit/ChangeLog 2018-07-05 20:28:54 UTC (rev 233537)
+++ trunk/Source/WebKit/ChangeLog 2018-07-05 20:41:48 UTC (rev 233538)
@@ -1,3 +1,29 @@
+2018-07-05 Chris Dumez <[email protected]>
+
+ Regression(r232886): WebsiteDataStore objects may get destroyed on a background thread
+ https://bugs.webkit.org/show_bug.cgi?id=187356
+ <rdar://problem/41854555>
+
+ Reviewed by Geoffrey Garen.
+
+ As of r232886, CallbackAggregators in WebsiteDataStore hold a Ref<> to their WebsiteDataStore. This
+ is an issue because CallbackAggregator objects can get destroyed on a background thread and may be
+ the last ones holding a ref to the data store. When this happens, the WebsiteDataStore would get
+ destroyed on a background store and potentially cause crashes. Note that even if the callback
+ aggregator would not be the last one to hold a ref to the store, it still would not be safe to deref
+ the store on the background thread since WebsiteDataStore is not ThreadSafeRefCounted.
+
+ To address the issue, this patch updates the CallbackAggregators' destructor to deref their data
+ store member on the main thread. Note that we could also have WebsiteDataStore subclass
+ ThreadSafeRefCounted<T, DestructionThread::Main> but the data store technically does not need to
+ be ThreadSafeRefCounted at the moment.
+
+ * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+ (WebKit::WebsiteDataStore::WebsiteDataStore):
+ (WebKit::WebsiteDataStore::~WebsiteDataStore):
+ (WebKit::WebsiteDataStore::fetchDataAndApply):
+ (WebKit::WebsiteDataStore::removeData):
+
2018-07-05 Dan Bernstein <[email protected]>
[macOS] REGRESSION: Development WebContent service has restricted entitlements, rendering it useless for development
Modified: trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp (233537 => 233538)
--- trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp 2018-07-05 20:28:54 UTC (rev 233537)
+++ trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp 2018-07-05 20:41:48 UTC (rev 233538)
@@ -100,6 +100,8 @@
WTF::setProcessPrivileges(allPrivileges());
maybeRegisterWithSessionIDMap();
platformInitialize();
+
+ ASSERT(RunLoop::isMain());
}
WebsiteDataStore::WebsiteDataStore(PAL::SessionID sessionID)
@@ -109,10 +111,14 @@
{
maybeRegisterWithSessionIDMap();
platformInitialize();
+
+ ASSERT(RunLoop::isMain());
}
WebsiteDataStore::~WebsiteDataStore()
{
+ ASSERT(RunLoop::isMain());
+
platformDestroy();
if (m_sessionID.isValid() && m_sessionID != PAL::SessionID::defaultSessionID()) {
@@ -239,17 +245,21 @@
void WebsiteDataStore::fetchDataAndApply(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply)
{
struct CallbackAggregator final : ThreadSafeRefCounted<CallbackAggregator> {
- explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply, WebsiteDataStore& dataStore)
+ CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply, WebsiteDataStore& dataStore)
: fetchOptions(fetchOptions)
, queue(WTFMove(queue))
, apply(WTFMove(apply))
, protectedDataStore(dataStore)
{
+ ASSERT(RunLoop::isMain());
}
~CallbackAggregator()
{
ASSERT(!pendingCallbacks);
+
+ // Make sure the data store gets destroyed on the main thread even though the CallbackAggregator can get destroyed on a background queue.
+ RunLoop::main().dispatch([protectedDataStore = WTFMove(protectedDataStore)] { });
}
void addPendingCallback()
@@ -653,12 +663,19 @@
void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, WallTime modifiedSince, Function<void()>&& completionHandler)
{
struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
- explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
+ CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
: completionHandler(WTFMove(completionHandler))
, protectedDataStore(dataStore)
{
+ ASSERT(RunLoop::isMain());
}
+ ~CallbackAggregator()
+ {
+ // Make sure the data store gets destroyed on the main thread even though the CallbackAggregator can get destroyed on a background queue.
+ RunLoop::main().dispatch([protectedDataStore = WTFMove(protectedDataStore)] { });
+ }
+
void addPendingCallback()
{
pendingCallbacks++;
@@ -913,12 +930,19 @@
}
struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
- explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
+ CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
: completionHandler(WTFMove(completionHandler))
, protectedDataStore(dataStore)
{
+ ASSERT(RunLoop::isMain());
}
+ ~CallbackAggregator()
+ {
+ // Make sure the data store gets destroyed on the main thread even though the CallbackAggregator can get destroyed on a background queue.
+ RunLoop::main().dispatch([protectedDataStore = WTFMove(protectedDataStore)] { });
+ }
+
void addPendingCallback()
{
pendingCallbacks++;
