Attachment 343384 Details for Bug 186941 – Patch

[patch] Patch

bug-186941-20180622155433.patch (text/plain), 5.53 KB, created by Chris Dumez on 2018-06-22 15:54:34 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-22 15:54:34 PDT

Size: 5.53 KB

patch

obsolete

>Subversion Revision: 233093
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 60ba70d2cddfcc3ac05ad32d019b7eb6b505180a..421ec43df87f977e4cfb7d02aff7d883e8b420c6 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-06-22  Chris Dumez  <cdumez@apple.com>
>+
>+        Do not do early processing of incoming sync IPC unless we're waiting for a sync IPC reply
>+        https://bugs.webkit.org/show_bug.cgi?id=186941
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The comment was claiming we were processing incoming sync messages while waiting for a
>+        sync IPC reply to prevent deadlocks. However, the code was failing to check if we were
>+        waiting for a sync IPC reply. As a result, incoming sync IPC messages would get processed
>+        early no matter what, jumping the line. This was the source of the flakiness in the blob
>+        tests since the IPC to register the blob in the network process was async and the follow-up
>+        IPC to ask the network process for the blob size was sync. The sync message to get the blob
>+        size would jump the line and get processed before the async message to register the blob.
>+        As a result, the network process would not know about the blob yet and return size 0. Of
>+        course, this could happen if the network process was sending sync IPC at the time. However,
>+        the network process never sends any sync IPC and therefore, should never process incoming
>+        IPC messages out of order.
>+
>+        * Platform/IPC/Connection.cpp:
>+        (IPC::Connection::processIncomingMessage):
>+
> 2018-06-22  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebResourceLoadStatisticsStore::mergeStatistics(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)
>diff --git a/Source/WebKit/Platform/IPC/Connection.cpp b/Source/WebKit/Platform/IPC/Connection.cpp
>index 9e27540315d6e968d922b7d46bc7622991b817d3..c230e9f10492c6479b2f164b3851fe698a172561 100644
>--- a/Source/WebKit/Platform/IPC/Connection.cpp
>+++ b/Source/WebKit/Platform/IPC/Connection.cpp
>@@ -697,8 +697,13 @@ void Connection::processIncomingMessage(std::unique_ptr<Decoder> message)
>     // Check if this is a sync message or if it's a message that should be dispatched even when waiting for
>     // a sync reply. If it is, and we're waiting for a sync reply this message needs to be dispatched.
>     // If we don't we'll end up with a deadlock where both sync message senders are stuck waiting for a reply.
>-    if (SyncMessageState::singleton().processIncomingMessage(*this, message))
>-        return;
>+    {
>+        LockHolder locker(m_syncReplyStateMutex);
>+        // If the m_onlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage flag is set, then our sync messages will not be processed by the destination process while it is waiting for a sync IPC reply.
>+        // As a result, we need to process incoming sync messages early here, even if we're not waiting for a sync IPC reply in order to prevent deadlocks.
>+        if ((m_onlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage || !m_pendingSyncReplies.isEmpty()) && SyncMessageState::singleton().processIncomingMessage(*this, message))
>+            return;
>+    }
> 
>     // Check if we're waiting for this message.
>     {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f12ad2e20fdd99412885ecb000c21bee346df17e..666c47ddb0f3ecec9d809511ecb16377fa915a5f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-06-22  Chris Dumez  <cdumez@apple.com>
>+
>+        Do not do early processing of incoming sync IPC unless we're waiting for a sync IPC reply
>+        https://bugs.webkit.org/show_bug.cgi?id=186941
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add layout test coverage.
>+
>+        * http/tests/misc/blob-size-expected.txt: Added.
>+        * http/tests/misc/blob-size.html: Added.
>+
> 2018-06-22  Chris Dumez  <cdumez@apple.com>
> 
>         performance-api/performance-observer-no-document-leak.html is flaky
>diff --git a/LayoutTests/http/tests/misc/blob-size-expected.txt b/LayoutTests/http/tests/misc/blob-size-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..99e006bc2120aead61a77f01f6dfe50338cbff5e
>--- /dev/null
>+++ b/LayoutTests/http/tests/misc/blob-size-expected.txt
>@@ -0,0 +1,10 @@
>+Tests that constructing a Blob and querying its size right away is not flaky
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS Getting the Blob size is not flaky
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/http/tests/misc/blob-size.html b/LayoutTests/http/tests/misc/blob-size.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..92d8b0c467c96880dd8c225d575e7f5eef3de85c
>--- /dev/null
>+++ b/LayoutTests/http/tests/misc/blob-size.html
>@@ -0,0 +1,26 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="/js-test-resources/js-test.js"></script>
>+<script>
>+description("Tests that constructing a Blob and querying its size right away is not flaky");
>+
>+function runTest() {
>+    for (let i = 0; i < 50; i++) {
>+        for (let j = 0; j < 200; j++) {
>+            fetch('/resources/square100.png', { cache: 'no-store' }).then(function(res) { });
>+        }
>+        for (let k = 0; k < 5; k++) {
>+        if ((new Blob(["aaaa"])).size != 4) {
>+            testFailed("Getting the Blob size is flaky");
>+            return;
>+        }
>+        }
>+    }
>+    testPassed("Getting the Blob size is not flaky");
>+}
>+
>+runTest();
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186941: 343366 | 343367 | 343376 | 343384 | 343421 | 350429 | 350467 | 350643