Bug 228923 - Blob URL changes after loading it
Summary: Blob URL changes after loading it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 228924
  Show dependency treegraph
 
Reported: 2021-08-09 13:55 PDT by Chris Dumez
Modified: 2022-03-22 02:49 PDT (History)
13 users (show)

See Also:


Attachments
Patch (53.62 KB, patch)
2021-08-09 14:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.74 KB, patch)
2021-08-09 14:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.55 KB, patch)
2021-08-09 15:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (54.97 KB, patch)
2021-08-09 15:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.94 KB, patch)
2021-08-09 18:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-08-09 13:55:45 PDT
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.
Comment 1 Chris Dumez 2021-08-09 14:01:00 PDT
Created attachment 435210 [details]
Patch
Comment 2 Geoffrey Garen 2021-08-09 14:14:06 PDT
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
Comment 3 Chris Dumez 2021-08-09 14:17:20 PDT
(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?
Comment 4 Chris Dumez 2021-08-09 14:22:43 PDT
Created attachment 435212 [details]
Patch
Comment 5 Chris Dumez 2021-08-09 15:25:50 PDT
Created attachment 435215 [details]
Patch
Comment 6 Geoffrey Garen 2021-08-09 15:50:30 PDT
> 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.
Comment 7 Chris Dumez 2021-08-09 15:59:49 PDT
Created attachment 435220 [details]
Patch
Comment 8 Chris Dumez 2021-08-09 18:54:06 PDT
Created attachment 435231 [details]
Patch
Comment 9 EWS 2021-08-09 20:33:12 PDT
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].
Comment 10 Radar WebKit Bug Importer 2021-08-09 20:40:34 PDT
<rdar://problem/81725874>
Comment 11 Antoine Quint 2022-03-22 02:49:36 PDT
This caused a regression in bug 236069.