Bug 228168

Summary:

imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html is a flaky failure since implementing BlobChannel

Product:

WebKit

Reporter:

Eric Hutchison <ehutchison>

Component:

New Bugs

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, alecflett, beidson, cdumez, darin, ehutchison, esprehn+autocc, ews-watchlist, ggaren, japhet, jsbell, kangil.han, sam, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=212276
https://bugs.webkit.org/show_bug.cgi?id=228923

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Eric Hutchison

Reported 2021-07-21 15:46:26 PDT

imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html is a flaky failure on macOS and iOS. History: https://results.webkit.org/?platform=mac&platform=ios&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fwebmessaging%2Fbroadcastchannel%2Fblobs.html Diff: --- /Volumes/Data/worker/bigsur-release-applesilicon-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs-expected.txt +++ /Volumes/Data/worker/bigsur-release-applesilicon-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs-actual.txt @@ -1,4 +1,4 @@ -PASS Blobs work on BroadcastChannel +FAIL Blobs work on BroadcastChannel assert_unreached: Reached unreachable code PASS Blobs work with workers on BroadcastChannel Initial failure on r280017. Reproduced on r280151 with run-webkit-tests -f --iterations 500 --exit-after-n-failures 2 imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html. Reproduced on r280017 with run-webkit-tests -f --iterations 500 --exit-after-n-failures 2 -1 imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html. Reproduced on r279971 with run-webkit-tests -f --iterations 500 --exit-after-n-failures 2 --clobber-old-results -1 imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html and run-webkit-tests -f --iterations 500 --exit-after-n-failures 2 --clobber-old-results imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html.

Attachments
Patch (49.40 KB, patch) 2021-07-29 13:02 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (50.93 KB, patch) 2021-07-30 14:28 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (51.01 KB, patch) 2021-07-30 16:54 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2021-07-21 15:46:59 PDT

<rdar://problem/80922502>

Eric Hutchison

Comment 2 2021-07-21 16:08:16 PDT

Test expectations for Mac and iOS updated on https://trac.webkit.org/changeset/280165/webkit.

Chris Dumez

Comment 3 2021-07-28 15:39:07 PDT

Seems to be a garbage collection issue, I can reproduce 100% with aggressive GC: Tools/Scripts/run-webkit-tests --debug imported/w3c/web-platform-tests/webmessaging/broadcastchannel/blobs.html --force --additional-env-var=__XPC_JSC_collectContinuously=true

Chris Dumez

Comment 4 2021-07-28 15:45:37 PDT

The test sends a Blob via BroadcastChannel.postMessage(). When we construct the SerializedScriptValue for the Blob, we only serialize the url of the Blob. If the blob gets deallocated by the time the message is received (and deserialization happens) then we're unable to lookup the blob from its URL.

Chris Dumez

Comment 5 2021-07-29 13:02:10 PDT

Created attachment 434561 [details] Patch

Alex Christensen

Comment 6 2021-07-30 10:30:01 PDT

Comment on attachment 434561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434561&action=review > Source/WebCore/ChangeLog:15 > + To address the issue, I introduced a Blob::Handle class that wraps the Blob's internal URL > + and keeps the Blob data alive as long as as someone is holding such handle. Previously, > + the Blob destructor would unregister the internal URL and the Blob data would go away. Should this replace extendBlobURLLifetimeIfNecessary?

Chris Dumez

Comment 7 2021-07-30 10:31:53 PDT

(In reply to Alex Christensen from comment #6) > Comment on attachment 434561 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434561&action=review > > > Source/WebCore/ChangeLog:15 > > + To address the issue, I introduced a Blob::Handle class that wraps the Blob's internal URL > > + and keeps the Blob data alive as long as as someone is holding such handle. Previously, > > + the Blob destructor would unregister the internal URL and the Blob data would go away. > > Should this replace extendBlobURLLifetimeIfNecessary? No, extendBlobURLLifetimeIfNecessary is about BlobURLs, which are public URLs. In this case, I don't have a public Blob URL, only a Blob (with its internal URL).

Chris Dumez

Comment 8 2021-07-30 10:40:13 PDT

(In reply to Chris Dumez from comment #7) > (In reply to Alex Christensen from comment #6) > > Comment on attachment 434561 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=434561&action=review > > > > > Source/WebCore/ChangeLog:15 > > > + To address the issue, I introduced a Blob::Handle class that wraps the Blob's internal URL > > > + and keeps the Blob data alive as long as as someone is holding such handle. Previously, > > > + the Blob destructor would unregister the internal URL and the Blob data would go away. > > > > Should this replace extendBlobURLLifetimeIfNecessary? > > No, extendBlobURLLifetimeIfNecessary is about BlobURLs, which are public > URLs. > > In this case, I don't have a public Blob URL, only a Blob (with its internal > URL). Trying to clarify: 1. URLWithBlobURLLifetimeExtension works only with public Blob URLs (which contains the security origin). It is suitable when you have a public Blob URL and want to extend its lifetime for something like a load or a navigation (which requires origin for security checks). Note that URLWithBlobURLLifetimeExtension actually makes a copy of the Blob data currently. 2. The newly introduced Blob::Handle keeps an internal blob URL valid and doesn't copy the actual Blob data. It's just a way to keep the blob data alive potentially longer than the Blob object itself. We don't have a public blob URL (no security origin).

Alex Christensen

Comment 9 2021-07-30 11:37:55 PDT

Comment on attachment 434561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434561&action=review > Source/WebCore/fileapi/Blob.cpp:88 > + static Ref<BlobInternalURL> create(URL&& url) We may as well make this a const URL& if we're just going to crossThreadCopy it. > Source/WebCore/fileapi/Blob.cpp:95 > + ThreadableBlobRegistry::unregisterBlobURL(m_internalURL); I don't think we should have this object unregister a URL that it didn't register. The same object should register and unregister the URLs. It's error prone otherwise. Is there a reason you didn't do this?

Chris Dumez

Comment 10 2021-07-30 11:53:58 PDT

Comment on attachment 434561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434561&action=review >> Source/WebCore/fileapi/Blob.cpp:88 >> + static Ref<BlobInternalURL> create(URL&& url) > > We may as well make this a const URL& if we're just going to crossThreadCopy it. Indeed. >> Source/WebCore/fileapi/Blob.cpp:95 >> + ThreadableBlobRegistry::unregisterBlobURL(m_internalURL); > > I don't think we should have this object unregister a URL that it didn't register. The same object should register and unregister the URLs. It's error prone otherwise. Is there a reason you didn't do this? I thought about this indeed. I agree it would be nicer. The reason I didn't do this is that when you register, you need more information besides the URL (data and type). Also, there are cases where we unregister but don't register ourselves (see the UninitializedContructor constructor, which is used by File subclass I believe). I'll take another look and see if I can pass more data and do the registration inside BlobInternalURL as I agree it would be a better design.

Chris Dumez

Comment 11 2021-07-30 14:28:37 PDT

Created attachment 434662 [details] Patch

Chris Dumez

Comment 12 2021-07-30 16:36:55 PDT

Comment on attachment 434662 [details] Patch Looks like I messed up something in this last iteration :/

Chris Dumez

Comment 13 2021-07-30 16:54:53 PDT

Created attachment 434675 [details] Patch

EWS

Comment 14 2021-08-02 11:47:01 PDT

Committed r280547 (240174@main): <https://commits.webkit.org/240174@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434675 [details].

Note You need to log in before you can comment on or make changes to this bug.