1404681 - WebExtensions: tabs.saveAsPDF() throwing exception in Firefox 57.0b3 & 58.0a1

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

I tried to save an edited web page as a PDF file using my Print Edit WE add-on.

The steps to reproduce are:

1. Navigate to Wikipedia home page.
2. Click on Print Edit WE toolbar button. This creates a duplicate tab.
3. Click on Print Edit WE toolbar button (now showing 'PREP' badge) again.
4. Click on 'Save' button on Print Edit WE toolbar and select 'Save As PDF'.
5. In 'Save As' dialog, select folder and clicked on 'Save' button.

A 0 byte file is saved and an error message appears in browser console.

The save is initiated by line 921 in  mozilla-central/browser/components/extensions/ext-tabs.js:

    921    activeTab.linkedBrowser.print(activeTab.linkedBrowser.outerWindowID, printSettings, null);

The problem appears to be the 'null' third argument, which is a print progress listener.

This works in Firefox 56, but does not work in Firefox 57.0b3 or Nightly 58.0a1.




Actual results:

After clicking on 'Save' button in 'Save As' dialog, a 0 byte file is saved and the following error message appears in the browser console:

    TypeError: Argument 3 of FrameLoader.print is not an object.[Learn More]  browser.xml:1577:13

Lines 1577 & 1578 in view-source:chrome://global/content/bindings/browser.xml are:

    1577    this.frameLoader.print(aOuterWindowID, aPrintSettings,
    1578                                  aPrintProgressListener);

This corresponds to lines 1587 & 1588 in mozilla-central/toolkit/content/widgets/browser.xml.



Expected results:

The file should have been saved correctly.  It should not have been a 0 byte file.  It should have contained a PDF representation of the Wikipedia home page.

Comment 1

[dw-dev]

Hi Bob,

I developed a new WebExtensions API, tabs.saveAsPDF(), to support the saving of a web page as a PDF file in my new 'Print Edit WE' add-on (which is a re-implementation of my legacy 'Print Edit' extension).

The code used (in mozilla-central/browser/components/extensions/ext-tabs.js) to save a web page as a PDF file is:

    activeTab.linkedBrowser.print(activeTab.linkedBrowser.outerWindowID, printSettings, null);

This works perfectly in Firefox 56, but fails in Firefox 57.0b3 and Nightly 58.0a1 with the following message:

    TypeError: Argument 3 of FrameLoader.print is not an object.[Learn More]  browser.xml:1577:13

The problem seems to be passing 'null' as the aPrintProgressListener argument.


TWO QUESTIONS:

  - Has something changed recently in the printing code to cause this error?

  - Do you have an example of what a print progress listener should look like?

Comment 2

[dw-dev]

Hi Shane,

It looks as if we will need to issue a patch to tabs.saveAsPDF() to fix this problem.

And we will need to uplift the patch to Firefox 57.

I am hoping that Bob Owen can give guidance on the simplest way to fix this.

Are you okay for me to work on this?

Comment 3

[Kris Maglione [:kmag]]

Oh, this is because we converted the FrameLoader bindings to WebIDL, which doesn't allow arguments to be null by default. If that argument is allowed to be null, we need to add the `optional` keyword and add `?` after its type.

Do we not have tests for this API?

Comment 4

[dw-dev]

Bob,

> Oh, this is because we converted the FrameLoader bindings to WebIDL, which
> doesn't allow arguments to be null by default. If that argument is allowed
> to be null, we need to add the `optional` keyword and add `?` after its type.

Is the aPrintProgressListener argument allowed to be null?

Comment 5

[dw-dev]

Kris,

> Do we not have tests for this API?

I assume you are referring to the tabs.saveAsPDF() API as opposed to the browser.print() call ?

Neither myself, Shane Caraveo or Bob Owen could think of a way of testing this API.

In my original Print Edit extension, I did have a print progress listener on the browser.print() call, so I can add a similar listener in this case if that is the best solution.

Comment 6

[Kris Maglione [:kmag]]

(In reply to dw-dev from comment #4)
> Bob,
> 
> > Oh, this is because we converted the FrameLoader bindings to WebIDL, which
> > doesn't allow arguments to be null by default. If that argument is allowed
> > to be null, we need to add the `optional` keyword and add `?` after its type.
> 
> Is the aPrintProgressListener argument allowed to be null?

Apparently it is, yes.

(In reply to dw-dev from comment #5)
> Kris,
> 
> > Do we not have tests for this API?
> 
> I assume you are referring to the tabs.saveAsPDF() API as opposed to the
> browser.print() call ?
> 
> Neither myself, Shane Caraveo or Bob Owen could think of a way of testing
> this API.

Yes. It should probably be enough to add a print progress listener and check that a PDF file with a valid magic number is created in the expected location. Checking that the contents are correct is probably unnecessarily complex. But it could probably done with some sort of reftest, if necessary.

Comment 7

[dw-dev]

Kris,

> It should probably be enough to add a print progress listener and check 
> that a PDF file with a valid magic number is created in the expected location.

When we looked at testing this before, there were several issues:

1. Which page should be loaded into the tab before saving?  Where would it be loaded from?

2. Where should the page be saved to?

3. How could we programmatically respond to the 'Save As' dialog without user interaction?

We thought about checking that a non-zero size file was created in the correct location. Checking that the file has a valid magic number is better. For a PDF file, I assume we would check that the file starts with "%PDF" (hex 25 50 44 46)?

Comment 8

[dw-dev]

Kris,

One other issue. When we were thinking about testing tabs.saveAsPDF(), I did some testing with my original Print Edit add-on, and it appeared that the saved file was created after the print progress listener returned the completed status.

Comment 9

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #4)
> Bob,
> 
> > Oh, this is because we converted the FrameLoader bindings to WebIDL, which
> > doesn't allow arguments to be null by default. If that argument is allowed
> > to be null, we need to add the `optional` keyword and add `?` after its type.
> 
> Is the aPrintProgressListener argument allowed to be null?

Looks like Kris beat me to it, but yes it is.

I think we actually have some printing tests now, but I don't know if they use this API and they still have to avoid native dialogs.
Of course in addition to the suggested test, I guess we'd also need some dummy test to call it with a null progress listener to cover this case.

I can't remember what (if any) native dialogs are involved here, we might be able to pass something in the print settings for the file name, but I'm not sure.

Comment 10

[Kris Maglione [:kmag]]

(In reply to dw-dev from comment #7)
> When we looked at testing this before, there were several issues:
> 
> 1. Which page should be loaded into the tab before saving?  Where would it
> be loaded from?

Any page should do.

> 2. Where should the page be saved to?

Probably the temp directory.

> 3. How could we programmatically respond to the 'Save As' dialog without
> user interaction?

We have a MockFilePicker stub for this: http://searchfox.org/mozilla-central/source/testing/specialpowers/content/MockFilePicker.jsm

> We thought about checking that a non-zero size file was created in the
> correct location. Checking that the file has a valid magic number is better.
> For a PDF file, I assume we would check that the file starts with "%PDF"
> (hex 25 50 44 46)?

Yup, that would probably be sufficient.

Comment 11

[dw-dev]

I have tried two prototype solutions - but they both cause the tab to crash - and I've no idea why!

FIRST PROTOTYPE

I took up Kris's suggestion in Comment 3 and added an `optional` keyword and a `?` after its type.

So it now looks like this in mozilla-central\dom\webidl\FrameLoader.webidl:

> 
>  /**
>   * Print the current document.
>   *
>   * @param aOuterWindowID the ID of the outer window to print
>   * @param aPrintSettings optional print settings to use; printSilent can be
>   *                       set to prevent prompting.
>   * @param aProgressListener optional print progress listener.
>   */
>  [Throws]
>  void print(unsigned long long aOuterWindowID,
>             nsIPrintSettings aPrintSettings,
>             optional nsIWebProgressListener? aProgressListener);
> 

SECOND PROTOTYPE

I added a print progress listener inside the tabs.saveAsPDF() API.

So it now looks like this in mozilla-central\browser\components\extensions\ext-tabs.js:

> 
> var printProgressListener = {
>   QueryInterface: function(aIID) {
>     if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
>         aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
>         aIID.equals(Components.interfaces.nsISupports))
>         return this;
>     throw Components.results.NS_NOINTERFACE;
>   },
>   onLocationChange : function(aWebProgress, aRequest, aLocation, aFlags) {},
>   onProgressChange : function (aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {},
>   onSecurityChange : function(aWebProgress, aRequest, aState) {},
>   onStateChange: function(aWebProgress, aRequest, aFlags, aStatus) {
>     if ((aFlags & Components.interfaces.nsIWebProgressListener.STATE_STOP) &&
>         (aFlags & Components.interfaces.nsIWebProgressListener.STATE_IS_DOCUMENT)) {
>       console.log("onStateChange");
>       resolve(retval == 0 ? "saved" : "replaced");
>     }
>   },
>   onStatusChange : function(aWebProgress, aRequest, aStatus, aMessage) {
>     if (aStatus != 0) {
>       console.log("onStatusChange");
>       resolve(retval == 0 ? "not_saved" : "not_replaced");
>     }
>   }
> };
> 
> activeTab.linkedBrowser.print(activeTab.linkedBrowser.outerWindowID, printSettings, printProgressListener);
> 

*************************************

 Any ideas on what I am doing wrong?

*************************************

Comment 12

[dw-dev]

With regards to Comment 10, is there any documentation on how to use the MockFilePicker stub or an example where is hace been used in a WebExtensions API test?

Comment 13

[Kris Maglione [:kmag]]

(In reply to dw-dev from comment #11)
> I have tried two prototype solutions - but they both cause the tab to crash
> - and I've no idea why!

Do you have a crash report?

(In reply to dw-dev from comment #12)
> With regards to Comment 10, is there any documentation on how to use the
> MockFilePicker stub or an example where is hace been used in a WebExtensions
> API test?

Not really, but there are a bunch of existing in-tree uses: http://searchfox.org/mozilla-central/search?q=MockFilePicker&case=true&path=

Comment 14

[dw-dev]

I've done some more testing of the Second Prototype (with print progress listener).  If I disable e10s, then the PDF file is saved and the tab doesn't crash.  However, the completion status isn't being notified back to my add-on, so I need to investigate that.

So this looks like an e10s problem.

Should I upload the crash report on this site or just send it to Mozilla through the Crash Reporter?

Comment 15

[dw-dev]

Kris,

I've made some progress with the print progress listener (Second Prototype).

I now have a version of tabs.saveAsPDF() with a print progress listener that works perfectly with the 'Enable multi-process Nightly' option disabled.

With the 'Enable multi-process Nightly' option enabled, the tab crashes with the message "Gah. Your tab just crashed."

I've looked on Help > Troubleshooting Information, but there are no Crash Reports recorded.

I'm not sure how to progress this investigation further.

I can't see why e10s makes any difference, because tabs.saveAsPDF() is called from the background script in Print Edit WE, and I assume that the print progress listener does not need to communicate with the tab content process.

Any suggestions?

Comment 16

[dw-dev]

Andy, Shane, Bob,

The tabs.saveAsPDF() is working fine in Firefox 56, but no longer works in Firefox 57.

The reason that tabs.saveAsPDF() no longer works in Firefox 57 is because the FrameLoader.print() binding has been converted to WebIDL and no longer accepts a 'null' 3rd argument.

I have prototyped adding a print progress listener (nsIWebProgessListener) as the 3rd argument and this works correctly if e10s is disabled, but crashes the tab if e10s is enabled.

I need to move this forward quickly, but am stuck at the moment, because:

- I don't know how to generate a Crash Report for the crashed tab.
- I'm not familiar with WebIDL or how to investigate problems with WebIDL

I suspect the issue is something to do with the WebIDL mapping of the Javascript FrameLoader.print() nsIWebProgessListener interface onto the underlying C++ implementation.  Although I can't see why e10s would make a difference.

Do you have any sugestions?

Comment 17

[Bob Owen (:bobowen)]

It appears that, nearly all the time, the PBrowser::Msg_Print__ID is getting processed before the PPrinting::Msg_PRemotePrintJobConstructor__ID despite the fact that it is sent first (or at least the Send is called first).

As the Print needs the RemotePrintJob to have been created in the child it blows up.

billm - I thought we had guarantees over the processing order of messages for precisely this problem, has that changed?

Comment 18

[dw-dev]

Andy,

Bearing in mind that:

    - that tabs.saveAsPDF() is a documented WebExtensions API
    - that several add-ons are now using this interface
    - that I have 3,000 users of my Print Edit WE add-on, and I expect this to grow rapidly in the coming weeks

I am concerned that fixing this problem is only considered Priority P2.

It really needs to be fixed in Firefox 57.

Comment 19

[Kris Maglione [:kmag]]

I agree, this seems like a P1 regression. It's unfortunate we didn't have tests to catch it before it hit beta.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bob Owen (:bobowen) from comment #17)
> It appears that, nearly all the time, the PBrowser::Msg_Print__ID is getting
> processed before the PPrinting::Msg_PRemotePrintJobConstructor__ID despite
> the fact that it is sent first (or at least the Send is called first).
> 
> As the Print needs the RemotePrintJob to have been created in the child it
> blows up.
> 
> billm - I thought we had guarantees over the processing order of messages
> for precisely this problem, has that changed?

That does sound like an issue caused by Quantum DOM scheduling. We now have different event queues for each tab, as well as a "system" event queue. There aren't any ordering guarantees between these queues. Any message in PBrowser is automatically opted into the tab event queue. And it looks like PPrinting was placed in the SystemGroup queue:
http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/toolkit/components/printingui/ipc/nsPrintingProxy.cpp#66-68

I think the right fix for this is to back out bug 1351190. That will cause the PPrinting message to be unlabeled, which means it will be ordered wrt all other events, including the PBrowser message.

Sorry about this.

Comment 21

[Bob Owen (:bobowen)]

Dave, looks like bug 1351190 will get backed-out soon and you should then be able to move forward.

I think you should probably go with both of your proposed fixes, so the progress listener becomes optional again (both assumes that having the progress listener as part of the API is useful, if not then just go with making it optional).

Comment 22

[Andy McKay]

The criteria for uplift into Firefox 57 is extremely high and I don't think a possible problem for 3,000 users (or even 10,000) is high enough. It's a shame it wasn't caught earlier, but I've been told 
"At this point, the rough bar for uplifted P1s for 57 should be things that you feel would drive a dot release in 57."

Comment 23

[dw-dev]

I agree it's a shame we didn't have tests to catch this, but the issue was highlighted and no-one offered suggestions on testing of tabs.saveAsPDF() could be performed.

Comment 24

[Marcia Knous [:marcia]]

Won't fixing for 57. Andy's comment 22 is spot on. Let's try to get this addressed for 58.

Comment 25

[Bob Owen (:bobowen)]

> "At this point, the rough bar for uplifted P1s for 57 should be things that you feel would drive a dot release in 57."

I agree we should be extra careful with Fx57 because of the spotlight on it, but I genuinely think that this bar is too high at this point, given we still have 4 weeks until the release candidate.

If that is really the current policy that would mean we will take almost no other fixes into 57, even if they are clear regressions and simple fixes.
I think we risk shipping lots of regressions that affect only a relatively small number of users, adding up to larger dissatisfaction.

Also, the web extension versions of popular addons are likely to get significant downloads throughout the lifetime of 57.
I suspect that the number of people this could affect (measured by people who use an extension that uses this API) will end up being in the many 10s of thousands at least.

Comment 26

[Marcia Knous [:marcia]]

Adding ni on Ritu as she is leading the 57 release and can address Comment 25.

Comment 27

[dw-dev]

As expected, the number of users of Print Edit WE has grown rapidly, and is now around 27,000.

As far as I can tell, Bug 1351190 has not yet been backed out of mozilla-central.  The tab is still crashing with e10s enabled.

I have tried making the "optional nsIWebProgressListener? aProgressListener" change in FrameLoader.webidl.

With e10s disabled, tabs.saveAsPDF() now works with a print progress listener or with a null argument.

Starting work on the automatic test for tabs.saveAsPDF().

Comment 28

[Ryan VanderMeulen [:RyanVM]]

The backout for bug 1351190 should be in currently Nightly builds and will go out in 57b7 tomorrow. Would be good to verify that it has indeed fixed this crash.

Comment 29

[Bob Owen (:bobowen)]

That backout was required to fix this bug, but it will require some other changes as well.
I verified that it fixed the crash that dw-dev was seeing with his fixes.

Comment 30

[dw-dev]

Kris,

I am struggling with writing the automatic test for tabs.saveAsPDF().

The key issue is how to import the MockFilePicker.jsm file which is a special powers module.

The examples I have found elsewhere, use either:

> var MockFilePicker = SpecialPowers.MockFilePicker;

or:

> const { MockFilePicker } =
>   Cu.utils.import("chrome://specialpowers/content/MockFilePicker.jsm", {});

However, neither of these work, and in both cases MockFilePicker is undefined.

What is the right way to get access to the MockFilePicker ?

Comment 31

[Kris Maglione [:kmag]]

SpecialPowers.MockFilePicker should work. Where are you running this, exactly?

`Cu.utils` is undefined, so that code should throw.

Comment 32

[dw-dev]

Attachment: Test for tabs.saveAsPDF() - browser_ext_tabs_saveAsPDF.js

Comment 33

[dw-dev]

Kris,

PROGRESS UPDATE

1. The backing out of Bug 1351190 has fixed the tab crash problem.

2. tabs.saveAsPDF() now works with/without e10s and with/without a progress listener - I have tested all 4 combinations.

3. I tried adding 'optional' for aProgressListener in FrameLoader.webidl, but 'mach build' threw this error:

> 1:48.28 d:\mozilla-central\obj-i686-pc-mingw32\dom\bindings\FrameLoaderBinding.cpp(886): error C2664: 'void nsFrameLoader::Print(uint64_t,nsIPrintSettings *,nsIWebProgressListener *,mozilla::ErrorResult &)': cannot convert argument 3 from 'const mozilla::dom::Optional<nsIWebProgressListener *>' to 'nsIWebProgressListener *'
> 1:48.28 d:\mozilla-central\obj-i686-pc-mingw32\dom\bindings\FrameLoaderBinding.cpp(886): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

4. I have added '?' for aProgressListener in FrameLoader.webidl, and this works fine.

5. This means that the 3rd argument to FrameLoader.print() can be 'null', but not missing completely.


PROBLEMS WITH TEST FILE

6. I have created a skeleton test file (see attachment).

7. When I run the test, I get the following errors

> 10 INFO Console message: [JavaScript Warning: "XrayWrapper denied access to property 0 (reason: value not same-origin with target). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported." {file: "chrome://specialpowers/content/MockFilePicker.jsm" line: 138}]
> GECKO(16916) | JavaScript error: chrome://specialpowers/content/MockFilePicker.jsm, line 142: TypeError: Argument 1 of File.createFromNsIFile is not an object.

8. This error seems to relate to the file parameter passed to MockFilePicker.setFiles([saveFile]).

9. I still need to add a check that the file has bee created and the the first four bytes are '%PDF'.
 

*** Please can you advise how the error described in 7 & 8 above can be avoided? ***

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I've pinged Andy about this one. If there is a fix that is low risk, we may consider uplifting in 57. 

Bob, it may seem the 57 uplift bar is unusually high but we've accepted over 300 uplifts so far, despite the recommendation to only consider requesting uplift for issues that are really severe. This is 2-3x churn as compared to a regular beta cycle. This kind of code churn poses a significant risk to quality, stability and schedule. We are doing our best to carefully weigh the risk vs reward for each uplift. Thank you for your support!

Comment 35

[Kris Maglione [:kmag]]

(In reply to dw-dev from comment #33)
> 3. I tried adding 'optional' for aProgressListener in FrameLoader.webidl,
> but 'mach build' threw this error:

It needs to be `optional nsIWebProgressListener? aProgressListener = null` so
the binding layer automatically passes null when the argument isn't provided
rather than passing an Optional<> instead.

> 7. When I run the test, I get the following errors
> 
> > 10 INFO Console message: [JavaScript Warning: "XrayWrapper denied access to property 0 (reason: value not same-origin with target). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported." {file: "chrome://specialpowers/content/MockFilePicker.jsm" line: 138}]
> > GECKO(16916) | JavaScript error: chrome://specialpowers/content/MockFilePicker.jsm, line 142: TypeError: Argument 1 of File.createFromNsIFile is not an object.

I'm not exactly sure what's going on there, but you really should be doing the
MockFilePicker stuff from outside of the extension code.

The background page will run in a child process on Windows, so it won't have
direct filesystem access, or the ability to override the file picker, in any
case.

Comment 36

[dw-dev]

Kris,

> I'm not exactly sure what's going on there, but you really should be doing the
> MockFilePicker stuff from outside of the extension code.

I've already tried putting the the MockFilePicker stuff:

- right at the beginning, immediately after:  the "use strict";

- inside the add_task, immediately after: await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/");

BUT in both cases the MockFilePicker is undefined.


Any ideas why?

Comment 37

[dw-dev]

Attachment: Bug 1404681 - change to FrameLoader.print();

Dependent on backout of Bug 1351190 to avoid tab crash - already done

Made 3rd argument to FrameLoader.print() optional so that it can be null or omitted.

Added print progress listener to call to FrameLoader.print() in tabs.saveAsPDF().

Created test file for tabs.saveAsPDF() to verify that PDF file actually saved.

Review commit: https://reviewboard.mozilla.org/r/188428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188428/

Comment 38

[dw-dev]

Quick commentary on the patch:

1. I've made the "optional nsIWebProgressListener? aProgressListener = null" change in FrameLoader.webidl.

2. I've tested calling FrameLoader.print() with 3rd argument as print progress listener or 'null' or omitted completely - using both e10s and non-e10s. With all six combinations of conditions, the PDF file was saved correctly.

3. The call to FrameLoader.print() in tabs.saveAsPDF() now has a print progress listener, so that the PDF file has definitely been saved by the time the 'saved' status is returned.

4. The test file for tabs.saveAsPDF() has been created and verifies that the PDF file has actually been saved.  All tests are passed.

5. All of the files have been checked with eslint.

6. I believe that a change to FrameLoader.webidl will require review by a DOM peer - would that be Bob ?

Comment 39

[Bob Owen (:bobowen)]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

https://reviewboard.mozilla.org/r/188428/#review194044

I'm not a DOM peer, sorry.
Kris - possibly makes sense to pass the webidl review to whoever did the original one.

Comment 40

[Kris Maglione [:kmag]]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

https://reviewboard.mozilla.org/r/188428/#review194216

Looks good for the most part, thanks. Just some relatively minor issues.

Please ask smaug for review on the WebIDL parts. And it might be better to move that to a separate patch.

::: browser/components/extensions/ext-tabs.js:846
(Diff revision 1)
>                    let fstream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
> -                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // write|create|truncate, file permissions rw-rw-rw- = 0666 = 0x1B6
> +                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // ioflags = write|create|truncate, file permissions = rw-rw-rw-
>                    fstream.close();  // unlock file

What's the purpose of this?

::: browser/components/extensions/ext-tabs.js:847
(Diff revision 1)
>              picker.open(function(retval) {
>                if (retval == 0 || retval == 2) {
>                  // OK clicked (retval == 0) or replace confirmed (retval == 2)
>                  try {
>                    let fstream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
> -                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // write|create|truncate, file permissions rw-rw-rw- = 0666 = 0x1B6
> +                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // ioflags = write|create|truncate, file permissions = rw-rw-rw-

Nit: Please use 0o666 rather than 0x1B6.

::: browser/components/extensions/ext-tabs.js:922
(Diff revision 1)
> -
> +                  QueryInterface: function(iID) {
> +                    if (iID.equals(Ci.nsIWebProgressListener) || iID.equals(Ci.nsISupportsWeakReference) || iID.equals(Ci.nsISupports)) {
> +                      return this;
> +                    }
> +                    throw Cr.NS_NOINTERFACE;
> +                  },

Nit: `XPCOMUtils.generateQI([Ci.nsIWebProgressListener])`

Using a weak reference here probably doesn't make sense, since we don't have anything holding this alive, and it will wind up being freed if the cycle collector runs before it finishes.

Is there a particular reason this needs to support weak references?

::: browser/components/extensions/ext-tabs.js:928
(Diff revision 1)
> +                  onLocationChange: function(webProgress, request, location, flags) {},
> +                  onProgressChange: function(webProgress, request, curSelfProgress, maxSelfProgress, curTotalProgress, maxTotalProgress) {},
> +                  onSecurityChange: function(webProgress, request, state) {},

Nit:

    onLocationChange(webProgress, ...) {},
    ...

::: browser/components/extensions/ext-tabs.js:938
(Diff revision 1)
> -                resolve(retval == 0 ? "saved" : "replaced");
> +                      resolve(retval == 0 ? "saved" : "replaced");
> +                    }
> +                  },
> +                  onStatusChange: function(webProgress, request, status, message) {
> +                    if (status != 0) {
> +                      resolve(retval == 0 ? "not_saved" : "not_replaced");

This should reject rather than resolve.

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:8
(Diff revision 1)
> +  let saveDir = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> +  saveDir.append("testsavedir");
> +  if (!saveDir.exists()) {
> +    saveDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0x1ED);  // file permissions rwxr-xr-x = 0755 = 0x1ED
> +  }

Nit:

    FileUtils.getDir("TmpD", [`testSaveDir-${Math.random()}`], true);

Also, please add something like:

    registerCleanupFunction(() => {
      saveDir.remove(true);
    });

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:56
(Diff revision 1)
> +    let fstream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +    fstream.init(saveFile, 0x01, 0x124, 0);  // ioflags = read, file permissions = r--r--r--
> +
> +    let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);
> +    cstream.init(fstream, "UTF-8", 0, 0);
> +
> +    cstream.readString(4, buffer);
> +    cstream.close();

Please use `OS.File.read` for this instead. We're trying to get away from using XPCOM streams in JS.

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:68
(Diff revision 1)
> +    cstream.close();
> +
> +    match = (buffer.value.substr(0, 4) == "%PDF");
> +  } catch (e) { }
> +
> +  is(match, true, "PDF file verified");

Nit: `is(data.slice(0, 4), "%PDF", "Got correct magic number");`

Comment 41

[dw-dev]

Kris,

Okay, I will make these changes, but there is one comment I don't understand ...

(Diff revision 1)
> +                  onLocationChange: function(webProgress, request, location, flags) {},
> +                  onProgressChange: function(webProgress, request, curSelfProgress, maxSelfProgress, curTotalProgress, maxTotalProgress) {},
> +                  onSecurityChange: function(webProgress, request, state) {},

> Nit:
> 
>     onLocationChange(webProgress, ...) {},
>     ...

What exactly do you want changed ?


I will remove the change in FrameLoader.webidl from this patch.  We don't need it to make tabs.saveAsPDF() work correctly, since FrameLoader.print() is called with a print progress listener (not null or omitted).

Comment 42

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #41)
...
> > +                  onLocationChange: function(webProgress, request, location, flags) {},
> > +                  onProgressChange: function(webProgress, request, curSelfProgress, maxSelfProgress, curTotalProgress, maxTotalProgress) {},
> > +                  onSecurityChange: function(webProgress, request, state) {},
> 
> > Nit:
> > 
> >     onLocationChange(webProgress, ...) {},
> >     ...
> 
> What exactly do you want changed ?

Pretty sure he just means that you don't need the ": function" any more, see [1]

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

Comment 43

[dw-dev]

Bob, Thanks.

Comment 44

[dw-dev]

Kris,

With regards to your question ...

(Diff revision 1)
>                    let fstream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);
> -                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // write|create|truncate, file permissions rw-rw-rw- = 0666 = 0x1B6
> +                  fstream.init(picker.file, 0x2A, 0x1B6, 0);  // ioflags = write|create|truncate, file permissions = rw-rw-rw-
>                    fstream.close();  // unlock file

> What's the purpose of this?

Before answering, I needed to do some testing to make sure that this workaround is still necessary.

Consider the case where a user is trying to replace an existing PDF file, but that file is open in another application, such as Adobe Acrobat Reader, and is therefore locked.  In this case, the print progress listener is never called, and so tabs.saveAsPDF() would not return a status to the calling add-on.  This is a bug that has existed since at least May 2014 when I first added this workaround into my original Print Edit add-on.  My testing has confirmed that this bug still exists.

With this workaround, a correct status is always returned to the calling add-on, so I would prefer not to change it.

I could add a comment to explain why this workaround is there.

Are you okay with this?

Comment 45

[dw-dev]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188428/diff/1-2/

Comment 46

[dw-dev]

Kris,

Commentary on revised patch:

1. I have removed the change in FrameLoader.webidl from this patch, since we don't need for tabs.saveAsPDF().

2. In Comment 44, I have explained why we need the 'locked file' workaround in tabs.saveAsPDF().

3. I have made all the changes you suggested in your review, except for changing 'resolve' to 'reject'.  When I first implemented tabs.saveAsPDF(), there was a lengthy discussion with Shane Caraveo about whether the 'not_saved' and 'not_replaced' cases should be handled as exceptions (reject) or just as returned statuses.  We agreed they should be handled as returned statuses, and this is how the interface is documented. Not being able to save a file for legitimate reasons, such as a file being locked or having restricted permissions, is not really an exception.  The calling add-on will be able to decide what action to take based on the returned status.

I assume this is this okay?

Comment 47

[Kris Maglione [:kmag]]

(In reply to dw-dev from comment #46)
> Kris,
> 
> Commentary on revised patch:
> 
> 1. I have removed the change in FrameLoader.webidl from this patch, since we
> don't need for tabs.saveAsPDF().

You should probably still make that change, but in a separate patch. That on its own should be enough to fix this API, and should be upliftable to beta. The rest of your changes are not upliftable, unfortunately.

Comment 48

[Marcia Knous [:marcia]]

Tracking 58+ based on Comment 19.

Comment 49

[dw-dev]

Kris,

I have been away for a few days and not been able to work on this.  I have now produced a new patch that just makes the changes to the FrameLoader.print() WebIDL declaration in FrameLoader.webidl.

When I did the patch commit, I asked for reviews by you and smaug, but when I tried to push the patch for review, I got an error message

> Bugzilla error: Olli Pettay [:smaug] (work week Oct 16-20) <[e-mail]> is not currently accepting 'review' requests. (HTTP 500, API Error 225)

How should I progress this review ?

Comment 50

[dw-dev]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188428/diff/2-3/

Comment 51

[dw-dev]

I have pushed the patch again and this time everything worked fine.

When this patch has been approved, I will request uplift to Firefox 57.

After that, I will submit another patch for Firefox 58, which will add the print progress listener and automated test file.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

https://reviewboard.mozilla.org/r/188428/#review197606

Comment 53

[dw-dev]

Kris,

Please can you review the latest patch for the change to FrameLoader.print() in FrameLoader.webidl.

I assume I can then request uplift by setting 'approval‑mozilla‑beta'?

Comment 54

[Bob Owen (:bobowen)]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

To be honest I don't think you need Kris for this as well, I've pushed a try run.

Comment 55

[Bob Owen (:bobowen)]

I think you'll need to resolve those old issues from the previous patch in mozreview, before I can push this.

Comment 56

[dw-dev]

The other changes in the previous patch should not be not necessary.  The latest patch should work fine on its own.  The existing call to FrameLoader.print() from tabs.saveAsPDF() has a 'null' 3rd argument (print progress listener).

Kris wants the latest patch uplifted to Firefox 57 on its own (see Comment 47).

When that has been done, I was going to submit another patch with the other changes for Firefox 58.

Is the problem that I am submitting 2 patches for the same bug?

Comment 57

[Bob Owen (:bobowen)]

It's just that this patch replaced the old one on mozreview, but it still has 8 issues against it.
They need to be set to resolved before we can push it to autoland.

Comment 58

[dw-dev]

Bob,

I sort of understand what you are saying, but I am not really familiar with the mozreview process/tools.

I thought that the previous patch was history, since I backed it out of my local repository before creating the latest patch.

Exactly what needs to be done to set the 8 issues to resolved?  And who needs to do it?

Comment 59

[Bob Owen (:bobowen)]

I'm hoping you can.
If you go to here, do you see an option to resolve each one:
https://reviewboard.mozilla.org/r/188428/#issue-summary

Comment 60

[dw-dev]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

https://reviewboard.mozilla.org/r/188428/#review198268

The 8 issues raised by Kris on the previous patch have been dropped because they are not relevant to the latest patch.

However, these issues are fixed and will be reviewed again when the next patch for Firefox 58 is reviewed.

Comment 61

[dw-dev]

Bob,

I have marked the 8 issues as dropped, since they will be reviewed again in the next patch.

Please go ahead and push the current patch that smaug approved.

Presumably, the uplift request will be the next thing.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b73eab93e819
change to FrameLoader.print(); r=smaug

Comment 63

[Bob Owen (:bobowen)]

Thanks Kris

Comment 64

[Attila Craciun [:arny]]

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

Comment 65

[dw-dev]

Based on Kris's Comment 47, this separate patch for the change in FrameLoader.webidl should be upliftable to Firefox 57 beta.

Can I go ahead and uplift by setting 'approval‑mozilla‑beta'?

Comment 66

[Marcia Knous [:marcia]]

(In reply to dw-dev from comment #65)
> Based on Kris's Comment 47, this separate patch for the change in
> FrameLoader.webidl should be upliftable to Firefox 57 beta.
> 
> Can I go ahead and uplift by setting 'approval‑mozilla‑beta'?

Yes, please do that and Ritu will review it.

Comment 67

[Kris Maglione [:kmag]]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1391110
[User impact if declined]: This issue breaks the main functionality of an add-on with tens of thousands of users.
[Is this code covered by automated tests?]: No, but tests are being added in a follow-up.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Reporter should verify.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is a trivial, one-line change to an IDL file.
[String changes made/needed]: None.

Comment 68

[dw-dev]

I have tested and verified this patch on Nightly 58.0a1 (2017-10-26) using my Print Edit WE 20.0 add-on.

Saving to PDF file works correctly.

Comment 69

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917443 [details]
Bug 1404681 - change to FrameLoader.print();

This seems like a low risk change, taking it. Beta57+

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/362e0a86dac7

Comment 71

[dw-dev]

Kris,

Two questions:

1. Is it okay to go ahead and submit a second patch with the other changes (print progress listener and automated test) using this same bug number?  If so, do I need to re-open the bug?  If so, how would I do that?

2. On a different topic.  Is there a way to make progress on Bug 1318532 which would support a full-function WebExtensions version of my popular legacy Tile Tabs add-on?  I have developed a Tile Tabs WE add-on, but this is receiving a lot of negative reviews due to the limitations of the WebExtensions APIs.  I'm fairly sure that there will be thousands of unhappy users as a result of losing legacy Tile Tabs when Firefox 57 is released.  Please can you take a look at my comments on Bug 1318532 (https://bugzilla.mozilla.org/show_bug.cgi?id=1318532#c33).

Comment 72

[Julien Cristau [:jcristau]]

Best to make a new bug for followup patch.