Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores...
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 19:28:18 +0000 (19:28 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 19:28:18 +0000 (19:28 +0000)
<rdar://problem/41019893> and https://bugs.webkit.org/show_bug.cgi?id=186682

Reviewed by Chris Dumez.

Source/WebKit:

* UIProcess/Storage/StorageProcessProxy.cpp:
(WebKit::StorageProcessProxy::didClose): Protect this and the process pool as the cleanup that follows
  might cause either to get destroyed.

* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::fetchDataAndApply): Protect the operating WebsiteDataStore while async operations
  are in flight. Otherwise if the data store is destroyed, the SessionIDs for those operations will get
  destroyed before they complete.
(WebKit::WebsiteDataStore::removeData): Ditto.

Tools:

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:
(TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232886 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3 [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-shm [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-wal [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm

index afd1b26..456d1cd 100644 (file)
@@ -1,3 +1,20 @@
+2018-06-15  Brady Eidson  <beidson@apple.com>
+
+        Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
+        <rdar://problem/41019893> and https://bugs.webkit.org/show_bug.cgi?id=186682
+
+        Reviewed by Chris Dumez.
+
+        * UIProcess/Storage/StorageProcessProxy.cpp:
+        (WebKit::StorageProcessProxy::didClose): Protect this and the process pool as the cleanup that follows
+          might cause either to get destroyed.
+
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::fetchDataAndApply): Protect the operating WebsiteDataStore while async operations
+          are in flight. Otherwise if the data store is destroyed, the SessionIDs for those operations will get
+          destroyed before they complete.
+        (WebKit::WebsiteDataStore::removeData): Ditto.
+
 2018-06-15  Per Arne Vollan  <pvollan@apple.com>
 
         DisplayRefreshMonitorMac should hold a weak pointer to WebPage.
index c55c06a..2d4baf2 100644 (file)
@@ -135,6 +135,8 @@ void StorageProcessProxy::getStorageProcessConnection(WebProcessProxy& webProces
 
 void StorageProcessProxy::didClose(IPC::Connection&)
 {
+    auto protectedProcessPool = makeRef(m_processPool);
+
     // The storage process must have crashed or exited, so send any pending sync replies we might have.
     while (!m_pendingConnectionReplies.isEmpty()) {
         auto reply = m_pendingConnectionReplies.takeFirst();
@@ -148,17 +150,14 @@ void StorageProcessProxy::didClose(IPC::Connection&)
 #endif
     }
 
-    for (const auto& callback : m_pendingFetchWebsiteDataCallbacks.values())
-        callback(WebsiteData());
-    m_pendingFetchWebsiteDataCallbacks.clear();
+    while (!m_pendingFetchWebsiteDataCallbacks.isEmpty())
+        m_pendingFetchWebsiteDataCallbacks.take(m_pendingFetchWebsiteDataCallbacks.begin()->key)(WebsiteData { });
 
-    for (const auto& callback : m_pendingDeleteWebsiteDataCallbacks.values())
-        callback();
-    m_pendingDeleteWebsiteDataCallbacks.clear();
+    while (!m_pendingDeleteWebsiteDataCallbacks.isEmpty())
+        m_pendingDeleteWebsiteDataCallbacks.take(m_pendingDeleteWebsiteDataCallbacks.begin()->key)();
 
-    for (const auto& callback : m_pendingDeleteWebsiteDataForOriginsCallbacks.values())
-        callback();
-    m_pendingDeleteWebsiteDataForOriginsCallbacks.clear();
+    while (!m_pendingDeleteWebsiteDataForOriginsCallbacks.isEmpty())
+        m_pendingDeleteWebsiteDataForOriginsCallbacks.take(m_pendingDeleteWebsiteDataForOriginsCallbacks.begin()->key)();
 
     // Tell ProcessPool to forget about this storage process. This may cause us to be deleted.
     m_processPool.storageProcessCrashed(this);
index a984c0f..c3f3182 100644 (file)
@@ -236,10 +236,11 @@ void WebsiteDataStore::fetchData(OptionSet<WebsiteDataType> dataTypes, OptionSet
 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)
+        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, RefPtr<WorkQueue>&& queue, Function<void(Vector<WebsiteDataRecord>)>&& apply, WebsiteDataStore& dataStore)
             : fetchOptions(fetchOptions)
             , queue(WTFMove(queue))
             , apply(WTFMove(apply))
+            , protectedDataStore(dataStore)
         {
         }
 
@@ -344,9 +345,10 @@ void WebsiteDataStore::fetchDataAndApply(OptionSet<WebsiteDataType> dataTypes, O
         Function<void(Vector<WebsiteDataRecord>)> apply;
 
         HashMap<String, WebsiteDataRecord> m_websiteDataRecords;
+        Ref<WebsiteDataStore> protectedDataStore;
     };
 
-    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(fetchOptions, WTFMove(queue), WTFMove(apply)));
+    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(fetchOptions, WTFMove(queue), WTFMove(apply), *this));
 
 #if ENABLE(VIDEO)
     if (dataTypes.contains(WebsiteDataType::DiskCache)) {
@@ -648,8 +650,9 @@ static ProcessAccessType computeWebProcessAccessTypeForDataRemoval(OptionSet<Web
 void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, WallTime modifiedSince, Function<void()>&& completionHandler)
 {
     struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
-        explicit CallbackAggregator(Function<void()>&& completionHandler)
+        explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
             : completionHandler(WTFMove(completionHandler))
+            , protectedDataStore(dataStore)
         {
         }
 
@@ -674,9 +677,10 @@ void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, WallTime
 
         unsigned pendingCallbacks = 0;
         Function<void()> completionHandler;
+        Ref<WebsiteDataStore> protectedDataStore;
     };
 
-    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(WTFMove(completionHandler)));
+    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(*this, WTFMove(completionHandler)));
 
 #if ENABLE(VIDEO)
     if (dataTypes.contains(WebsiteDataType::DiskCache)) {
@@ -906,8 +910,9 @@ void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, const Ve
     }
 
     struct CallbackAggregator : ThreadSafeRefCounted<CallbackAggregator> {
-        explicit CallbackAggregator(Function<void()>&& completionHandler)
+        explicit CallbackAggregator(WebsiteDataStore& dataStore, Function<void()>&& completionHandler)
             : completionHandler(WTFMove(completionHandler))
+            , protectedDataStore(dataStore)
         {
         }
 
@@ -932,9 +937,10 @@ void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, const Ve
 
         unsigned pendingCallbacks = 0;
         Function<void()> completionHandler;
+        Ref<WebsiteDataStore> protectedDataStore;
     };
 
-    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(WTFMove(completionHandler)));
+    RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(*this, WTFMove(completionHandler)));
     
     if (dataTypes.contains(WebsiteDataType::DiskCache)) {
         HashSet<WebCore::SecurityOriginData> origins;
index 59005c7..ea058ab 100644 (file)
@@ -1,3 +1,14 @@
+2018-06-15  Brady Eidson  <beidson@apple.com>
+
+        Crash in both StorageProcess and UIProcess when using custom WKWebsiteDataStores for data management.
+        <rdar://problem/41019893> and https://bugs.webkit.org/show_bug.cgi?id=186682
+
+        Reviewed by Chris Dumez.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:
+        (TEST):
+
 2018-06-15  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] Fix adding error in browserperfdash_runner.
index 128a87a..b4763f9 100644 (file)
                5120C83E1E67678F0025B250 /* WebsiteDataStoreCustomPaths.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5120C83B1E674E350025B250 /* WebsiteDataStoreCustomPaths.html */; };
                51393E221523952D005F39C5 /* DOMWindowExtensionBasic_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 51393E1D1523944A005F39C5 /* DOMWindowExtensionBasic_Bundle.cpp */; };
                5142B2731517C8C800C32B19 /* ContextMenuCanCopyURL.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5142B2721517C89100C32B19 /* ContextMenuCanCopyURL.html */; };
+               51460E1220D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */; };
+               51460E1320D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */; };
+               51460E1420D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal in Copy Resources */ = {isa = PBXBuildFile; fileRef = 51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */; };
                514958BE1F7427AC00E87BAD /* WKWebViewAutofillTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 514958BD1F7427AC00E87BAD /* WKWebViewAutofillTests.mm */; };
                515BE16F1D428BB100DD7C68 /* StoreBlobToBeDeleted.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 515BE16E1D4288FF00DD7C68 /* StoreBlobToBeDeleted.html */; };
                515BE1711D428E4B00DD7C68 /* StoreBlobThenDelete.mm in Sources */ = {isa = PBXBuildFile; fileRef = 515BE1701D428BD100DD7C68 /* StoreBlobThenDelete.mm */; };
                        dstPath = TestWebKitAPI.resources;
                        dstSubfolderSpec = 7;
                        files = (
+                               51460E1220D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 in Copy Resources */,
+                               51460E1320D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm in Copy Resources */,
+                               51460E1420D421F2005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal in Copy Resources */,
                                1A9E52C913E65EF4006917F5 /* 18-characters.html in Copy Resources */,
                                379028B914FAC24C007E6B43 /* acceptsFirstMouse.html in Copy Resources */,
                                1C2B81871C8925A000A5529F /* Ahem.ttf in Copy Resources */,
                51393E1E1523944A005F39C5 /* DOMWindowExtensionBasic.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DOMWindowExtensionBasic.cpp; sourceTree = "<group>"; };
                5142B2701517C88B00C32B19 /* ContextMenuCanCopyURL.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ContextMenuCanCopyURL.mm; sourceTree = "<group>"; };
                5142B2721517C89100C32B19 /* ContextMenuCanCopyURL.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = ContextMenuCanCopyURL.html; sourceTree = "<group>"; };
+               51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3"; sourceTree = "<group>"; };
+               51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3-shm"; sourceTree = "<group>"; };
+               51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-2.sqlite3-wal"; sourceTree = "<group>"; };
                514958BD1F7427AC00E87BAD /* WKWebViewAutofillTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebViewAutofillTests.mm; sourceTree = "<group>"; };
                515BE16E1D4288FF00DD7C68 /* StoreBlobToBeDeleted.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = StoreBlobToBeDeleted.html; sourceTree = "<group>"; };
                515BE1701D428BD100DD7C68 /* StoreBlobThenDelete.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = StoreBlobThenDelete.mm; sourceTree = "<group>"; };
                A16F66B81C40E9E100BD4D24 /* Resources */ = {
                        isa = PBXGroup;
                        children = (
+                               51460E0F20D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3 */,
+                               51460E1020D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-shm */,
+                               51460E1120D4216E005345F2 /* SimpleServiceWorkerRegistrations-2.sqlite3-wal */,
                                C25CCA0C1E5140E50026CB8A /* AllAhem.svg */,
                                F4A9202E1FEE34C800F59590 /* apple-data-url.html */,
                                F47D30EB1ED28619000482E1 /* apple.gif */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3 b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3
new file mode 100644 (file)
index 0000000..3420cd8
Binary files /dev/null and b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3 differ
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-shm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-shm
new file mode 100644 (file)
index 0000000..c41f4cb
Binary files /dev/null and b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-shm differ
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-wal b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-wal
new file mode 100644 (file)
index 0000000..92e4486
Binary files /dev/null and b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SimpleServiceWorkerRegistrations-2.sqlite3-wal differ
index 147a0aa..797ea4e 100644 (file)
@@ -225,6 +225,66 @@ TEST(WebKit, WebsiteDataStoreCustomPaths)
     EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
 }
 
+TEST(WebKit, CustomDataStorePathsVersusCompletionHandlers)
+{
+    // Copy the baked database files to the database directory
+    NSURL *url1 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3" subdirectory:@"TestWebKitAPI.resources"];
+    NSURL *url2 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3-shm" subdirectory:@"TestWebKitAPI.resources"];
+    NSURL *url3 = [[NSBundle mainBundle] URLForResource:@"SimpleServiceWorkerRegistrations-2" withExtension:@"sqlite3-wal" subdirectory:@"TestWebKitAPI.resources"];
+
+    NSURL *swPath = [NSURL fileURLWithPath:[@"~/Library/Caches/TestWebKitAPI/WebKit/ServiceWorkers/" stringByExpandingTildeInPath]];
+    [[NSFileManager defaultManager] removeItemAtURL:swPath error:nil];
+    EXPECT_FALSE([[NSFileManager defaultManager] fileExistsAtPath:swPath.path]);
+
+    [[NSFileManager defaultManager] createDirectoryAtURL:swPath withIntermediateDirectories:YES attributes:nil error:nil];
+    [[NSFileManager defaultManager] copyItemAtURL:url1 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3"] error:nil];
+    [[NSFileManager defaultManager] copyItemAtURL:url2 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3-shm"] error:nil];
+    [[NSFileManager defaultManager] copyItemAtURL:url3 toURL:[swPath URLByAppendingPathComponent:@"ServiceWorkerRegistrations-2.sqlite3-wal"] error:nil];
+
+    auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
+    websiteDataStoreConfiguration.get()._serviceWorkerRegistrationDirectory = swPath;
+    auto dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
+
+    // Fetch SW records
+    auto websiteDataTypes = adoptNS([[NSSet alloc] initWithArray:@[WKWebsiteDataTypeServiceWorkerRegistrations]]);
+    static bool readyToContinue;
+    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        EXPECT_EQ((int)dataRecords.count, 1);
+        readyToContinue = true;
+    }];
+    TestWebKitAPI::Util::run(&readyToContinue);
+    readyToContinue = false;
+
+    // Fetch records again, this time releasing our reference to the data store while the request is in flight.
+    // Without a bug fix, this would crash the StorageProcess and the UI process wouldn't get the info back.
+    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        EXPECT_EQ((int)dataRecords.count, 1);
+        readyToContinue = true;
+    }];
+    dataStore = nil;
+    TestWebKitAPI::Util::run(&readyToContinue);
+    readyToContinue = false;
+
+    // Delete all SW records, releasing our reference to the data store while the request is in flight.
+    // Same as above - We used to crash the storage process and the records weren't actually deleted.
+    dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
+    [dataStore removeDataOfTypes:websiteDataTypes.get() modifiedSince:[NSDate distantPast] completionHandler:^() {
+        readyToContinue = true;
+    }];
+    dataStore = nil;
+    TestWebKitAPI::Util::run(&readyToContinue);
+    readyToContinue = false;
+
+    // The StorageProcess should not have crashed, the records should have been deleted, and the callback should have been made.
+    // Now refetch the records to verify they are gone.
+    dataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]);
+    [dataStore fetchDataRecordsOfTypes:websiteDataTypes.get() completionHandler:^(NSArray<WKWebsiteDataRecord *> *dataRecords) {
+        EXPECT_EQ((int)dataRecords.count, 0);
+        readyToContinue = true;
+    }];
+    TestWebKitAPI::Util::run(&readyToContinue);
+}
+
 TEST(WebKit, WebsiteDataStoreEphemeral)
 {
     RetainPtr<WebsiteDataStoreCustomPathsMessageHandler> handler = adoptNS([[WebsiteDataStoreCustomPathsMessageHandler alloc] init]);