1282407 - revokeObjectURL breaks blob download with download attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Patrick Westerhoff]

When using URL.revokeObjectURL directly after triggering a blob download via click() on a link element, then the download does not appear. Removing the revokeObjectURL will make it work.

See this example:


  (function () {
    let blob = new Blob(['test'], { type: 'text/plain' });
    let url = URL.createObjectURL(blob);

    let link = document.createElement('a');
    link.href = url;
    link.download = 'example.txt';
    document.body.appendChild(link);
    link.click();

    //URL.revokeObjectURL(url);
  })();


If you uncomment the revokeObjectURL line, then the example stops triggering a download.

It does when when you delay the execution of it using a zero-timeout, it does not work however when using an instantly fulfilled promise.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Hmm, yeah, internally we should keep object url working a bit longer, but not let new use of it to succeed after revokeObjectURL.

Comment 2

[Andrea Marchesini [:baku]]

I don't think this is a good idea: this behavior would be detectable and it's not covered by the spec.

Comment 3

[Andrew Overholt [:overholt]]

Do we need to change the spec? Anne, WDYT?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I'm pretty sure we're just having a bug here because we do some operation async during download and the spec doesn't have such async task. So we should keep the object url internally alive longer.
Looking at the spec...
"When an a or area element's activation behaviour is invoked, the user agent may allow the user to indicate a preference regarding whether the hyperlink is to be used for navigation or whether the resource it specifies is to be downloaded."
So, in this case downloaded, then Gecko artificially prevents that download because we try to use the object url asynchronously after 'activation behavior'.

Comment 5

[Anne (:annevk)]

The way the standards are intended to be written, parsing URLs happens synchronously, always, and that results in the resulting URL record getting a copy of the object in the blob store. At that point revocation doesn't matter, since <a> holds a copy in its associated URL record.

Comment 6

[Andrew Overholt [:overholt]]

Olli, can you clarify for baku what needs to be done?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I wonder how we should implement this.
Depends on where this fails. My guess is that this fails somewhere under InternalLoad or InternalLoadEvent in docshell, and there we pass nsIURI object. Could nsIURI implementation enforce object URL to be internally alive, but it would be revoked from JS point of view.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Looks like this has been reported to stackoverflow last year.
http://stackoverflow.com/questions/28178553/firefox-is-not-working-proper-with-url-revokeobjecturl

Comment 9

[Andrea Marchesini [:baku]]

Attachment: revoke_blobURL.patch

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8770959 [details] [diff] [review]
revoke_blobURL.patch

It doesn't work in e10s.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: revoke_blobURL.patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8770981 [details] [diff] [review]
revoke_blobURL.patch

>+  nsCOMPtr<nsISupports> tmp;
>+  MOZ_ALWAYS_SUCCEEDS(uriBlobImpl->GetBlobImpl(getter_AddRefs(tmp)));
>+  RefPtr<BlobImpl> blobImpl = static_cast<BlobImpl*>(tmp.get());
Why static_cast here...

>   DataInfo* info = GetDataInfo(spec);
> 
>-  if (!info) {
>-    return NS_ERROR_DOM_BAD_URI;
>-  }
>-
>-  nsCOMPtr<BlobImpl> blob = do_QueryInterface(info->mObject);
When you actually can QI. So, I think I'd prefer QI when it is possible. A bit safer in general.



Please explain the changes to Read and Write.
And BlobImpl isn't nsISerializable, so whote does NS_WriteOptionalCompoundObject even work?

It isn't clear to me why we even support serialization of bloburls. sicking or bz might know.


We must have a test for this.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: revoke_blobURL.patch

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8771250 [details] [diff] [review]
revoke_blobURL.patch

We need that testcase ;)
Separate patch for that is fine.

Comment 15

[Andrea Marchesini [:baku]]

> We need that testcase ;)
> Separate patch for that is fine.

I don't know how to write a test that detect a download.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Modify the tests which were added in bug 676619 ?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b20019c22d
Implement nsIURIWithBlobImpl to support blobURL after revoking them, r=smaug

Comment 18

[Andrea Marchesini [:baku]]

Attachment: revoke_test.patch

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8771962 [details] [diff] [review]
revoke_test.patch

You never remove the listener you add to Services.wm. Doesn't this end up breaking other tests which run after this one?

Comment 20

[Andrea Marchesini [:baku]]

Attachment: revoke_test.patch

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8771970 [details] [diff] [review]
revoke_test.patch

Not sure why you need download_after_revoke_page.html. Couldn't you use random data: url or even about:blank. But either way.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1b20019c22d

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45312f91ab91
Test for nsIURIWithBlobImpl, r=smaug

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45312f91ab91

Comment 25

[Jukka Jylänki]

The commit from this bug seems to have regressed releasing blobs from memory via revokeObjectURL(). Marked that down as https://bugzilla.mozilla.org/show_bug.cgi?id=1307791.

Comment 26

[Jukka Jylänki]

Does the spec currently say that the test case (with the revokeObjectURL() uncommented) should work?

In the code patterns we have worked with create/revokeObjectURL(), the understanding has been that only after the onload event fires, it is safe to revokeObjectURL(). To me it feels that the code pattern from the test case should not work?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Yes, per spec it should work. See comments 4 and 5.