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.
<rdar://problem/80922502>
Test expectations for Mac and iOS updated on https://trac.webkit.org/changeset/280165/webkit.
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
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.
Created attachment 434561 [details] Patch
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?
(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).
(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).
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?
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.
Created attachment 434662 [details] Patch
Comment on attachment 434662 [details] Patch Looks like I messed up something in this last iteration :/
Created attachment 434675 [details] Patch
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].