Revoking Blob URL after calling XMLHttpRequest::open() causes the XHR to fail

[Chris Dumez]

Revoking Blob URL after calling XMLHttpRequest::open() causes the XHR to fail. This doesn't match the behavior of other browsers and is causing WebKit to fail one of the subtests on:
- http://wpt.live/FileAPI/url/url-with-xhr.any.html

[Chris Dumez]

Created attachment 433182 [details]
Patch

[youenn fablet]

Comment on attachment 433182 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:121
> +    , m_url(makeUniqueRef<BlobURLLifeTimeExtender>())

We are doing an allocation for no good reason here.
I think it would look better if we had a separate BlobURLLifeTimeExtender that can happen to be null if URL is not a blob.
Also, we could probably try to nullify the blob life time extender as soon as we do not need it, maybe when clearing the request for instance.

> Source/WebCore/xml/XMLHttpRequest.h:219
> +    UniqueRef<BlobURLLifeTimeExtender> m_url;

It is a bit odd for BlobURLLifeTimeExtender to be called m_url.

[Chris Dumez]

(In reply to youenn fablet from comment #2)
> Comment on attachment 433182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433182&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:121
> > +    , m_url(makeUniqueRef<BlobURLLifeTimeExtender>())
> 
> We are doing an allocation for no good reason here.
> I think it would look better if we had a separate BlobURLLifeTimeExtender
> that can happen to be null if URL is not a blob.
> Also, we could probably try to nullify the blob life time extender as soon
> as we do not need it, maybe when clearing the request for instance.

I initially had a separate data member to extend the lifetime of the blob URL. However, this is error prone and we need to make sure we update it whenever m_url gets updated.

> > Source/WebCore/xml/XMLHttpRequest.h:219
> > +    UniqueRef<BlobURLLifeTimeExtender> m_url;
> 
> It is a bit odd for BlobURLLifeTimeExtender to be called m_url.

Sure, we can rename it but it does store a URL.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 433182 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433182&action=review
> > 
> > > Source/WebCore/xml/XMLHttpRequest.cpp:121
> > > +    , m_url(makeUniqueRef<BlobURLLifeTimeExtender>())
> > 
> > We are doing an allocation for no good reason here.
> > I think it would look better if we had a separate BlobURLLifeTimeExtender
> > that can happen to be null if URL is not a blob.
> > Also, we could probably try to nullify the blob life time extender as soon
> > as we do not need it, maybe when clearing the request for instance.
> 
> I initially had a separate data member to extend the lifetime of the blob
> URL. However, this is error prone and we need to make sure we update it
> whenever m_url gets updated.
> 
> > > Source/WebCore/xml/XMLHttpRequest.h:219
> > > +    UniqueRef<BlobURLLifeTimeExtender> m_url;
> > 
> > It is a bit odd for BlobURLLifeTimeExtender to be called m_url.
> 
> Sure, we can rename it but it does store a URL.

Maybe if I rename BlobURLLifeTimeExtender to URLWithBlobURLLifetimeExtension and add an assignment operator, then it will be clearer?

[Chris Dumez]

Created attachment 433239 [details]
Patch

[Chris Dumez]

Created attachment 433240 [details]
Patch

[EWS]

Committed r279881 (239633@main): <https://commits.webkit.org/239633@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/80531744>