Title: [273996] trunk/Source/WebKit
- Revision
- 273996
- Author
- [email protected]
- Date
- 2021-03-05 12:49:27 -0800 (Fri, 05 Mar 2021)
Log Message
Fix potential thread-safety issue in WebsiteDataStore::fetchDataAndApply()
https://bugs.webkit.org/show_bug.cgi?id=222807
Reviewed by Geoffrey Garen.
Fix an issue when WebsiteDataStore::fetchDataAndApply() failed to create an isolated
copy of the WebsiteDataRecords before passing them to its internal dispatch queue.
Also add more threading assertions to help catch issues since we have crashes in this
area of the code.
* UIProcess/WebsiteData/WebsiteDataRecord.cpp:
(WebKit::WebsiteDataRecord::isolatedCopy const):
* UIProcess/WebsiteData/WebsiteDataRecord.h:
* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::fetchDataAndApply):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (273995 => 273996)
--- trunk/Source/WebKit/ChangeLog 2021-03-05 20:47:54 UTC (rev 273995)
+++ trunk/Source/WebKit/ChangeLog 2021-03-05 20:49:27 UTC (rev 273996)
@@ -1,3 +1,21 @@
+2021-03-05 Chris Dumez <[email protected]>
+
+ Fix potential thread-safety issue in WebsiteDataStore::fetchDataAndApply()
+ https://bugs.webkit.org/show_bug.cgi?id=222807
+
+ Reviewed by Geoffrey Garen.
+
+ Fix an issue when WebsiteDataStore::fetchDataAndApply() failed to create an isolated
+ copy of the WebsiteDataRecords before passing them to its internal dispatch queue.
+ Also add more threading assertions to help catch issues since we have crashes in this
+ area of the code.
+
+ * UIProcess/WebsiteData/WebsiteDataRecord.cpp:
+ (WebKit::WebsiteDataRecord::isolatedCopy const):
+ * UIProcess/WebsiteData/WebsiteDataRecord.h:
+ * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+ (WebKit::WebsiteDataStore::fetchDataAndApply):
+
2021-03-05 Ryan Haddad <[email protected]>
Unreviewed, fix the build with recent SDKs.
Modified: trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.cpp (273995 => 273996)
--- trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.cpp 2021-03-05 20:47:54 UTC (rev 273995)
+++ trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.cpp 2021-03-05 20:49:27 UTC (rev 273996)
@@ -29,6 +29,7 @@
#include <WebCore/LocalizedStrings.h>
#include <WebCore/PublicSuffix.h>
#include <WebCore/SecurityOrigin.h>
+#include <wtf/CrossThreadCopier.h>
#if PLATFORM(COCOA)
#import <pal/spi/cf/CFNetworkSPI.h>
@@ -170,4 +171,23 @@
return emptyString();
}
+WebsiteDataRecord WebsiteDataRecord::isolatedCopy() const
+{
+ return WebsiteDataRecord {
+ crossThreadCopy(displayName),
+ types,
+ size,
+ crossThreadCopy(origins),
+ crossThreadCopy(cookieHostNames),
+#if ENABLE(NETSCAPE_PLUGIN_API)
+ crossThreadCopy(pluginDataHostNames),
+#endif
+ crossThreadCopy(HSTSCacheHostNames),
+ crossThreadCopy(alternativeServicesHostNames),
+#if ENABLE(RESOURCE_LOAD_STATISTICS)
+ crossThreadCopy(resourceLoadStatisticsRegistrableDomains),
+#endif
+ };
}
+
+}
Modified: trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.h (273995 => 273996)
--- trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.h 2021-03-05 20:47:54 UTC (rev 273995)
+++ trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.h 2021-03-05 20:49:27 UTC (rev 273996)
@@ -59,6 +59,11 @@
void addResourceLoadStatisticsRegistrableDomain(const WebCore::RegistrableDomain&);
#endif
+ bool matches(const WebCore::RegistrableDomain&) const;
+ String topPrivatelyControlledDomain();
+
+ WebsiteDataRecord isolatedCopy() const;
+
String displayName;
OptionSet<WebsiteDataType> types;
@@ -78,9 +83,6 @@
#if ENABLE(RESOURCE_LOAD_STATISTICS)
HashSet<WebCore::RegistrableDomain> resourceLoadStatisticsRegistrableDomains;
#endif
-
- bool matches(const WebCore::RegistrableDomain&) const;
- String topPrivatelyControlledDomain();
};
}
Modified: trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp (273995 => 273996)
--- trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp 2021-03-05 20:47:54 UTC (rev 273995)
+++ trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp 2021-03-05 20:49:27 UTC (rev 273996)
@@ -310,7 +310,7 @@
void WebsiteDataStore::fetchDataAndApply(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply)
{
- struct CallbackAggregator final : ThreadSafeRefCounted<CallbackAggregator> {
+ struct CallbackAggregator final : ThreadSafeRefCounted<CallbackAggregator, WTF::DestructionThread::MainRunLoop> {
CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply, WebsiteDataStore& dataStore)
: fetchOptions(fetchOptions)
, queue(WTFMove(queue))
@@ -322,19 +322,19 @@
~CallbackAggregator()
{
+ ASSERT(RunLoop::isMain());
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()
{
- pendingCallbacks++;
+ ASSERT(RunLoop::isMain());
+ ++pendingCallbacks;
}
void removePendingCallback(WebsiteData websiteData)
{
+ ASSERT(RunLoop::isMain());
ASSERT(pendingCallbacks);
--pendingCallbacks;
@@ -420,6 +420,7 @@
void callIfNeeded()
{
+ ASSERT(RunLoop::isMain());
if (pendingCallbacks)
return;
@@ -426,7 +427,7 @@
Vector<WebsiteDataRecord> records;
records.reserveInitialCapacity(m_websiteDataRecords.size());
for (auto& record : m_websiteDataRecords.values())
- records.uncheckedAppend(WTFMove(record));
+ records.uncheckedAppend(queue ? crossThreadCopy(record) : WTFMove(record));
auto processRecords = [apply = WTFMove(apply), records = WTFMove(records)] () mutable {
apply(WTFMove(records));
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
