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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Hutchison]

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.

[Radar WebKit Bug Importer]

<rdar://problem/80922502>

[Eric Hutchison]

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

[Chris Dumez]

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]

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]

Created attachment 434561 [details]
Patch

[Alex Christensen]

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]

(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]

(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 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 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]

Created attachment 434662 [details]
Patch

[Chris Dumez]

Comment on attachment 434662 [details]
Patch

Looks like I messed up something in this last iteration :/

[Chris Dumez]

Created attachment 434675 [details]
Patch

[EWS]

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].