1063217 - Support a PDF DownloadSaver

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Wesley Johnston (:wesj)]

Fennec's current PDF output adds files to the Download list. Its not very easy to do so with Downloads.jsm because all downloads have to use a saver of some sort. A nice fix would be to just support a PDFDownloadSaver in DownloadsCore.jsm.

Unfortunately, the current PDF backend also travels through the printing backend, and takes windows as parameters. So this would also need to do allow DownloadSource to take nsIDOMWindow paramters (i.e. I don't want to have to reopen the page just to Save as PDF in Fennec.

Long term, it would be nice if this saver would also take uris, open them in hiddenWindow, and then save that. But for now it could just throw if its not given a window object... Opinions?

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This works pretty well but doesn't report the right file size yet. What do you think paolo?

Comment 2

[:Paolo Amadini]

Comment on attachment 8484775 [details] [diff] [review]
Patch

(In reply to Wesley Johnston (:wesj) from comment #1)
> This works pretty well but doesn't report the right file size yet. What do
> you think paolo?

This seems like the right approach to me.

We have to pay attention to the fact we cannot restart PDF downloads after a browser restart. This is also true for complete page saves on Desktop, but in this case we actually know that in advance, so we may be smart about the behavior.

The patch needs dedicated tests, since document/window saving is not directly tested right now.

I'll definitely be able to review at some point next week.

I don't know why the file size is not correctly reported, but I believe fixing bug 941063 first may provide what you require with no need for extra handling inside the specific saver.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

OK. Updated this with some basic tests (and found some bugs along the way). Are there other tests you'd like written (I'm not sure how to test things like cancelling mid pdf save...) since we don't control a server here....

I'm also not sure what to do to prevent this from restarting on startup. Looking at the code, I guess we just don't restart if its succeeded, cancelled, or error properties are set. That would mean that we were killed mid save. I didn't want to flip any of those, and wasn't sure of a way to do this. I assume we'd basically try to resave without a window and then throw/fail.

Comment 4

[Wesley Johnston (:wesj)]

https://tbpl.mozilla.org/?tree=Try&rev=2e8569388731

Comment 5

[:Paolo Amadini]

Comment on attachment 8487580 [details] [diff] [review]
Patch

Review of attachment 8487580 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty good, only some simplifications and adjustments are needed.

(In reply to Wesley Johnston (:wesj) from comment #3)
> OK. Updated this with some basic tests (and found some bugs along the way).
> Are there other tests you'd like written (I'm not sure how to test things
> like cancelling mid pdf save...) since we don't control a server here....

If you don't have access to progress, you can simply trigger a cancellation just after the download starts, and verify that the destination file is not present when the cancellation is finished.

> I'm also not sure what to do to prevent this from restarting on startup.

I think we just need to prevent the PDF download object from being serialized at all, unless it has succeeded, in which case we serialize its saver as a "copy" type (see DownloadLegacySaver.toSerializable).

Currently, downloads are omitted from saving if serialization throws an exception (see DownloadsStore.save), but this would be reported to the console. We can proceed in two ways:
- Throw a special exception that is not reported when caught by DownloadStore.save.
- Modify DownloadStore.save to skip the item if serialization returns null, and return null in Download serialization if DownloadSaver serialization returns null.

This logic in DownloadStore would need its own xpcshell test.

(In reply to Wesley Johnston (:wesj) from comment #4)
> https://tbpl.mozilla.org/?tree=Try&rev=2e8569388731

Strangely, I see failures in nsIPrintSettingsService.initPrintSettingsFromPrefs.

::: modules/libpref/init/all.js
@@ +837,5 @@
>  pref("print.print_edge_left", 0);
>  pref("print.print_edge_right", 0);
>  pref("print.print_edge_bottom", 0);
>  
> +// PDF DownloadSaver

// Used by "DownloadPDFSaver"

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1108,5 @@
>      source.url = aSerializable.spec;
> +  } else if (aSerializable instanceof Ci.nsIDOMWindow) {
> +    source.url = aSerializable.location.href;
> +    source.isPrivate = PrivateBrowsingUtils.isWindowPrivate(aSerializable);
> +    source.window = aSerializable;

Technically, this representation is not serializable, but I admit it makes sense to have this code here. We should update the documentation of Download.createDownload and friends, and mention that downloads constructed in this way are not saved unless succeeded.

I believe we should also null out the "window" property as soon as the saver terminates (i.e. the download fails, is canceled, or succeeded). This should reduce chance of leaks. In any case, if printing to PDF fails the first time, it is unlikely that retrying would be useful.

We may define a canRestart property on the Download object, so that UI can disable the restart button, instead of trying again and failing, but this is better handled in a follow-up bug.

@@ +2236,5 @@
>    return new DownloadLegacySaver();
>  };
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// DownloadCopySaver

DownloadPDFSaver

@@ +2239,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// DownloadCopySaver
> +
> +/**
> + * Saver object that simply copies the entire source file to the target.

Update comment, describing behavior and limitations.

@@ +2248,5 @@
> +this.DownloadPDFSaver.prototype = {
> +  __proto__: DownloadSaver.prototype,
> +
> +  /**
> +   * An nsIWebBrowserPrint instance for printing this page

nit: dot at end of comments

@@ +2255,5 @@
> +
> +  /**
> +   * Implements "DownloadSaver.execute".
> +   */
> +  execute: function DCS_execute(aSetProgressBytesFn, aSetPropertiesFn)

DPS_execute, or you can just omit the name. Also for the other inline functions below.

@@ +2260,5 @@
> +  {
> +    return Task.spawn(function task_DCS_execute() {
> +      let deferSaveComplete = Promise.defer();
> +
> +      if (!this.download.source.window || !(this.download.source.window instanceof Ci.nsIDOMWindow)) {

I guess this is always a Ci.nsIDOMWindow, so the check is unneeded, but if it can somehow be set to a different type of window, there should be an xpcshell test for it.

@@ +2270,5 @@
> +
> +      this.addToHistory();
> +
> +      let target = this.download.target.path;
> +      let printSettingsService = Cc["@mozilla.org/gfx/printsettings-service;1"].getService(Ci.nsIPrintSettingsService);

nit: use a lazy service getter at the top of the file (gPrintSettingsService).

@@ +2280,5 @@
> +      printSettings.showPrintProgress = false;
> +      printSettings.printToFile = true;
> +      printSettings.toFileName = target;
> +
> +      // TODO: If the souce isn't a window, open it in hiddenWindow...

Please avoid TODO comments in the code, only file a separate bug.

@@ +2284,5 @@
> +      // TODO: If the souce isn't a window, open it in hiddenWindow...
> +      this._webBrowserPrint = this.download.source.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                                       .getInterface(Ci.nsIWebBrowserPrint);
> +
> +      this._webBrowserPrint.print(printSettings, {

Use "new Promise" syntax here:

yield new Promise((resolve, reject) => {
  this._webBrowserPrint.print(...)
});

Also the promise should reject with a DownloadError if the status is an error code (see onSaveComplete in the backgroundFileSaver call in this file).

@@ +2285,5 @@
> +      this._webBrowserPrint = this.download.source.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                                       .getInterface(Ci.nsIWebBrowserPrint);
> +
> +      this._webBrowserPrint.print(printSettings, {
> +        onStateChange: function(webProgress, request, stateFlags, status) {

nit: space after "function" (throughout the file)

@@ +2286,5 @@
> +                                                       .getInterface(Ci.nsIWebBrowserPrint);
> +
> +      this._webBrowserPrint.print(printSettings, {
> +        onStateChange: function(webProgress, request, stateFlags, status) {
> +          Task.spawn(function task_DCS_execute() {

Rather than being a new task, this code should be moved right after the "yield new Promise" block.

@@ +2291,5 @@
> +            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP && status === Cr.NS_OK) {
> +              try {
> +                let fileInfo = yield OS.File.stat(target);
> +                aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);
> +              } catch(ex) {

nit: space after "catch"

@@ +2292,5 @@
> +              try {
> +                let fileInfo = yield OS.File.stat(target);
> +                aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);
> +              } catch(ex) {
> +                Cu.reportError("Couldn't get file size for " + target + ", " + ex)

Just Cu.reportError(ex)

@@ +2300,5 @@
> +            }
> +          });
> +        },
> +
> +        onProgressChange: function(webProgress, request, curSelfProgress, maxSelfProgress, curTotalProgress, maxTotalProgress) {

Wrap at 80 characters (see indentation examples in the rest of the file).

@@ +2305,5 @@
> +          aSetProgressBytesFn(curTotalProgress, maxTotalProgress, false);
> +        },
> +
> +        onLocationChange: function(webProgress, request, location, flags) {
> +        },

Just "onLocationChange: function () { }," as in other parts of the file.

@@ +2323,5 @@
> +   * This can be called by the saver implementation when the download is already
> +   * started, to add it to the browsing history.  This method has no effect if
> +   * the download is private.
> +   */
> +  addToHistory: function ()

This is already defined on the prototype, no need to reimplement.

@@ +2358,5 @@
> +  cancel: function DCS_cancel()
> +  {
> +    if (this._webBrowserPrint) {
> +      this._webBrowserPrint.cancel();
> +    }

We should null out this._webBrowserPrint in "execute" just after the "print" call returns.

@@ +2366,5 @@
> +   * Implements "DownloadSaver.removePartialData".
> +   */
> +  removePartialData: function ()
> +  {
> +    yield OS.File.remove(this.download.target.path);

I don't think this is needed, since we can't handle partial PDF downloads anyways.

We may want to ensure the file is deleted if "execute" fails, if WebBrowserPrint does not do it already.

@@ +2374,5 @@
> +   * Implements "DownloadSaver.toSerializable".
> +   */
> +  toSerializable: function ()
> +  {
> +    return "pdf"

Should become a "copy" type.

@@ +2381,5 @@
> +  /**
> +   * Implements "DownloadSaver.getSha256Hash"
> +   */
> +  _sha256Hash: null,
> +  getSha256Hash: function ()

Again, you can omit these functions implemented on the prototype.

@@ +2416,5 @@
> + * @return The newly created DownloadPDFSaver object.
> + */
> +this.DownloadPDFSaver.fromSerializable = function (aSerializable) {
> +  let saver = new DownloadPDFSaver();
> +  return saver;

Just "return new..."

::: toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js
@@ +15,5 @@
> +    let tab = gBrowser.addTab("http://mochi.test:8888/");
> +    if (aPrivate) {
> +      tab.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing = true;
> +    }
> +    yield afterAllTabsLoaded();

I'm pretty sure you can wait for the page to be loaded without this complex helper... we should probably load a sample page from our "data" folder anyways.

@@ +19,5 @@
> +    yield afterAllTabsLoaded();
> +
> +    let download = yield Downloads.createDownload({
> +      source: tab.linkedBrowser.contentWindow,
> +      target: { path: getTempFile(TEST_TARGET_FILE_NAME).path },

For clarity, you may want to use a target file name with a .pdf extension here.

@@ +32,5 @@
> +    is(download.source.isPrivate, aPrivate, "Download source is private");
> +    ok(download.stopped, "Download is stopped");
> +    ok(!download.canceled, "Download is not cancelled");
> +    ok(download.succeeded, "Download succeeded");
> +    is(download.error, null, "Download error is not defined");

nit: I don't think the descriptions add much value, but since they are required, make them equal to the condition, like "download.succeeded".

@@ +35,5 @@
> +    ok(download.succeeded, "Download succeeded");
> +    is(download.error, null, "Download error is not defined");
> +
> +    let exists = yield OS.File.exists(download.target.path)
> +    ok(exists, "Target file exists");

Can you check that the target file is actually in PDF format?

Comment 6

[:Paolo Amadini]

By the way, I want to note that this patch is particularly helpful for Desktop too, not only because of the PDF saver itself, but also because this would use the same technique required to save complete HTML pages without going through DownloadLegacySaver. When implemented, that will finally solve the restart bugs we have with complete HTML pages.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated.

Comment 8

[:Paolo Amadini]

Comment on attachment 8491176 [details] [diff] [review]
Patch

Review of attachment 8491176 [details] [diff] [review]:
-----------------------------------------------------------------

This still needed some of the changes from the previous iteration, I've added some details now in case the rationale for some of the requests was unclear.

I have not looked in detail into all tests, at a glance they look good.

::: modules/libpref/init/all.js
@@ +839,5 @@
>  pref("print.print_edge_left", 0);
>  pref("print.print_edge_right", 0);
>  pref("print.print_edge_bottom", 0);
>  
> +// PDF DownloadSaver

Please change this comment to something like "Used by DownloadPDFSaver" at least, so that the relevant code can be cross-referenced by searching for the text. Even better, write a more detailed comment about how these preferences are used.

The preference names are generated dynamically, so it's not easy to find out where they are used with a simple global text search using their names.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +913,5 @@
>      let saver = this.saver.toSerializable();
> +    if (!saver) {
> +      // If we are unable to serialize the saver, we won't persist the download.
> +      return null;
> +    } else  if (saver !== "copy") {

Please move the "let saver" and first "if" above the comment, as the comment actually applies to the second "if". We also don't generally use "else" when the first branch ends with a "return".

@@ +1114,5 @@
>      source.url = aSerializable.spec;
> +  } else if (aSerializable instanceof Ci.nsIDOMWindow) {
> +    source.url = aSerializable.location.href;
> +    source.isPrivate = PrivateBrowsingUtils.isWindowPrivate(aSerializable);
> +    source.window = Cu.getWeakReference(aSerializable);

.windowRef

@@ +2242,5 @@
>    return new DownloadLegacySaver();
>  };
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// DownloadCopySaver

This should be "DownloadPDFSaver"

@@ +2245,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// DownloadCopySaver
> +
> +/**
> + * Saver object that saves a window into a pdf document.

This comment should be expanded.

@@ +2254,5 @@
> +this.DownloadPDFSaver.prototype = {
> +  __proto__: DownloadSaver.prototype,
> +
> +  /**
> +   * An nsIWebBrowserPrint instance for printing this page.

Add: "This is null when saving has not started or has completed, or while the operation is being canceled." or something along these lines.

(missed this in the first pass, sorry)

@@ +2269,5 @@
> +
> +      let win = null;
> +      if (this.download.source.window) {
> +        win = this.download.source.window.get();
> +      }

This would be "let window = this.download.source.windowRef && this.download.source.windowRef.get();".

But actually, you need to check for the two null cases separately, and use a different message if it is "get" that fails (window closed?).

Also, with regard to restart, in the current state it seems to me that, if the print fails and the user retries later, a completely different page or website could be saved with the old file name.

We should probably do one of those things as soon as this function is called:
 - Null out windowRef.
 - Set and check a state variable that prevents retrying.
 - Store a weak reference to the original document the first time, and verify that the window document matches on restarts. This would need tests, I'd suggest doing it in a follow-up bug.

@@ +2279,5 @@
> +        });
> +      }
> +
> +      this._webBrowserPrint = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIWebBrowserPrint);

Hm, I just realized that, to avoid race conditions in case new asynchronous code is added, the "this._webBrowserPrint" variable should only be assigned exactly before the call to "print".

@@ +2297,5 @@
> +
> +      this._webBrowserPrint.print(printSettings, {
> +        onStateChange: function (webProgress, request, stateFlags, status) {
> +          Task.spawn(function task_DCS_execute() {
> +            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {

Please use "new Promise" syntax and no nested task. Let me know if you need help with that!

@@ +2298,5 @@
> +      this._webBrowserPrint.print(printSettings, {
> +        onStateChange: function (webProgress, request, stateFlags, status) {
> +          Task.spawn(function task_DCS_execute() {
> +            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> +              if (status !== Cr.NS_OK) {

!Components.isSuccessCode(status)

Question: in this case, do we need to ensure the target file is deleted? If so, it should be done in a catch-and-rethrow block placed around the "yield new Promise" block.

@@ +2308,5 @@
> +                let fileInfo = yield OS.File.stat(target);
> +                aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);
> +              } catch (ex) {
> +                Cu.reportError("Couldn't get file size for " + target +
> +                               ", " + ex);

If we get here, something is really wrong, because the file should exist at this point. This is why special details in the exception handler are not needed, and you should just use "Cu.reportError(ex)" in a standard catch-and-continue way.

Actually, if you make this not nested, as I suggested, there is nothing more done after this block by the "execute" function. So, the entire catch-and-continue block becomes unneeded, and the code simpler.

@@ +2321,5 @@
> +                                    maxTotalProgress) {
> +          aSetProgressBytesFn(curTotalProgress, maxTotalProgress, false);
> +        },
> +
> +        onLocationChange: function (webProgress, request, location, flags) {

No parameter names for empty functions (as per style in this file), and either "{}" or "{ }" on same line. No empty lines between empty functions.

@@ +2332,5 @@
> +        },
> +      });
> +
> +      // Remove the print object to avoid leaks
> +      this._webBrowserPrint = null;

Given that "print" could raise an exception, this should go into a finally block. Also, this is not only for leaks but also for cancellation.

@@ +2343,5 @@
> +   * This can be called by the saver implementation when the download is already
> +   * started, to add it to the browsing history.  This method has no effect if
> +   * the download is private.
> +   */
> +  addToHistory: function ()

Is there a reason to reimplement this?

@@ +2387,5 @@
> +   * Implements "DownloadSaver.removePartialData".
> +   */
> +  removePartialData: function ()
> +  {
> +    throw new Error("Not implemented.");

This should not throw, just leave out the function to use the base implementation (returns a resolved promise).

::: toolkit/components/jsdownloads/src/DownloadStore.jsm
@@ +164,5 @@
>            if (!this.onsaveitem(download)) {
>              continue;
>            }
> +
> +          let serialized = download.toSerializable();

nit: "serializable"

@@ +165,5 @@
>              continue;
>            }
> +
> +          let serialized = download.toSerializable();
> +          if (serialized) {

if (!serializable) {
  // This item cannot be persisted across sessions.
  continue;
}

Comment 9

[:Paolo Amadini]

Hey Wesley, do you need any help with the code changes here?

Comment 10

[Wesley Johnston (:wesj)]

Haven't had time to look at this in a bit.

Comment 11

[:Margaret Leibovic]

Paolo, how close is this patch to landing? And is it required for bug 901360 to land? I'm trying to finish up and land the patches in that bug, and I'm wondering if there's a way for me to do that without waiting on this bug. Or, if we do need this bug, maybe I can help finish it.

Comment 12

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #11)
> Paolo, how close is this patch to landing? And is it required for bug 901360
> to land? I'm trying to finish up and land the patches in that bug, and I'm
> wondering if there's a way for me to do that without waiting on this bug.
> Or, if we do need this bug, maybe I can help finish it.

Hey Margaret, thanks a lot for helping with getting the migration landed :-)

This bug has a clear path forward - see comment 8 for the latest review comments. Getting this bug finished is probably the quickest way to solve bug 997723, which was a regression with the Downloads.jsm migration.

Bug 997723 comment 7 mentioned a possible different approach, but I still think finishing this bug will be fast and also get us much better testing.

Comment 13

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #8)

> I have not looked in detail into all tests, at a glance they look good.

The xpcshell test pass, but the browser-chrome ones don't :( I haven't looked into these myself yet, but I can try to debug what's going on. Also, it looks like wesj didn't hg add testFile.html, but I assume it was just a random test page, so I can make something up for that.

> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm

> @@ +2269,5 @@
> > +
> > +      let win = null;
> > +      if (this.download.source.window) {
> > +        win = this.download.source.window.get();
> > +      }
> 
> This would be "let window = this.download.source.windowRef &&
> this.download.source.windowRef.get();".
> 
> But actually, you need to check for the two null cases separately, and use a
> different message if it is "get" that fails (window closed?).
> 
> Also, with regard to restart, in the current state it seems to me that, if
> the print fails and the user retries later, a completely different page or
> website could be saved with the old file name.
> 
> We should probably do one of those things as soon as this function is called:
>  - Null out windowRef.
>  - Set and check a state variable that prevents retrying.
>  - Store a weak reference to the original document the first time, and
> verify that the window document matches on restarts. This would need tests,
> I'd suggest doing it in a follow-up bug.

I'm going to leave this for a follow-up bug :)

> @@ +2297,5 @@
> > +
> > +      this._webBrowserPrint.print(printSettings, {
> > +        onStateChange: function (webProgress, request, stateFlags, status) {
> > +          Task.spawn(function task_DCS_execute() {
> > +            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> 
> Please use "new Promise" syntax and no nested task. Let me know if you need
> help with that!

I've used the "new Promise" syntax before, but I'm not clear on how to swap that in here, given the deferSaveComplete promise that needs to be resolved/rejected as well. Can you help elaborate?

Comment 14

[:Margaret Leibovic]

Attachment: Updated patch

I've addressed the other review comments here. I'll look into the test failures before asking for review.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8510616 [details] [diff] [review]
Updated patch

Review of attachment 8510616 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1115,5 @@
>    } else if (aSerializable instanceof Ci.nsIURI) {
>      source.url = aSerializable.spec;
> +  } else if (aSerializable instanceof Ci.nsIDOMWindow) {
> +    source.url = aSerializable.location.href;
> +    source.isPrivate = PrivateBrowsingUtils.isContentWindowPrivate(aSerializable);

I updated this to use isContentWindowPrivate instead of isWindowPrivate, since this has changed since this patch was written.

However, this has me a bit worried about e10s. Will this run in the content process? I don't think we need to block on e10s support before landing this, since it will only be used on Android to start, but it would be worth thinking about this if we want to use this on desktop in the future.

Comment 16

[:Margaret Leibovic]

Attachment: Updated patch (v2)

This is getting closer! I found that the initPrintSettingsFromPrefs call was throwing an error when I ran the tests, but manually setting the prefs (like we currently do in our browser.js) works. Do you know why this may be causing a problem in the test environment?

I'm seeing 2 remaining failures, both related to not finding the saved file. Any clues as to what might be going wrong? This patch did actually work on my Android device when I applied the patches from bug 901360 on top of it, so maybe there's just something wrong with the test?

19 INFO Console message: [JavaScript Error: "Unix error 2 during operation setPermissions on file /Users/leibovic/Library/Caches/TemporaryItems/test-download-174327.pdf (No such file or directory)" {file: "resource://gre/modules/DownloadIntegration.jsm" line: 469}]
20 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js | Target file exists - 
Stack trace:
    chrome://mochitests/content/browser/toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js:test_createDownload_pdf:45
    self-hosted:next:914
    Tester_execTest@chrome://mochikit/content/browser-test.js:646:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:572:7

40 INFO Console message: [JavaScript Error: "Unix error 2 during operation setPermissions on file /Users/leibovic/Library/Caches/TemporaryItems/test-download-174328.pdf (No such file or directory)" {file: "resource://gre/modules/DownloadIntegration.jsm" line: 469}]
41 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js | Target file exists - 
Stack trace:
    chrome://mochitests/content/browser/toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js:test_createDownload_pdf:45
    self-hosted:next:914
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7

Comment 17

[:Paolo Amadini]

Comment on attachment 8510677 [details] [diff] [review]
Updated patch (v2)

Review of attachment 8510677 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Margaret Leibovic from comment #15)
> However, this has me a bit worried about e10s. Will this run in the content
> process? I don't think we need to block on e10s support before landing this,
> since it will only be used on Android to start, but it would be worth
> thinking about this if we want to use this on desktop in the future.

This code is all executed in the parent process. I think this should do something similar to what "save as" and normal printing do. I don't know the state of e10s compatibility for these.

(In reply to :Margaret Leibovic from comment #16)
> This is getting closer! I found that the initPrintSettingsFromPrefs call was
> throwing an error when I ran the tests, but manually setting the prefs (like
> we currently do in our browser.js) works. Do you know why this may be
> causing a problem in the test environment?

Hm, maybe the call is looking for other global preferences that are not defined or have changed? This is the only suggestion I have, unfortunately :-(

> I'm seeing 2 remaining failures, both related to not finding the saved file.
> Any clues as to what might be going wrong? This patch did actually work on
> my Android device when I applied the patches from bug 901360 on top of it,
> so maybe there's just something wrong with the test?
> 
> 19 INFO Console message: [JavaScript Error: "Unix error 2 during operation
> setPermissions on file
> /Users/leibovic/Library/Caches/TemporaryItems/test-download-174327.pdf (No
> such file or directory)" {file:
> "resource://gre/modules/DownloadIntegration.jsm" line: 469}]

I didn't spot anything obvious in the tests, but this error looks like production code, so maybe the file is really not created for some reason, or created later.

In xpcshell tests we run in an environment where some DownloadIntegration functions are disabled, but this isn't one of those:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#804

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +2247,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// DownloadPDFSaver
> +
> +/**
> + * Saver object that saves a window into a pdf document.

"This DownloadSaver type creates a PDF file from the current document in a given window, specified using the windowRef property of the DownloadSource object associated with the download.

In order to prevent the download from saving a different document than the one originally loaded in the window, any attempt to restart the download will fail.

Since this DownloadSaver type requires a live document as a source, it cannot be persisted across sessions, unless the download already succeeded."

@@ +2283,5 @@
> +        throw new DownloadError({
> +          message: "PDF saver can't save a window that has been closed.",
> +          becauseSourceFailed: true,
> +        });
> +      }

We still need to prevent retrying, either by setting windowRef to null (and adjusting the error message above) or adding a separate state variable triggering its own failure message.

@@ +2312,5 @@
> +      this._webBrowserPrint = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIWebBrowserPrint);
> +
> +      try {
> +        this._webBrowserPrint.print(printSettings, {

The "new Promise" syntax would look like this (within the "try-finally"):

yield new Promise((resolve, reject) => {
  this._webBrowserPrint.print(printSettings, {
    onStateChange: function (webProgress, request, stateFlags, status) {
      if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
        if (!Components.isSuccessCode(status)) {
          reject(new DownloadError(...from status code...));
        } else {
          resolve();
        }
      }
    },
    onProgressChange: function (...) {
       aSetProgressBytesFn(...);
    },
    ...
  });
});

So, "yield" will wait for the operation to complete, then continue or raise an exception (the DownloadError passed to "reject"). In this case the "finally" block will be executed before the exception is propagated to the caller of "execute".

The current code has the issue that there's no instruction to wait before proceeding to the "finally" block, so cancellation won't work.

After the try-finally, you can implement this non-cancelable operation:

let fileInfo = yield OS.File.stat(target);
aSetProgressBytesFn(fileInfo.size, fileInfo.size, false);

This way, you don't need the deferred object anymore.

::: toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js
@@ +56,5 @@
> +    ok(download.source.windowRef.get(), "Download window was removed");
> +  }
> +}
> +
> +add_task(buildTask(true, "pdf"));

Quick note about style (I've still not fully reviewed the tests):

function* test_createDownload_common(aPrivate, aType) {
  ...
}

And then for each test:

add_task(function* test_createDownload_pdf_private() {
  yield test_createDownload_common(true, "pdf");
});

This ensures a unique test description (the function name) is used by the test suite.

::: toolkit/components/jsdownloads/test/browser/head.js
@@ +31,5 @@
> +                                  "resource://testing-common/httpd.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +
> +const TEST_TARGET_FILE_NAME = "test-download.pdf";

nit: TEST_TARGET_FILE_NAME_PDF

Comment 18

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #17)

> > I'm seeing 2 remaining failures, both related to not finding the saved file.
> > Any clues as to what might be going wrong? This patch did actually work on
> > my Android device when I applied the patches from bug 901360 on top of it,
> > so maybe there's just something wrong with the test?
> > 
> > 19 INFO Console message: [JavaScript Error: "Unix error 2 during operation
> > setPermissions on file
> > /Users/leibovic/Library/Caches/TemporaryItems/test-download-174327.pdf (No
> > such file or directory)" {file:
> > "resource://gre/modules/DownloadIntegration.jsm" line: 469}]
> 
> I didn't spot anything obvious in the tests, but this error looks like
> production code, so maybe the file is really not created for some reason, or
> created later.

The problem was that the file didn't exist, but the DownloadPDFSaver expected to be passed a target file that already existed. I added logic to create the target file if it doesn't already exist, since it looks like we do that elsewhere in the downloads code, but let me know if this was the wrong approach.

After that fix, the tests were all passing locally for me, so I made a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e59cc17a6622

Comment 19

[:Margaret Leibovic]

Attachment: Updated patch (v3)

Comment 20

[:Margaret Leibovic]

I think there's something wrong with this patch. I was testing with a build with the patches from bug 901360 applied as well, and it looks like we restart a cancelled save as PDF download when the app restarts.

Comment 21

[:Paolo Amadini]

Comment on attachment 8512270 [details] [diff] [review]
Updated patch (v3)

Review of attachment 8512270 [details] [diff] [review]:
-----------------------------------------------------------------

We're getting closer. Thanks for debugging the PDF printer! I believe this work and the tests for it are definitely worth the effort.

(In reply to :Margaret Leibovic from comment #20)
> I think there's something wrong with this patch. I was testing with a build
> with the patches from bug 901360 applied as well, and it looks like we
> restart a cancelled save as PDF download when the app restarts.

There's actually a bug we found recently while working on bug 1068664, because of a missing deserialization of the "error" property of downloads:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#988

So failed downloads look like they're in-progress, and are indeed restarted by DownloadStore:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadStore.jsm#122

On Desktop this doesn't happen because we don't serialize them at all. However, this shouldn't be related to canceled downloads, only failed ones, so not sure it's what you're seeing here.

I'll ask Steven if he has already done some work here.

I've now reviewed the tests as well, though I've noticed they are still similar to the original patch, in order to help with their clean up so that the next iteration can be a quick final pass.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +2256,5 @@
> + *
> + * In order to prevent the download from saving a different document than the one
> + * originally loaded in the window, any attempt to restart the download will fail.
> + *
> + * Since this DownloadSaver type requires a live document as a source, it cannot 

nit: whitespace at end of line

@@ +2280,5 @@
> +  {
> +    return Task.spawn(function task_DCS_execute() {
> +      if (!this.download.source.windowRef) {
> +        throw new DownloadError({
> +          message: "PDF saver must be passed an open window.",

"PDF saver must be passed an open window, and cannot be restarted."

@@ +2288,5 @@
> +
> +      let win = this.download.source.windowRef.get();
> +      if (!win) {
> +        // Set windowRef to null to avoid re-trying.
> +        this.download.source.windowRef = null;

This should happen unconditionally, even if the window is still there.

@@ +2301,5 @@
> +
> +      let targetPath = this.download.target.path;
> +      // The download implementation may not have created the target file if
> +      // no data was received from the source.  In this case, ensure that an
> +      // empty file is created as expected.

In this case the comment would be different, as the issue there was that some external download implementations using the LegacySaver didn't create any file when the source was empty.

The issue here (if understand correctly) is that an empty target file must exist for the PDF printer to work correctly.

@@ +2304,5 @@
> +      // no data was received from the source.  In this case, ensure that an
> +      // empty file is created as expected.
> +      try {
> +        // This atomic operation is more efficient than an existence check.
> +        let file = yield OS.File.open(targetPath, { create: true });

And in fact, this probably needs to be "truncate" rather than "create", as we need to overwrite any existing target file. We should also raise errors normally, if truncation can't happen, so no catch is needed.

@@ +2336,5 @@
> +          this._webBrowserPrint.print(printSettings, {
> +            onStateChange: function (webProgress, request, stateFlags, status) {
> +              if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> +                if (!Components.isSuccessCode(status)) {
> +                  reject(new DownloadError({ message: "PDF download failed with status " + status }));

new DownloadError({ result: status, inferCause: true })

This should give better insight on whether the issue is with the source or the target.

::: toolkit/components/jsdownloads/test/browser/browser.ini
@@ +2,5 @@
> +support-files =
> +    head.js
> +    testFile.html
> +
> +[browser_test_pdf_download.js]

nit: just "browser_pdf_download" works, or maybe "browser_DownloadPDFSaver"

::: toolkit/components/jsdownloads/test/browser/browser_test_pdf_download.js
@@ +4,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Tests using the PDF download saver, and passing windows to passing windows to
> + * downloads savers

typo

@@ +28,5 @@
> +  is(aDownload.error, null, "Download error is not defined");
> +}
> +
> +function* test_createDownload_common(aPrivate, aType) {
> +  let tab = gBrowser.addTab("https://example.com/browser/toolkit/components/jsdownloads/test/browser/testFile.html");

There's a poorly documented (undocumented?) way to get this URL:

getRootDirectory(gTestPath) + "testFile.html"

That might be a "chrome" URL however, I guess it should work as well.

@@ +37,5 @@
> +
> +  let download = yield Downloads.createDownload({
> +    source: tab.linkedBrowser.contentWindow,
> +    target: getTempFile(TEST_TARGET_FILE_NAME_PDF),
> +    saver: { type: aType }

global nit: comma after last property too, when closing intializer brace is on next line.

i.e.

saver: { type: aType },

but not

saver: { type: aType, },

@@ +51,5 @@
> +  is(arr.length, 0, "Successful pdf downloads stored");
> +
> +  list = yield Downloads.getList(aPrivate ? Downloads.PUBLIC : Downloads.PRIVATE);
> +  arr = yield list.getAll();
> +  is(arr.length, 0, "Successful pdf downloads stored");

These tests for DownloadList should be removed - adding downloads to the list based on their private status is already covered elsewhere.

@@ +54,5 @@
> +  arr = yield list.getAll();
> +  is(arr.length, 0, "Successful pdf downloads stored");
> +
> +  gBrowser.removeTab(tab);
> +  ok(download.source.windowRef.get(), "Download window was removed");

This test is checking the opposite of what it is saying? I believe it can be removed, as we're not testing the failure case.

Or, you may want to add a test for the "window closed" case, but need to wait for the DOM window to be actually removed. This may be done in a follow-up.

Most of the changes above apply to the other test as well.

@@ +72,5 @@
> +add_task(function* test_createDownload_copy_not_private() {
> +  yield test_createDownload_common(false, "copy");
> +});
> +
> +add_task(function() {

Needs test name.

@@ +84,5 @@
> +  });
> +
> +  download.start();
> +  // immediately cancel the download to test that it is erased correctly
> +  yield download.cancel()

nit: missing semicolon

::: toolkit/components/jsdownloads/test/unit/test_DownloadStore.js
@@ +61,5 @@
> +  let pdfDownload = yield Downloads.createDownload({
> +    source: { url: httpUrl("empty.txt"),
> +              referrer: TEST_REFERRER_URL },
> +    target: getTempFile(TEST_TARGET_FILE_NAME),
> +    saver: "pdf"

comma

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
@@ +74,5 @@
> +  try {
> +    yield download.start();
> +    do_throw("The download should have failed.");
> +  } catch (ex if ex instanceof Downloads.Error && ex.becauseSourceFailed) {
> +  }

nit: brace on same line with space in-between

Comment 22

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #21)
> There's actually a bug we found recently while working on bug 1068664,
> because of a missing deserialization of the "error" property of downloads:

By the way, the issue with the restart doesn't prevent this bug from landing (though it may block the full migration).

Comment 23

[:Margaret Leibovic]

Attachment: Updated patch (v4)

My last try run had some failures, and I suspect that the tabLoaded helper function had some problems, so I replaced that with a simpler promiseBrowserLoaded function.

I'm going to make another try push when the try tree re-opens, but this is passing locally, and I think it addresses all of the review comments.

Comment 24

[:Paolo Amadini]

Comment on attachment 8513065 [details] [diff] [review]
Updated patch (v4)

Review of attachment 8513065 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +2312,5 @@
> +      printSettings.showPrintProgress = false;
> +      printSettings.printToFile = true;
> +      printSettings.toFileName = targetPath;
> +
> +      // TODO: Create preferences for these settings.

Can you please file a bug to figure out how to do this using initPrintSettingsFromPrefs or another method, and remove the TODO?

::: toolkit/components/jsdownloads/test/browser/browser_DownloadPDFSaver.js
@@ +50,5 @@
> +  yield download.start();
> +
> +  yield test_download_state_complete(tab, download, aPrivate, false);
> +  let exists = yield OS.File.exists(download.target.path);
> +  ok(exists, "Target file exists");

I just remembered that we can check the file is a PDF instead of just its existence.

This would look something like this:

let signature = yield OS.File.read(download.target.path, { bytes: 4, encoding: "us-ascii" });
is(signature, "PDF%", "File exists and signature matches");

@@ +92,5 @@
> +  ok(!exists, "Target file does not exist");
> +
> +  let list = yield Downloads.getList(Downloads.PUBLIC);
> +  let arr = yield list.getAll();
> +  is(arr.length, 0, "Failed pdf downloads aren't stored");

This DownloadList test can be removed, as downloads aren't stored in the list anyways unless you add code for that.

Comment 25

[:Margaret Leibovic]

Sigh, this is causing e10s test failures:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca2039a11a70

Luckily, I was able to reproduce these failures locally if I run the tests with the --e10s flag.

mconley, I believe you've been working on e10s printing, can you take a look at this patch to let me know what I might need to do to make it e10s compatible? Or redirect this request if you're not the right person? :)

I'm worried that this patch might have some fundamental problems :/

Comment 26

[:Paolo Amadini]

Unless making PDF printing work with e10s is trivial (and I guess it's not because this requires the parent process to access the content DOM, with some other kind of message passing), I'd disable the test and leave this for a follow-up bug.

In fact this code will not be used on Desktop soon, and work for fixing e10s printing in general may help here.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

So the problem here is that the DownloadPDFSaver execute function is calling / manipulating an nsIDOMWindow, getting its nsIWebBrowserPrint, and calling functions on it.

In non-e10s, this is perfectly fine. With e10s, this just won't work - the nsIDOMWindow will actually be a CPOW, and I can tell you with a high degree of confidence that our printing machinery doesn't like being handed CPOWs.

The general solution to this is to create the print settings in the child process, and have the child process kick off the printing machinery. That would involve running a content script and sending a message, which isn't so hard. The hard part is mapping the nsIDOMWindow CPOW to a remote browser for us to message. There are app-specific mechanisms for doing that for both Fennec and Firefox Desktop, but we'd probably want some app-agnostic solution for this particular case.

margaret tells me that this is the last thing preventing Fennec from switching to Downloads.jsm. Since this Download-to-PDF thing isn't something that's even exposed to desktop (yet), I suggest just disabling the failing test for e10s, and filing a follow-up bug to make this e10s compatible.

It sucks adding more non-e10s-compatible code to the tree, but the e10s has bigger fish to fry right now. I, for one, think we can let this one slide. :)

Comment 28

[:Margaret Leibovic]

There are still problems with the tests, although not on all platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d87bcad82d50

Looks like we have timeouts on Windows, and some print settings leak on Linux asan. I don't have a Windows machine, so I don't know how to reproduce locally. Maybe I can try to find a machine to use in the office.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Whoops, should have cleared my needinfo.

Comment 30

[:Margaret Leibovic]

wesj is back from PTO and said he would take this back.

Comment 31

[:Paolo Amadini]

This is pretty close to landing. I debugged the failure on Windows, corresponding to this issue:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/7c8a564172aba8d0382969cdd3f87a375f01a8c755d9f722f844aec5e8579425c590142ceed2c084c0bf021361478d30836de467f698dcdbff112aba7b36ea8a

I found out it could be resolved simply by removing the line that sets the printer name:

printSettings.printerName = "mozPDF";

I don't know if this may be related to the failures on other platforms. In general I don't think the printer name is required if we don't use it to read the preferences.

(In reply to :Paolo Amadini from comment #24)
> let signature = yield OS.File.read(download.target.path, { bytes: 4,
> encoding: "us-ascii" });
> is(signature, "PDF%", "File exists and signature matches");

This works fine, but the right signature is "%PDF".

Comment 32

[Wesley Johnston (:wesj)]

I am too swamped. If paolo or someone can finish it up quick... I'd be happy (and I think the rest of our downloads.jsm work is ready to land then as well...)

Comment 33

[:Paolo Amadini]

I've investigated some of the test failures and they're probably related to existing bugs and leaks in the printing code, that is currently not covered by regression tests.

I don't believe we should try and fix printing code in this bug, so I've disabled the tests except on Windows, where they seem to work. I've started a new try build:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6c8b2dee4a89

Comment 34

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #33)
> I've investigated some of the test failures and they're probably related to
> existing bugs and leaks in the printing code, that is currently not covered
> by regression tests.
> 
> I don't believe we should try and fix printing code in this bug, so I've
> disabled the tests except on Windows, where they seem to work. I've started
> a new try build:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6c8b2dee4a89

Thanks for helping drive this forwards. Hopefully this is all green and we can just land this.

Comment 35

[:Paolo Amadini]

Attachment: Checked in

https://hg.mozilla.org/integration/fx-team/rev/a8f9ed3d7554

Comment 36

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #34)
> Thanks for helping drive this forwards. Hopefully this is all green and we
> can just land this.

Looks like the try build only had unrelated failures, I've landed the version in the latest attachment.

Margaret, can you verify that this still works as expected on Android, together with the other patches? I've removed the printer name, and while I don't think this is relevant for Android, only actual testing can tell.

Comment 37

[:Margaret Leibovic]

With the patches from bug 901360 applied, saving a page as PDF appears to work fine (notification works, about:downloads works, PDF looks like it's printed correctly), but I'm seeing this one error in the console:

W/GeckoConsole(22113): [JavaScript Error: "Unix error 1 during operation setPermissions on file /storage/emulated/0/Download/Mozilla.pdf (Operation not permitted)" {file: "resource://gre/modules/DownloadIntegration.jsm" line: 486}]

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 39

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #37)
> W/GeckoConsole(22113): [JavaScript Error: "Unix error 1 during operation
> setPermissions on file /storage/emulated/0/Download/Mozilla.pdf (Operation
> not permitted)" {file: "resource://gre/modules/DownloadIntegration.jsm"
> line: 486}]

This happens because we try to set final permissions but they are not supported on the FAT file system where downloads are saved. We could catch that OS.File exception specifically on Android, and avoid reporting it to the Console, or display a log message instead.

Comment 40

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #39)
> (In reply to :Margaret Leibovic from comment #37)
> > W/GeckoConsole(22113): [JavaScript Error: "Unix error 1 during operation
> > setPermissions on file /storage/emulated/0/Download/Mozilla.pdf (Operation
> > not permitted)" {file: "resource://gre/modules/DownloadIntegration.jsm"
> > line: 486}]
> 
> This happens because we try to set final permissions but they are not
> supported on the FAT file system where downloads are saved. We could catch
> that OS.File exception specifically on Android, and avoid reporting it to
> the Console, or display a log message instead.

Sounds like a good mentor bug. Do you want to file a follow-up?

Comment 41

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #40)
> Sounds like a good mentor bug. Do you want to file a follow-up?

Filed bug 1114637, it could be mentored by someone on the mobile team and I'm available to review the patches.