Closed
Bug 1271345
Opened 9 years ago
Closed 8 years ago
WebExtensions: chrome.downloads.download will not download blob created in background script
Categories
(WebExtensions :: Untriaged, defect, P2)
Tracking
(firefox49 fixed)
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.
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
Updated•9 years ago
|
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: [downloads] triaged
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Ever confirmed: true
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/56932/#review53838
> Promises don't have a `finally` method...
whoops, i was having a bluebird flashback
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/332438c70582
Fix brower.download.download() on blob: urls r=kmag
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
(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?
Comment 16•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 17•8 years ago
|
||
(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.
Comment 18•8 years ago
|
||
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 ?
Assignee | ||
Comment 19•8 years ago
|
||
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...
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
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?
Comment 22•8 years ago
|
||
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.
Comment 23•8 years ago
|
||
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
Comment 25•8 years ago
|
||
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
Assignee | ||
Comment 27•7 years ago
|
||
Please open a new bug and attach a complete working example.
Flags: needinfo?(aswan) → needinfo?(dw-dev)
Reporter | ||
Comment 28•7 years ago
|
||
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)
Assignee | ||
Comment 29•7 years ago
|
||
(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.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•