Closed Bug 1271345 Opened 8 years ago Closed 8 years ago

WebExtensions: chrome.downloads.download will not download blob created in background script

Categories

(WebExtensions :: Untriaged, defect, P2)

45 Branch
defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: dw-dev, Assigned: aswan)

References

Details

(Whiteboard: [downloads] triaged)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

Steps to reproduce:

I am in the final stage of migrating my Print Edit add-on to use the new WebExtensions APIs with the objective of having the same source code for Firefox and Chrome versions.

I have managed to work around most of the incompabibilities between Firefox and Chrome, but there is one show-stopper problem remaining: chrome.downloads.download will not download a blob created in the background script.

Here is a simplified version of the source code:

chrome.runtime.onMessage.addListener(
function(message,sender,sendResponse)
{
    chrome.tabs.query({ lastFocusedWindow: true, active: true },
    function(tabs)
    {
        var blob,objectURL;
        
        blob = new Blob( [ (new Uint8Array([ 0xEF,0xBB,0xBF ])), message.textstring ], { type : "text/html" });
        
        objectURL = window.URL.createObjectURL(blob);
        
        chrome.downloads.download({ url: objectURL, filename: tabs[0].title + ".html", saveAs: true },
        function(downloadItemId)
        {
        });
    });
});

Note: message.textstring is a long text string that is constructed in a content script.


Actual results:

When this code is executed in Firefox, it fails with the following error message:

Type error for parameter options (Error processing url: Error: Access denied for URL blob:moz-extension://52ad408b-a345-4339-b1b7-f90eec9aed20/1b2921be-704c-49fe-9fc4-82f11b303bb5) for downloads.download.

When this code is executed in Chrome, it works correctly and downloads the blob into a file.




Expected results:

The blob should be downloaded into a file.

There should not be any cross-origin issues since the blob is both created and downloaded in the background script.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Have the same issue. Currently this is blocking my extension.

Simple example:

var blob = new Blob(['text'], {
    type: 'text/plain'
});
var url = URL.createObjectURL(blob);
browser.downloads.download({
    url: url
});

Here is my sample extension to test this bug: https://github.com/egoroof/sample-chrome-extension/tree/firefox
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: [downloads] triaged
I have the same problem

(In reply to mail from comment #2)
> Have the same issue. Currently this is blocking my extension.
> 
> Simple example:
> 
> var blob = new Blob(['text'], {
>     type: 'text/plain'
> });
> var url = URL.createObjectURL(blob);
> browser.downloads.download({
>     url: url
> });
> 
> Here is my sample extension to test this bug:
> https://github.com/egoroof/sample-chrome-extension/tree/firefox
I have this problem in one from my extension.
Calling download() on a blob URL was failing in schema validation
since we weren't propagating the extension principal all the way
to the call to scriptSecurityManager.checkLoadURI...

Review commit: https://reviewboard.mozilla.org/r/56932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56932/
Attachment #8758772 - Flags: review?(kmaglione+bmo)
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Ever confirmed: true
Comment on attachment 8758772 [details]
MozReview Request: Bug 1271345 Fix brower.download.download() on blob: urls r?kmag

https://reviewboard.mozilla.org/r/56932/#review53722

::: toolkit/components/extensions/ext-downloads.js:429
(Diff revision 1)
> +            try {
> -            let uri = NetUtil.newURI(options.url).QueryInterface(Ci.nsIURL);
> +              let uri = NetUtil.newURI(options.url).QueryInterface(Ci.nsIURL);
> -            target = OS.Path.join(downloadsDir, uri.fileName);
> +              filename = uri.fileName;
> +            } catch (ex) {
> +              // A blob: url doesn't support nsIURL
> +              if (ex.result != Cr.NS_NOINTERFACE) {

let uri = NetUtil.newURI(options.url);
    if (uri instanceof Ci.nsIURL) {
      filename = uri.fileName;
    }

::: toolkit/components/extensions/ext-downloads.js:433
(Diff revision 1)
> +              // A blob: url doesn't support nsIURL
> +              if (ex.result != Cr.NS_NOINTERFACE) {
> +                throw ex;
> +              }
> +            }
> +            if (!filename || filename.length == 0) {

`filename.length == 0` implies `!filename`.

This could be written as:

    target = OS.Path.join(downloadsDir, filename || "download");

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html:54
(Diff revision 1)
>      Services.prefs.clearUserPref("browser.download.dir");
>    });
>  }
>  
>  function backgroundScript() {
> -  browser.test.onMessage.addListener(function(msg) {
> +  browser.test.onMessage.addListener(function(msg, ...args) {

Since you're not using `arguments` anymore, can you make this an arrow function?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html:61
(Diff revision 1)
> +      let options = args[0];
> +
> +      if (options.blobme) {
> +        let blob = new Blob(options.blobme);
> +        delete options.blobme;
> +        options.url = window.URL.createObjectURL(blob);

We should call `revokeObjectURL` after the download completes.
Attachment #8758772 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8758772 [details]
MozReview Request: Bug 1271345 Fix brower.download.download() on blob: urls r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56932/diff/1-2/
Comment on attachment 8758772 [details]
MozReview Request: Bug 1271345 Fix brower.download.download() on blob: urls r?kmag

https://reviewboard.mozilla.org/r/56932/#review53838

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html:69
(Diff revisions 1 - 2)
>        // download() throws on bad arguments, we can remove the extra
>        // promise when bug 1250223 is fixed.
>        return Promise.resolve().then(() => browser.downloads.download(options))
>               .then((id) => browser.test.sendMessage("download.done", {status: "success", id}))
> -             .catch(error => browser.test.sendMessage("download.done", {status: "error", errmsg: error.message}));
> +             .catch(error => browser.test.sendMessage("download.done", {status: "error", errmsg: error.message}))
> +             .finally(() => {

Promises don't have a `finally` method...
Attachment #8758772 - Flags: review+
https://reviewboard.mozilla.org/r/56932/#review53838

> Promises don't have a `finally` method...

whoops, i was having a bluebird flashback
Comment on attachment 8758772 [details]
MozReview Request: Bug 1271345 Fix brower.download.download() on blob: urls r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56932/diff/2-3/
Attachment #8758772 - Flags: review?(kmaglione+bmo)
Comment on attachment 8758772 [details]
MozReview Request: Bug 1271345 Fix brower.download.download() on blob: urls r?kmag

https://reviewboard.mozilla.org/r/56932/#review53864

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html:74
(Diff revisions 2 - 3)
> -             .catch(error => browser.test.sendMessage("download.done", {status: "error", errmsg: error.message}))
> -             .finally(() => {
> -               window.URL.revokeObjectURL(options.url);
> -             });
> +                      browser.test.sendMessage("download.done", {status: "success", id});
> +                    })
> +                    .catch(error => {
> +                      browser.test.sendMessage("download.done", {status: "error", errmsg: error.message});
> +                    });
> +    } else if (msg == "killTheBlob") {

Is this necessary? I'd expect this to work as long as the download has started by the time we revoke the blob URL.
Attachment #8758772 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/56932/#review53864

> Is this necessary? I'd expect this to work as long as the download has started by the time we revoke the blob URL.

Yeah, when we call `download()` there is some work involving asynchronous I/O to figure out the target path before we get around to actually starting the download (callers follow the progress and detect errors via onChanged).  If we revoke the blob right away when `download()` returns there is a race and DownloadCore can reference the revoked URL and throw.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/332438c70582
Fix brower.download.download() on blob: urls r=kmag
https://reviewboard.mozilla.org/r/56932/#review53864

> Yeah, when we call `download()` there is some work involving asynchronous I/O to figure out the target path before we get around to actually starting the download (callers follow the progress and detect errors via onChanged).  If we revoke the blob right away when `download()` returns there is a race and DownloadCore can reference the revoked URL and throw.

OK, we should probably fix that, then. I think it's reasonable to expect add-ons to be able to revoke the blob after `download()` resolves.
(In reply to Kris Maglione [:kmag] from comment #14)
> > Yeah, when we call `download()` there is some work involving asynchronous I/O to figure out the target path before we get around to actually starting the download (callers follow the progress and detect errors via onChanged).  If we revoke the blob right away when `download()` returns there is a race and DownloadCore can reference the revoked URL and throw.
> 
> OK, we should probably fix that, then. I think it's reasonable to expect
> add-ons to be able to revoke the blob after `download()` resolves.

Any thoughts on how to do that?
https://hg.mozilla.org/mozilla-central/rev/332438c70582
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to Andrew Swan [:aswan] from comment #15)
> (In reply to Kris Maglione [:kmag] from comment #14)
> > OK, we should probably fix that, then. I think it's reasonable to expect
> > add-ons to be able to revoke the blob after `download()` resolves.
> 
> Any thoughts on how to do that?

That's a good question. Can you check if it works in Chrome? If not, we can probably leave it as-is for now. If it does, we may have to add an extra callback so we can tell when the channel has been successfully opened.
For more input: in my chrome extension I catch downloads.onChanged and when downloads.State is "interrupted" or "complete" only then I call URL.revokeObjectURL.

The issue is now working, thanks.

Also I've got an error that "saveAs" isn't supported by Firefox. Why not this mentioned in docs https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Downloads/download ?
Implementing saveAs is tracked in bug 1247791, I've also updated the docs.  I don't think anybody here has short-term plans to implement it, but I'd be happy to help you out with it if you want to contribute...
Thanks for suggestion. No :)
In short-term you can just ignore saveAs option and continue downloading with warning in console (not breaking it). This will improve compatibility with existing Chrome extensions.
Hello again. Some input after testing.

When I call:

var blob = new Blob(['text'], {
    type: 'text/plain'
});
var url = URL.createObjectURL(blob);
browser.downloads.download({
    url: url
});

On Firefox 49 (dev) and 50 (nightly) download complete successful but in console I get:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://app/modules/DownloadsCommon.jsm :: onDownloadChanged :: line 781"  data: no]

Stack:

onDownloadChanged() DownloadsCommon.jsm:781
this.DownloadList.prototype._notifyAllViews() DownloadList.jsm:213
DL_change() DownloadList.jsm:134
bound DL_change() self-hosted
D_notifyChange() DownloadCore.jsm:314
task_D_start() DownloadCore.jsm:555
InterpretGeneratorResume() self-hosted
next() self-hosted
TaskImpl_run() Task.jsm:319
bound TaskImpl_run() self-hosted
Handler.prototype.process() Promise-backend.js:937
this.PromiseWalker.walkerLoop() Promise-backend.js:816
bound walkerLoop() self-hosted
bound bound walkerLoop() self-hosted

It prevent some async tasks which I call in downloads.onChanged (but sync code in downloads.onChanged works). Should I open new issue for it?



The second problem - downloads.download failed with subdirectories in filename:

var blob = new Blob(['text'], {
    type: 'text/plain'
});
var url = URL.createObjectURL(blob);
browser.downloads.download({
    url: url,
    filename: 'one/file.txt'
});

This works fine in chrome and creates subdirectory "one" and file in it file.txt. It described here: https://developer.chrome.com/extensions/downloads#method-download. Is it bug or Firefox won't support it?
Can you please file a follow-up bug for the setPageAnnotation error? It looks like it shouldn't cause any real issues, since it's the last statement in a try-catch block, but it would still be good to fix.

A separate follow-up for file names in subdirectories would also be good, but I'm not sure we'll support it. And we'll need to decide how to handle platform-specific path separators.
About setPageAnnotation error - you are right, it's my fail. Not real problem.
My problem was that downloads.DownloadItem has "byExtensionName" and "byExtensionId" equal to undefined. I was using byExtensionName to check downloads and process only those who are from my ext. This is an issue but I don't use byExtensionName anymore so it isn't important for me.

Subdirectories issue is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1280044
Hi,

While downloading the blob content like below code,


var pdfLink ="<html><body onload=myFunction()>	<script>function myFunction(){  window.location.assign('https://local.xyz.com/record/)}</script></body></html>"

var blob = new Blob([pdfLink], {
				    type: 'data:text/html;charset=utf-8'
				});
				var finalUrl= URL.createObjectURL(blob);


In Background.Js downloading the blob like below,

browser.downloads.download({url: finalUrl,filename:"abc.html"},function(download) {
			
			browser.downloads.onChanged.addListener(function (download) {
				console.log( download.state);
			});
			
		});

I am getting  below error as,


Security Error: Content at moz-extension://27e3d23f-12a6-4893-9849-97a7991c3b1a/_generated_background_page.html may not load data from blob:https://local.xyz.com/05fc9018-291d-47c3-ad21-291765aad500.

and after that Below message 

Unchecked lastError value: Error: Type error for parameter options (Error processing url: Error: Access denied for URL blob:https://local.xyz.com/05fc9018-291d-47c3-ad21-291765aad500) for downloads.download.

"Couple of month back it was working fine."

Please Advise,

Thanks In Advance,

Thank you,
Yogesh
Anyone get a chance to look at the posted issue?
Flags: needinfo?(aswan)
Please open a new bug and attach a complete working example.
Flags: needinfo?(aswan) → needinfo?(dw-dev)
Not sure exactly what information is wanted from me.

After reported this bug, I implemented a workaround in the content script, which created a link with a 'download' attribute and simulated a click on the link.

I have not been back to test chrome.downloads.download with Print Edit WE since this bug was fixed.  The workaround works very well and is actually more efficient because there is no need to send a very long message to the background page.

I could test chrome.downloads.download with Print Edit WE if that would help.
Flags: needinfo?(dw-dev)
(In reply to dw-dev from comment #28)
> Not sure exactly what information is wanted from me.

Oops, I put in the wrong address for the needinfo, it was meant for the author of comment 25, but it looks like he subsequently opened bug 1369146.  Sorry for the confusion.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: