Bug 228923 – Blob URL changes after loading it

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Blob URL changes after loading it. This is because we use URLWithBlobURLLifetimeExtension to extend the lifetime of the Blob URL during the load and URLWithBlobURLLifetimeExtension ends up creating a new public Blob URL.
This is Web-observable so we need a way to extend the lifetime of a Blob URL without actually generating a new Blob URL.

[Chris Dumez]

Created attachment 435210 [details]
Patch

[Geoffrey Garen]

Comment on attachment 435210 [details]
Patch

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

r=me

> Source/WebCore/fileapi/ThreadableBlobRegistry.cpp:175
> +    if (isMainThread()) {
> +        blobRegistry().registerBlobURLHandle(url);
> +        return;
> +    }
> +    callOnMainThread([url = url.isolatedCopy()] {
> +        blobRegistry().registerBlobURLHandle(url);
> +    });

Can we use ensureOnMainThread here?

> Source/WebCore/fileapi/ThreadableBlobRegistry.cpp:186
> +    if (isMainThread()) {
> +        blobRegistry().unregisterBlobURLHandle(url);
> +        return;
> +    }
> +    callOnMainThread([url = url.isolatedCopy()] {
> +        blobRegistry().unregisterBlobURLHandle(url);
> +    });

Ditto

[Chris Dumez]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 435210 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435210&action=review
> 
> r=me
> 
> > Source/WebCore/fileapi/ThreadableBlobRegistry.cpp:175
> > +    if (isMainThread()) {
> > +        blobRegistry().registerBlobURLHandle(url);
> > +        return;
> > +    }
> > +    callOnMainThread([url = url.isolatedCopy()] {
> > +        blobRegistry().registerBlobURLHandle(url);
> > +    });
> 
> Can we use ensureOnMainThread here?
> 
> > Source/WebCore/fileapi/ThreadableBlobRegistry.cpp:186
> > +    if (isMainThread()) {
> > +        blobRegistry().unregisterBlobURLHandle(url);
> > +        return;
> > +    }
> > +    callOnMainThread([url = url.isolatedCopy()] {
> > +        blobRegistry().unregisterBlobURLHandle(url);
> > +    });
> 
> Ditto

I considered it. The code would become more concise but then we would do an extra isolatedCopy() in the common case where we're already on the main thread.

What do you think? Still worth it?

[Chris Dumez]

Created attachment 435212 [details]
Patch

[Chris Dumez]

Created attachment 435215 [details]
Patch

[Geoffrey Garen]

> I considered it. The code would become more concise but then we would do an
> extra isolatedCopy() in the common case where we're already on the main
> thread.
> 
> What do you think? Still worth it?

Interesting!

Probably not optimizable, even if we inlined the ensureOnMainThread, since isolatedCopy() has side effects.

Almost makes me want to use a macro. But macros are bad. :(

I guess it's a tradeoff with no obvious winner; I'll leave it up to you.

[Chris Dumez]

Created attachment 435220 [details]
Patch

[Chris Dumez]

Created attachment 435231 [details]
Patch

[EWS]

Committed r280824 (240387@main): <https://commits.webkit.org/240387@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/81725874>

[Antoine Quint]

This caused a regression in bug 236069.