Closed Bug 1282407 Opened 3 years ago Closed 3 years ago

revokeObjectURL breaks blob download with download attribute

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: PatrickWesterhoff, Assigned: baku)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Hmm, yeah, internally we should keep object url working a bit longer, but not let new use of it to succeed after revokeObjectURL.
Flags: needinfo?(amarchesini)
I don't think this is a good idea: this behavior would be detectable and it's not covered by the spec.
Flags: needinfo?(amarchesini)
Do we need to change the spec? Anne, WDYT?
Flags: needinfo?(annevk)
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'.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(annevk)
Olli, can you clarify for baku what needs to be done?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attached patch revoke_blobURL.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8770959 - Flags: review?(bugs)
Comment on attachment 8770959 [details] [diff] [review]
revoke_blobURL.patch

It doesn't work in e10s.
Attachment #8770959 - Flags: review?(bugs)
Attached patch revoke_blobURL.patch (obsolete) — Splinter Review
Attachment #8770959 - Attachment is obsolete: true
Attachment #8770981 - Flags: review?(bugs)
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.
Attachment #8770981 - Flags: review?(bugs) → review-
Attachment #8770981 - Attachment is obsolete: true
Attachment #8771250 - Flags: review?(bugs)
Comment on attachment 8771250 [details] [diff] [review]
revoke_blobURL.patch

We need that testcase ;)
Separate patch for that is fine.
Attachment #8771250 - Flags: review?(bugs) → review+
> We need that testcase ;)
> Separate patch for that is fine.

I don't know how to write a test that detect a download.
Flags: needinfo?(bugs)
Modify the tests which were added in bug 676619 ?
Flags: needinfo?(bugs)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b20019c22d
Implement nsIURIWithBlobImpl to support blobURL after revoking them, r=smaug
Attached patch revoke_test.patch (obsolete) — Splinter Review
Attachment #8771962 - Flags: review?(bugs)
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?
Attachment #8771962 - Flags: review?(bugs) → review-
Attachment #8771962 - Attachment is obsolete: true
Attachment #8771970 - Flags: review?(bugs)
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.
Attachment #8771970 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a1b20019c22d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Depends on: 1307791
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?
Yes, per spec it should work. See comments 4 and 5.
Depends on: CVE-2017-5415
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.