1462926 - browser.downloads.download Popup to background script download fails, because of too early revoking

Status: RESOLVED INVALID

(Firefox :: Untriaged, defect)

Description

[rugk]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180507144316

Steps to reproduce:

WebExtension
A message with an svg string (svgString) in is sent from my popup to my background script:

            const file = new File([svgString], "qrcode.svg", {type: "image/svg+xml;charset=utf-8"});

browser.runtime.sendMessage({
                type: "saveFileAs",
                file: file,
                filename: "qrcode.svg",
            }).then(() => {
                Logger.logInfo("SVG image saved on disk", svgElem, svgString);
            }).catch(() => {
                Logger.logError("Could not save SVG image saved on disk", svgElem, svgString);
                MessageHandler.showError("errorDownloadingFile", true);
            });

In that background script, it is then downloaded with saveAs set to true:

            const objectUrl = URL.createObjectURL(request.file);



            return browser.downloads.download({
                url: objectUrl,
                filename: request.filename,
                saveAs: true
            }).then(() => {
                // send response
                sendResponse();
            }).finally(() => {
                // clean-up
                URL.revokeObjectURL(objectUrl);
            });





Actual results:

It cannot download the file, i.e. an orange indicator appears in the download UI.

It does, however, correctly…
* display the "saveAs" dialog
* run the then and finally block, after the user selected a download location (i.e. no error in Promise handling as it seems)
* work, if I do not call URL.revokeObjectURL(objectUrl); or call it with a timeout (i.e. it seems it6 is too early revoked or so)

So my workaround is:

setTimeout(() => {
                    console.log("objectUrl revoked:", objectUrl);
                    URL.revokeObjectURL(objectUrl);
                }, 5000);

It does also not work, if I set saveAs to "false", so it does not depend on this setting.

However, there is a funny error behaviour, which **is the same when the download works, i.e. I remove the revokeObjectURL line** in Firefox 60, because it shown two errors:

When "saveAs" popup is triggered:

19:38:29.752 [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: resource://gre/modules/ExtensionUtils.jsm :: sendAsyncMessage :: line 533"  data: no]  (unbekannt)
	sendAsyncMessage resource://gre/modules/ExtensionUtils.jsm:533:51
	_handleMessage/deferred.promise< resource://gre/modules/MessageChannel.jsm:984:9

After "saveAs" popup is closed:

19:38:33.176 [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/DownloadHistory.jsm :: updateMetaData :: line 130"  data: no]  (unbekannt)
	updateMetaData resource://gre/modules/DownloadHistory.jsm:130:7
	onDownloadChanged resource:///modules/DownloadsCommon.jsm:755:9
	_notifyAllViews resource://gre/modules/DownloadList.jsm:189:28
	DL_change resource://gre/modules/DownloadList.jsm:105:5
	DL_change self-hosted:1018:17
	D_notifyChange resource://gre/modules/DownloadCore.jsm:269:9
	D_start/< resource://gre/modules/DownloadCore.jsm:519:11
	InterpretGeneratorResume self-hosted:1258:8
	next self-hosted:1213:9
	
Firefox 61 only shows one error instead:

 [Show/hide message details.] [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/DownloadHistory.jsm :: updateMetaData :: line 130"  data: no]
updateMetaData
resource://gre/modules/DownloadHistory.jsm:130:7
onDownloadChanged
resource:///modules/DownloadsCommon.jsm:753:9
_notifyAllViews
resource://gre/modules/DownloadList.jsm:189:28
DL_change
resource://gre/modules/DownloadList.jsm:105:5
DL_change self-hosted:1026:17 D_notifyChange
resource://gre/modules/DownloadCore.jsm:269:9
D_start/<
resource://gre/modules/DownloadCore.jsm:524:11
InterpretGeneratorResume self-hosted:1266:8 next self-hosted:1221:9



Expected results:

Download should work.

BTW I implemented this all just because of bug 1461134.

GNOME, Fedora 28

Comment 1

[Tomislav Jovanovic :zombie]

You don't seem to be sending the `svgString`, but a `File` with the content of `svgString`.  Try sending the string and creating the file in the background page.

Comment 2

[rugk]

Indeed,I think "File" is just a container format. Of course, I create the object URL in the background page – that needs to be done there.
However, I just changed it to send the string and the result is the same. The issue is still that URL.revokeObjectURL(…) is still too early executed in the finally block as browser.downloads.download seems to resolve the Promise everything too early. (at least that's my guess what is wrong here)

Details:

---popup---


const svgString = (new XMLSerializer()).serializeToString(svgElem);

            // const file = new File([svgString], "qrcode.svg", {type: "image/svg+xml;charset=utf-8"});

            browser.runtime.sendMessage({
                type: "saveFileAs",
                file: svgString,
                filename: "qrcode.svg",
            }).then(() => {
                Logger.logInfo("SVG image saved on disk", svgElem, svgString);
            }).catch(() => {
                Logger.logError("Could not save SVG image saved on disk", svgElem, svgString);
                MessageHandler.showError("errorDownloadingFile", true);
            });

---background.js---

function handleMessages(request, sender, sendResponse) {

const file = new File([request.file], "qrcode.svg", {type: "image/svg+xml;charset=utf-8"});

            const objectUrl = URL.createObjectURL(file);

            return browser.downloads.download({
                url: objectUrl,
                filename: request.filename,
                saveAs: true
            }).then(() => {
                // send response
                sendResponse();
            }).finally(() => {
                // clean-up
                // workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=1462926
                // setTimeout(() => {
                //     console.log("objectUrl revoked:", objectUrl);
                    URL.revokeObjectURL(objectUrl);
                // }, 5000);
            });

}

Comment 3

[rugk]

So if you want to see the real example, here it is:

https://github.com/rugk/offline-qr-code/commit/3eb07e7e08db0c179a21a6ad3a4517a678c2504d

Comment 4

[Tomislav Jovanovic :zombie]

Ah, you probably need to delay revoking the URL until the download is complete, by listening for downloads.onChange event.

Comment 5

[rugk]

But this is not the correct behaviour, is not it?

When the download thing fulfills the promise, I expect I can revoke the object URL as it has been "done".

So either:
* document that (as it then affects all downloads)
* or change the behaviour

Comment 6

[rugk]

Ah, okay, seems I can only revoke the URI when the download is really completed… okay.