Attachment 426076 Details for Bug 224566 – Patch

[patch] Patch

bug-224566-20210414230824.patch (text/plain), 10.59 KB, created by Chris Dumez on 2021-04-14 23:08:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2021-04-14 23:08:25 PDT

Size: 10.59 KB

patch

obsolete

>Subversion Revision: 276000
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 05e3a8b5bd0be48313199b1afcc1bdb6954cf024..9c879c25c41aa742f803822414d8d23f3979fd48 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,38 @@
>+2021-04-14  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION(r275921-r275922): [ wk2 ] http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html is flakey crashing
>+        https://bugs.webkit.org/show_bug.cgi?id=224566
>+        <rdar://problem/76657007>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        r275921-r275922 changed the timing of when the WebProcess/RemoteRenderingBackendProxy sends the GPUConnectionToWebProcess::CreateRenderingBackend
>+        IPC to the GPUProcess. This IPC used to get sent as soon as the RemoteRenderingBackendProxy was constructed. However, I delayed sending it until
>+        the RemoteRenderingBackendProxy actually needs to create an ImageBuffer to avoid launching the GPUProcess prematurely.
>+
>+        Unfortunately, this exposed a pre-existing synchronization issue with regards to the RemoteRenderingBackend IPC. In particular, the
>+        GPUConnectionToWebProcess::CreateRenderingBackend was getting sent asynchronously and processed by the GPUProcess on the main thread. This would
>+        cause the creation of the RemoteRenderingBackend object on the GPUProcess side, which would then register itself as a WorkQueueMessageReceiver.
>+        The issue is that the WebProcess was sending the RemoteRenderingBackend IPC so quickly after the CreateRenderingBackend IPC that when the IPC is
>+        received (on the IPC thread), the RemoteRenderingBackend has not had a change to register itself as a WorkQueueMessageReceiver yet. As a result,
>+        some of the early IPC was getting dispatched to the main thread and later IPC would get sent to the RemoteRenderingBackend WorkQueue. This was
>+        causing RemoteRenderingBackend to get processed out of order.
>+
>+        To address the synchronization issue for now, I made the GPUConnectionToWebProcess::CreateRenderingBackend synchronous. This may not be the
>+        best design long term but it is an easy and safe way to resolve this for now. It makes sure the WebProcess cannot send RemoteRenderingBackend
>+        IPC until after the GPUProcess has created the RemoteRenderingBackend and the RemoteRenderingBackend object has registered itself as a
>+        WorkQueueMessageReceiver. We probably want to follow-up when we come up with a better design. For now though, I have verified locally that this
>+        fixes the out of order IPC messaging AND the flaky crashes on these canvas tests.
>+
>+        No new tests, unskipped existing tests that are no longer flaky.
>+
>+        * GPUProcess/GPUConnectionToWebProcess.cpp:
>+        (WebKit::GPUConnectionToWebProcess::createRenderingBackend):
>+        * GPUProcess/GPUConnectionToWebProcess.h:
>+        * GPUProcess/GPUConnectionToWebProcess.messages.in:
>+        * WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
>+        (WebKit::RemoteRenderingBackendProxy::ensureGPUProcessConnection):
>+
> 2021-04-14  Basuke Suzuki  <basuke.suzuki@sony.com>
> 
>         Fix WK_EXPORT macro for declspec compilers
>diff --git a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp
>index 441ab1c6e76beb17e6551bfb497298d675436924..16d11c92b3f20cae1e00b0258a11a637b3a21fd6 100644
>--- a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp
>+++ b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp
>@@ -362,12 +362,13 @@ RemoteImageDecoderAVFProxy& GPUConnectionToWebProcess::imageDecoderAVFProxy()
> }
> #endif
> 
>-void GPUConnectionToWebProcess::createRenderingBackend(RemoteRenderingBackendCreationParameters&& creationParameters)
>+void GPUConnectionToWebProcess::createRenderingBackend(RemoteRenderingBackendCreationParameters&& creationParameters, CompletionHandler<void()>&& completionHandler)
> {
>     auto addResult = m_remoteRenderingBackendMap.ensure(creationParameters.identifier, [&]() {
>         return IPC::ScopedActiveMessageReceiveQueue { RemoteRenderingBackend::create(*this, WTFMove(creationParameters)) };
>     });
>     ASSERT_UNUSED(addResult, addResult.isNewEntry);
>+    completionHandler();
> }
> 
> void GPUConnectionToWebProcess::releaseRenderingBackend(RenderingBackendIdentifier renderingBackendIdentifier)
>diff --git a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h
>index f2dd9502abe36a00c71e2876360b8b2cef04f89f..aeaa31300ecde5a1534b4cd9498f495f3353ba6b 100644
>--- a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h
>+++ b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h
>@@ -161,7 +161,7 @@ private:
> #endif
> #endif
> 
>-    void createRenderingBackend(RemoteRenderingBackendCreationParameters&&);
>+    void createRenderingBackend(RemoteRenderingBackendCreationParameters&&, CompletionHandler<void()>&&);
>     void releaseRenderingBackend(RenderingBackendIdentifier);
> 
> #if ENABLE(WEBGL)
>diff --git a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in
>index 774558e107537a821f160ed33c6f959f70fe8f3a..8d1daf86ddc24fa8ceb92b0e47781a6ce28b9263 100644
>--- a/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in
>+++ b/Source/WebKit/GPUProcess/GPUConnectionToWebProcess.messages.in
>@@ -23,7 +23,7 @@
> #if ENABLE(GPU_PROCESS)
> 
> messages -> GPUConnectionToWebProcess WantsDispatchMessage {
>-    void CreateRenderingBackend(struct WebKit::RemoteRenderingBackendCreationParameters creationParameters)
>+    void CreateRenderingBackend(struct WebKit::RemoteRenderingBackendCreationParameters creationParameters) -> () Synchronous
>     void ReleaseRenderingBackend(WebKit::RenderingBackendIdentifier renderingBackendIdentifier)
> #if ENABLE(WEBGL)
>     void CreateGraphicsContextGL(struct WebCore::GraphicsContextGLAttributes attributes, WebKit::GraphicsContextGLIdentifier graphicsContextGLIdentifier, WebKit::RenderingBackendIdentifier renderingBackendIdentifier, IPC::StreamConnectionBuffer stream)
>diff --git a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>index d0dca63d2d983d722b5a912a7b553412020952a1..5dc42a63221b15cf4784a6e6a7c076d8eae1d5a7 100644
>--- a/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>+++ b/Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp
>@@ -75,7 +75,10 @@ GPUProcessConnection& RemoteRenderingBackendProxy::ensureGPUProcessConnection()
>         auto& gpuProcessConnection = WebProcess::singleton().ensureGPUProcessConnection();
>         gpuProcessConnection.addClient(*this);
>         gpuProcessConnection.messageReceiverMap().addMessageReceiver(Messages::RemoteRenderingBackendProxy::messageReceiverName(), renderingBackendIdentifier().toUInt64(), *this);
>-        gpuProcessConnection.connection().send(Messages::GPUConnectionToWebProcess::CreateRenderingBackend(m_parameters), 0, IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply);
>+        // This message is synchronous to ensure that the RemoteRenderingBackend has been created and has registered itself as a WorkQueueMessageReceiver before we send it IPC.
>+        // Without this synchronization, some IPC messages may get received by the GPUProcess before the RemoteRenderingBackend has registered itself as a WorkQueueMessageReceiver
>+        // and IPC may get processed out of order.
>+        gpuProcessConnection.connection().sendSync(Messages::GPUConnectionToWebProcess::CreateRenderingBackend(m_parameters), Messages::GPUConnectionToWebProcess::CreateRenderingBackend::Reply(), 0);
>         m_gpuProcessConnection = makeWeakPtr(gpuProcessConnection);
>     }
>     return *m_gpuProcessConnection;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ad02a511980b0f0561801197d1dccbb562ac780a..e1885741842321fddf757d2eb2a92bd876f78b8a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2021-04-14  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION(r275921-r275922): [ wk2 ] http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html is flakey crashing
>+        https://bugs.webkit.org/show_bug.cgi?id=224566
>+        <rdar://problem/76657007>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Unskip layout tests that should no longer be flaky.
>+
>+        * platform/ios-simulator-wk2/TestExpectations:
>+        * platform/mac-wk2/TestExpectations:
>+
> 2021-04-14  Diego Pino Garcia  <dpino@igalia.com>
> 
>         [GTK][WPE] Unreviewed test gardening. Add missing images after r275996.
>diff --git a/LayoutTests/platform/ios-simulator-wk2/TestExpectations b/LayoutTests/platform/ios-simulator-wk2/TestExpectations
>index a61af65ed237fdd5396fd970c84c452306873c87..b1822af6b160bd55e0f141d16f4f427a60a1f9e4 100644
>--- a/LayoutTests/platform/ios-simulator-wk2/TestExpectations
>+++ b/LayoutTests/platform/ios-simulator-wk2/TestExpectations
>@@ -169,7 +169,3 @@ webkit.org/b/224121 css2.1/20110323/replaced-intrinsic-ratio-001.htm [ Pass Fail
> webrtc/peerconnection-new-candidate-page-cache.html [ Pass Crash ]
> http/tests/navigation/page-cache-mediastream.html [ Pass Crash ]
> 
>-# webkit.org/b/224566 The following three tests are flaky crashing causing issues with EWS/commit queue.
>-http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html [ Pass Crash ]
>-imported/w3c/web-platform-tests/webmessaging/with-ports/011.html [ Pass Crash ]
>-webgl/1.0.3/conformance/glsl/matrices/glsl-mat3-construction.html [ Pass Crash ]
>\ No newline at end of file
>diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations
>index e80e0bd9afe0fe149493d329934e5c469481697d..e8e9bd2daa0a123e0bdac18daecb7dd312e722e0 100644
>--- a/LayoutTests/platform/mac-wk2/TestExpectations
>+++ b/LayoutTests/platform/mac-wk2/TestExpectations
>@@ -1377,9 +1377,4 @@ webkit.org/b/224463 [ Debug arm64 ] webrtc/disable-encryption.html [ Pass Timeou
> webrtc/peerconnection-new-candidate-page-cache.html [ Pass Crash ]
> http/tests/navigation/page-cache-mediastream.html [ Pass Crash ]
> 
>-# webkit.org/b/224566 The following three tests are flaky crashing causing issues with EWS/commit queue.
>-http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html [ Pass Crash ]
>-imported/w3c/web-platform-tests/webmessaging/with-ports/011.html [ Pass Crash ]
>-webgl/1.0.3/conformance/glsl/matrices/glsl-mat3-construction.html [ Pass Crash ]
>-
>-webkit.org/b/221985 fast/mediastream/audio-track-enabled.html [ Pass Failure ]
>\ No newline at end of file
>+webkit.org/b/221985 fast/mediastream/audio-track-enabled.html [ Pass Failure ]

Flags:

ews-feeder: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 224566: 426076