Bug 229501 – REGRESSION (r281516): imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https.html is failing

• Component: Page Loading
• Status: RESOLVED FIXED

imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https.html

Is a flaky failure on iOS-14-Simulator-WK2-Tests-EWS and macOS-Catalina-Release-WK2-Tests-EWS.

The flaky failure is also showing up in the open source directory: https://results.webkit.org/?suite=layout-tests&test=imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https.html

Result page: https://build.webkit.org/results/Apple-iPadOS-14-Simulator-Release-WK2-Tests/r281555%20(1898)/results.html

Diff:

--- /Volumes/Data/worker/ipados-simulator-14-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https-expected.txt
+++ /Volumes/Data/worker/ipados-simulator-14-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https-actual.txt
@@ -1,3 +1,5 @@
-
-FAIL IndexedDB: Attempting to serialize a SharedArrayBuffer should throw assert_true: The page is served with COOP and COEP, it should be cross-origin-isolated. expected true got false
-
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584

Looks like the flaky failure started at https://trac.webkit.org/changeset/281516/webkit.

[Radar WebKit Bug Importer]

<rdar://problem/82346152>

Updated test expectations https://trac.webkit.org/changeset/281561/webkit

[Chris Dumez]

(In reply to ayumi_kojima from comment #2)
> Updated test expectations https://trac.webkit.org/changeset/281561/webkit

Thanks.

[Alexey Proskuryakov]

The title says "flaky", but on most queues, it's a 100% failure.

*** Bug 229565 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 436548 [details]
WIP patch

[Chris Dumez]

Created attachment 436561 [details]
Patch

[Chris Dumez]

Created attachment 436564 [details]
Patch

Just FYI expectation is marked for imported/w3c/web-platform-tests/encoding/sharedarraybuffer.https.html here https://trac.webkit.org/changeset/281626/webkit

The bug is already duped.

[Alex Christensen]

Comment on attachment 436564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436564&action=review

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:639
> +    // In case of a COOP process-swap, the old process gets a didFailProvisionalLoadWithErrorForFrame delegate call. We want to ignore

Safari also uses didFailProvisionalLoadWithErrorForFrame.  Do we want to prevent this call from WebKit in the case of a COOP process swap?

[Chris Dumez]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 436564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436564&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:639
> > +    // In case of a COOP process-swap, the old process gets a didFailProvisionalLoadWithErrorForFrame delegate call. We want to ignore
> 
> Safari also uses didFailProvisionalLoadWithErrorForFrame.  Do we want to
> prevent this call from WebKit in the case of a COOP process swap?

I am not convinced we want to bypass this call at WebKit level. The old process starts a provisional load and this provisional load gets cancelled when we process-swap. I think calling didFailProvisionalLoadWithErrorForFrame() is correct behavior in the old process otherwise, the process would get a didStartProvisionalLoadForFrame call and then nothing else. Wouldn't it be bad too?

I think the issue is really in WebKitTestRunner's injected bundle because the logic there thinks the test failed even though it merely proceeded in a new process.

[Chris Dumez]

Comment on attachment 436564 [details]
Patch

Oh, looks like this is causing http/tests/contentextensions/block-everything-if-domain.html to fail. The test is expected to fail loading before committing any load so it triggers my new logic :/

[Chris Dumez]

Created attachment 436584 [details]
Patch

[Chris Dumez]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 436564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436564&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:639
> > +    // In case of a COOP process-swap, the old process gets a didFailProvisionalLoadWithErrorForFrame delegate call. We want to ignore
> 
> Safari also uses didFailProvisionalLoadWithErrorForFrame.  Do we want to
> prevent this call from WebKit in the case of a COOP process swap?

To be clear, I think we should call didFailProvisionalLoadWithErrorForFrame in the injected bundle in the old process (since it got a didStartProvisionalLoadForFrame and since the load gets cancelled in that process). However, we should definitely not call didFailProvisionalLoadWithErrorForFrame() at UIProcess level. We may have a bug there. I will follow-up to add a test and fix if we indeed have that bug.

[Chris Dumez]

Comment on attachment 436584 [details]
Patch

EWS is happy

[EWS]

Committed r281699 (241049@main): <https://commits.webkit.org/241049@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436584 [details].