Assert in NetworkBlobRegistry::unregisterBlobURL after network process had terminated
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Aug 2018 20:51:19 +0000 (20:51 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Aug 2018 20:51:19 +0000 (20:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188880

Reviewed by Saam Barati.

Source/WebKit:

Removed the debug assertion. WebContent process might be asking this network process
to unregister a blob registered from another network processs which had since crashed.

We could keep track of which blob had been registered with which network process
in WebContent process and avoid sending IPC to the network process but that's a lot of
house-keeping for virtually no benefit other than not hitting this assertion.

* NetworkProcess/FileAPI/NetworkBlobRegistry.cpp:
(WebKit::NetworkBlobRegistry::unregisterBlobURL):

Tools:

Fixed the bug that testRunner's terminateNetworkProcess, terminateServiceWorkerProcess, and terminateStorageProcess
were asynchronously terminating respective processes. Do so synchronously so that we can deterministically
test WebKit's behavior in layout tests.

* WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::terminateNetworkProcess):
(WTR::TestRunner::terminateServiceWorkerProcess):
(WTR::TestRunner::terminateStorageProcess):
* WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle):
(WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):

LayoutTests:

Added a layout test which demonstrates this debug assertion.

* TestExpectations:
* fast/files/blob-network-process-crash-expected.txt: Added.
* fast/files/blob-network-process-crash.html: Added.
* platform/wk2/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/files/blob-network-process-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/files/blob-network-process-crash.html [new file with mode: 0644]
LayoutTests/platform/wk2/TestExpectations
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp
Tools/ChangeLog
Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
Tools/WebKitTestRunner/TestInvocation.cpp

index ae73e31..b4cc988 100644 (file)
@@ -1,3 +1,17 @@
+2018-08-22  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assert in NetworkBlobRegistry::unregisterBlobURL after network process had terminated
+        https://bugs.webkit.org/show_bug.cgi?id=188880
+
+        Reviewed by Saam Barati.
+
+        Added a layout test which demonstrates this debug assertion.
+
+        * TestExpectations:
+        * fast/files/blob-network-process-crash-expected.txt: Added.
+        * fast/files/blob-network-process-crash.html: Added.
+        * platform/wk2/TestExpectations:
+
 2018-08-23  Youenn Fablet  <youenn@apple.com>
 
         Update libwebrtc up to 984f1a80c0
index 2b94980..db7c940 100644 (file)
@@ -389,6 +389,9 @@ http/tests/xmlhttprequest/gzip-content-type-no-content-encoding.html [ Skip ]
 # Only supported in WebKit2.
 http/wpt/cross-origin-resource-policy/ [ Skip ]
 
+# testRunner.terminateNetworkProcess() only works in WebKit2
+fast/files/blob-network-process-crash.html [ Skip ]
+
 #//////////////////////////////////////////////////////////////////////////////////////////
 # End platform-specific tests.
 #//////////////////////////////////////////////////////////////////////////////////////////
diff --git a/LayoutTests/fast/files/blob-network-process-crash-expected.txt b/LayoutTests/fast/files/blob-network-process-crash-expected.txt
new file mode 100644 (file)
index 0000000..a7cc400
--- /dev/null
@@ -0,0 +1,2 @@
+This tests unregistering a Blob object after the network process to which it was registered crashed.
+WebKit should not hit a debug assertion in the network process.
diff --git a/LayoutTests/fast/files/blob-network-process-crash.html b/LayoutTests/fast/files/blob-network-process-crash.html
new file mode 100644 (file)
index 0000000..1c403f9
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests unregistering a Blob object after the network process to which it was registered crashed.<br>
+WebKit should not hit a debug assertion in the network process.</p>
+<script>
+
+if (!window.testRunner)
+    document.write('This test requires testRunner and GCController');
+else {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+
+    let blobs = new Array(100);
+    for (let i = 0; i < 100; i++) {
+        blobs[i] = new Array(10);
+        for (let j = 0; j < 10; j++)
+            blobs[i][j] = new Blob(["some text"]);
+    }
+
+    testRunner.terminateNetworkProcess();
+
+    setTimeout(() => {
+        const newBlob = new Blob(["some text"]);
+        for (let i = 0; i < 100; i++) {
+            for (let j = 0; j < 10; j++)
+                blobs[i][j] = { };
+        }
+        blobs = null;
+        GCController.collect();
+        fetch('blob-network-process-crash.html').then(() => {
+            testRunner.notifyDone();
+        });
+    }, 0);
+}
+
+</script>
+</body>
+</html>
index e1a5679..a1c6147 100644 (file)
@@ -740,6 +740,8 @@ http/wpt/cross-origin-resource-policy/ [ Pass ]
 http/tests/navigation/useragent-reload.php [ Pass ]
 webkit.org/b/187931 storage/indexeddb/modern/opendatabase-after-storage-crash.html [ Skip ]
 
+# testRunner.terminateNetworkProcess() only works in WebKit2
+fast/files/blob-network-process-crash.html [ Pass ]
 
 ### END OF (5) Progressions, expected successes that are expected failures in WebKit1.
 ########################################
index ea7a423..f7a618d 100644 (file)
@@ -1,3 +1,20 @@
+2018-08-22  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assert in NetworkBlobRegistry::unregisterBlobURL after network process had terminated
+        https://bugs.webkit.org/show_bug.cgi?id=188880
+
+        Reviewed by Saam Barati.
+
+        Removed the debug assertion. WebContent process might be asking this network process
+        to unregister a blob registered from another network processs which had since crashed.
+
+        We could keep track of which blob had been registered with which network process
+        in WebContent process and avoid sending IPC to the network process but that's a lot of
+        house-keeping for virtually no benefit other than not hitting this assertion.
+
+        * NetworkProcess/FileAPI/NetworkBlobRegistry.cpp:
+        (WebKit::NetworkBlobRegistry::unregisterBlobURL):
+
 2018-08-23  Sihui Liu  <sihui_liu@apple.com>
 
         Remove keys of defaults that are no longer used in webProcessPool
index fda8b91..482fd13 100644 (file)
@@ -122,7 +122,6 @@ void NetworkBlobRegistry::unregisterBlobURL(NetworkConnectionToWebProcess* conne
 
     blobRegistry().unregisterBlobURL(url);
 
-    ASSERT(mapIterator->value.contains(url));
     mapIterator->value.remove(url);
 }
 
index f038e38..2939bf5 100644 (file)
@@ -1,3 +1,22 @@
+2018-08-22  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Assert in NetworkBlobRegistry::unregisterBlobURL after network process had terminated
+        https://bugs.webkit.org/show_bug.cgi?id=188880
+
+        Reviewed by Saam Barati.
+
+        Fixed the bug that testRunner's terminateNetworkProcess, terminateServiceWorkerProcess, and terminateStorageProcess
+        were asynchronously terminating respective processes. Do so synchronously so that we can deterministically
+        test WebKit's behavior in layout tests.
+
+        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
+        (WTR::TestRunner::terminateNetworkProcess):
+        (WTR::TestRunner::terminateServiceWorkerProcess):
+        (WTR::TestRunner::terminateStorageProcess):
+        * WebKitTestRunner/TestInvocation.cpp:
+        (WTR::TestInvocation::didReceiveMessageFromInjectedBundle):
+        (WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):
+
 2018-08-23  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [WHLSL] Allow native types to have type arguments (like "vector<float, 4>")
index 8446f73..d3cf06b 100644 (file)
@@ -1236,19 +1236,19 @@ void TestRunner::setShouldDownloadUndisplayableMIMETypes(bool value)
 void TestRunner::terminateNetworkProcess()
 {
     WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("TerminateNetworkProcess"));
-    WKBundlePagePostMessage(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr);
+    WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr, nullptr);
 }
 
 void TestRunner::terminateServiceWorkerProcess()
 {
     WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("TerminateServiceWorkerProcess"));
-    WKBundlePagePostMessage(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr);
+    WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr, nullptr);
 }
 
 void TestRunner::terminateStorageProcess()
 {
     WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("TerminateStorageProcess"));
-    WKBundlePagePostMessage(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr);
+    WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr, nullptr);
 }
 
 static unsigned nextUIScriptCallbackID()
index 645d74a..f8ee616 100644 (file)
@@ -735,24 +735,6 @@ void TestInvocation::didReceiveMessageFromInjectedBundle(WKStringRef messageName
         return;
     }
 
-    if (WKStringIsEqualToUTF8CString(messageName, "TerminateStorageProcess")) {
-        ASSERT(!messageBody);
-        TestController::singleton().terminateStorageProcess();
-        return;
-    }
-
-    if (WKStringIsEqualToUTF8CString(messageName, "TerminateNetworkProcess")) {
-        ASSERT(!messageBody);
-        TestController::singleton().terminateNetworkProcess();
-        return;
-    }
-
-    if (WKStringIsEqualToUTF8CString(messageName, "TerminateServiceWorkerProcess")) {
-        ASSERT(!messageBody);
-        TestController::singleton().terminateServiceWorkerProcess();
-        return;
-    }
-
     if (WKStringIsEqualToUTF8CString(messageName, "RunUIProcessScript")) {
         WKDictionaryRef messageBodyDictionary = static_cast<WKDictionaryRef>(messageBody);
         WKRetainPtr<WKStringRef> scriptKey(AdoptWK, WKStringCreateWithUTF8CString("Script"));
@@ -1435,6 +1417,24 @@ WKRetainPtr<WKTypeRef> TestInvocation::didReceiveSynchronousMessageFromInjectedB
         return nullptr;
     }
 
+    if (WKStringIsEqualToUTF8CString(messageName, "TerminateStorageProcess")) {
+        ASSERT(!messageBody);
+        TestController::singleton().terminateStorageProcess();
+        return nullptr;
+    }
+
+    if (WKStringIsEqualToUTF8CString(messageName, "TerminateNetworkProcess")) {
+        ASSERT(!messageBody);
+        TestController::singleton().terminateNetworkProcess();
+        return nullptr;
+    }
+
+    if (WKStringIsEqualToUTF8CString(messageName, "TerminateServiceWorkerProcess")) {
+        ASSERT(!messageBody);
+        TestController::singleton().terminateServiceWorkerProcess();
+        return nullptr;
+    }
+
     ASSERT_NOT_REACHED();
     return nullptr;
 }