Changeset 248959 in webkit

Timestamp:

Aug 21, 2019, 1:02:35 PM (7 years ago)

Author:

Chris Dumez

Message:

Crash under StringImpl::~StringImpl() in NetworkProcess::deleteWebsiteDataForRegistrableDomains()
​https://bugs.webkit.org/show_bug.cgi?id=200986
<rdar://problem/32850192>

Reviewed by Brent Fulgham.

Code was calling postStorageTask() with a lambda that captures Strings that are not isolated copied.
The lambda would get executed on another thread so this is not safe. The CrossThreadTask constructor
does not take care of this for you, despite its name (the createCrossThreadTask() function does though).

• NetworkProcess/NetworkProcess.cpp:

(WebKit::NetworkProcess::fetchWebsiteData):
(WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains):
(WebKit::NetworkProcess::registrableDomainsWithWebsiteData):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkProcess.cpp (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-08-21  Chris Dumez  <cdumez@apple.com>
+
+        Crash under StringImpl::~StringImpl() in NetworkProcess::deleteWebsiteDataForRegistrableDomains()
+        https://bugs.webkit.org/show_bug.cgi?id=200986
+        <rdar://problem/32850192>
+
+        Reviewed by Brent Fulgham.
+
+        Code was calling postStorageTask() with a lambda that captures Strings that are not isolated copied.
+        The lambda would get executed on another thread so this is not safe. The CrossThreadTask constructor
+        does not take care of this for you, despite its name (the createCrossThreadTask() function does though).
+
+        * NetworkProcess/NetworkProcess.cpp:
+        (WebKit::NetworkProcess::fetchWebsiteData):
+        (WebKit::NetworkProcess::deleteWebsiteDataForRegistrableDomains):
+        (WebKit::NetworkProcess::registrableDomainsWithWebsiteData):
+
 2019-08-21  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

@@ -1361 +1361 @@
     if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::IndexedDBDatabases)) {
         // FIXME: Pick the right database store based on the session ID.
-        postStorageTask(CrossThreadTask([this, callbackAggregator = callbackAggregator.copyRef(), path = WTFMove(path)]() mutable {
+        postStorageTask(CrossThreadTask([this, callbackAggregator = callbackAggregator.copyRef(), path = crossThreadCopy(path)]() mutable {
             RunLoop::main().dispatch([callbackAggregator = WTFMove(callbackAggregator), securityOrigins = indexedDatabaseOrigins(path)] {
                 for (const auto& securityOrigin : securityOrigins)
@@ -1733 +1733 @@
     if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::IndexedDBDatabases)) {
         // FIXME: Pick the right database store based on the session ID.
-        postStorageTask(CrossThreadTask([this, sessionID, callbackAggregator = callbackAggregator.copyRef(), path = WTFMove(path), domainsToDeleteAllButCookiesFor]() mutable {
-            RunLoop::main().dispatch([this, sessionID, domainsToDeleteAllButCookiesFor = crossThreadCopy(domainsToDeleteAllButCookiesFor), callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {
+        postStorageTask(CrossThreadTask([this, sessionID, callbackAggregator = callbackAggregator.copyRef(), path = crossThreadCopy(path), domainsToDeleteAllButCookiesFor = crossThreadCopy(domainsToDeleteAllButCookiesFor)]() mutable {
+            RunLoop::main().dispatch([this, sessionID, domainsToDeleteAllButCookiesFor = WTFMove(domainsToDeleteAllButCookiesFor), callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {
                 Vector<SecurityOriginData> entriesToDelete;
                 for (const auto& securityOrigin : securityOrigins) {
@@ -1879 +1879 @@
     if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::IndexedDBDatabases)) {
         // FIXME: Pick the right database store based on the session ID.
-        postStorageTask(CrossThreadTask([this, callbackAggregator = callbackAggregator.copyRef(), path = WTFMove(path)]() mutable {
+        postStorageTask(CrossThreadTask([this, callbackAggregator = callbackAggregator.copyRef(), path = crossThreadCopy(path)]() mutable {
             RunLoop::main().dispatch([callbackAggregator = callbackAggregator.copyRef(), securityOrigins = indexedDatabaseOrigins(path)] {
                 for (const auto& securityOrigin : securityOrigins)