1189846 - Print Edit 15.10 add-on no longer works with e10s and sandboxing enabled

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150730030208

Steps to reproduce:

I am the author of the "Print Edit" add-on. One of the features of Print Edit is to save the print preview as a PDF file.

This feature relies on the nsIWebBrowserPrint print() function and worked in previous versions of Firefox up to and including 41.0a1 (in both non-e10s and e10s windows).

With Firefox Nightly 42.0a1, nsIWebBrowserPrint print() works in non-e10s windows, but does not work in e10s windows.

Here is the relevant extract from the Print Edit source code:

        /* Initialize print settings */
        
        printSettings = printEdit.getPrintSettings(true);
        
        printSettings.printToFile = true;
        printSettings.toFileName = message.json.filepath;
        
        printSettings.printSilent = true;  /* no print dialog */
        printSettings.showPrintProgress = false;  /* no print progress */
        
        printSettings.printFrameType = Components.interfaces.nsIPrintSettings.kFramesAsIs;  /* print frames as is */
        printSettings.outputFormat = Components.interfaces.nsIPrintSettings.kOutputFormatPDF;  /* output in PDF format */
        
        /* Print to PDF file */
        
        try
        {
            docShell.printPreview.print(printSettings,printEdit.printProgressListener);
        }
        catch (e)
        {
            sendAsyncMessage("printedit-cmdsavepdf-reply",{ errorcode: "SPDF-2" },{ });
            return;
        }



Actual results:

With Firefox Nightly 42.0a1, in an e10s window, nsIWebBrowserPrint print() throws an exception.


Expected results:

With Firefox Nightly 42.0a1, in an e10s window, nsIWebBrowserPrint print()  should work.

Comment 1

[Alice0775 White]

Could you install the tool Mozregression (see http://mozilla.github.io/mozregression/ for details) and find a regression range.

Comment 2

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

What is the exception that the print method throws?

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=96862be5fb53&tochange=72e664840cf4

Triggered by 72e664840cf4	Bob Owen — Bug 1151767: On Windows, make level 1 the default content sandbox on Nightly. r=blassey


WORKAROUND:
security.sandbox.content.level = 0 in about:config

Comment 4

[dw-dev]

CORRECTION

In my original bug report I said that saving the print preview as a PDF file, using the nsIWebBrowserPrint print() function, worked in previous versions of Firefox up to and including 41.0a1 (in both non-e10s and e10s windows). I should have said 40.0a1 (not 41.0a1).

REGRESSION TESTING

I have manually performed regression testing in e10s windows using previous "mozilla-central" builds.

In an e10s window, saving the print preview as a PDF file works with the "2015-04-29 03-02-02 mozilla-central" build, but fails with the "2015-04-30 03-02-01 mozilla-central" build.

When it fails the exception message is:

Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPrint]

Comment 5

[dw-dev]

Alice, I have tried the "security.sandbox.content.level = 0" workaround and it resolves the problem.

Please can you confirm that "security.sandbox.content.level = 0" is the default setting for Firefox Aurora, Beta and Release channels?

Comment 6

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #5)
> Alice, I have tried the "security.sandbox.content.level = 0" workaround and
> it resolves the problem.
> 
> Please can you confirm that "security.sandbox.content.level = 0" is the
> default setting for Firefox Aurora, Beta and Release channels?

Yes, the low integrity sandbox is only enabled on Nightly at the moment.
This is largely because of print to file issues. :-)
The problem here is almost certainly that the code is trying to write the PDF file from the child process.
Running at low integrity only allows write access to a few special (low integrity) places.

If I understand correctly, you are just using hidden functionality within Gecko to print to PDF.
Which is interesting, because I understood from some of our graphics people that rendering to PDF wasn't supported on Windows.
However it possibly explains some of the printing code that I've seen.

This problem may well go away as part of my fix for bug 1156742.

The PDF rendering may also give us other options for that bug.
So I'll do some debugging with your addon next week, to work out what's going on.

Comment 7

[dw-dev]

My understanding is that with e10s the nsIWebBrowserPrint print() & printPreview() functions must be run in the content process.  Both Firefox and Print Edit call these functions from content process frame scripts.

Print Edit uses the "print to PDF" capability in Cairo graphics library (included in Gecko). I believe this is the same way that PDF's are generated in Mobile Firefox (Android).  Unfortunately, the Cairo graphics library only supports PDF Version 5, not later versions such as Version 7, which would have been nice.

The "print to PDF" capability works fine with:
 - Firefox 4 or later on Windows XP.
 - Firefox 28 or later on Windows Vista/7/8.
 - Firefox 4 - 27 on Windows Vista/7/8, if Firefox is run in Windows XP compatibility mode, or if hardware acceleration is disabled in Firefox Tools > Options> Advanced > General.

Print Edit also makes Firefox's print preview functionality available on Mac OS X, although for some reason the "print to PDF" capability does not work, but this is not really an issue because of the built-in "print to PDF" support in Mac OS X.

Comment 8

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

Where might we get a copy of your add-on to test?

Comment 9

[dw-dev]

Attachment: Pre-Release version of Print Edit 15.10

This is the final pre-release version of Print Edit 15.10, which will be submitted to Mozilla for review and release in the next few days.

Comment 10

[dw-dev]

I have uploaded the  final pre-release version of Print Edit 15.10 as an attachment. 

This version is fully up to date with the latest changes in PrintUtils.js and browser-content.js (e.g. passing content window as outerWindowID) and will be submitted to Mozilla for review and release in the next few days.

With "security.sandbox.content.level = 0", using the "Save PDF" feature in print preview works fine.

With "security.sandbox.content.level = 1", using the "Save PDF" feature causes nsIWebBrowserPrint to throw an 0x80004005 (NS_ERROR_FAILURE) exception, which Print Edit traps and displays as an "SPDF-2" error message.

Comment 11

[Bob Owen (:bobowen)]

I've had a look at this and it is just going through, what I believe to be an old branch of the code because the output format is set to kOutputFormatPDF:
https://dxr.mozilla.org/mozilla-central/rev/4e883591bb5dff021c108d3e30198a99547eed1e/widget/windows/nsDeviceContextSpecWin.cpp#281

As this is all in the child process, the sandbox means that the file cannot be saved.
I don't know if we once offered a save to PDF option on Windows and that is why this code exists. 

The changes that I'm doing for bug 1156742 aren't going to fix this.

We would either have to send the PDF itself up to the parent or maybe save the file in a low integrity area and then ask the parent to move it.
Alternatively we would need to find a way to convert the EMF(s) that we are thinking of using to PDF in the parent.

Comment 12

[dw-dev]

Just wondering if there is any update on this?

My "Print Edit" add-on has over 100,000 users, many of whom use the 'Save As PDF' feature.

This feature relies on calling nsIWebBrowserPrint print() with the 'kOutputFormatPDF' setting.

Some users are beginning to notice that this feature no longer works with Nightly.

Will users still be able to set 'security.sandbox.content.level' to '0' to make this feature work in future release versions of Firefox?

Comment 13

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #12)
> Just wondering if there is any update on this?
> 
> My "Print Edit" add-on has over 100,000 users, many of whom use the 'Save As
> PDF' feature.
> 
> This feature relies on calling nsIWebBrowserPrint print() with the
> 'kOutputFormatPDF' setting.
> 
> Some users are beginning to notice that this feature no longer works with
> Nightly.

The current way I was looking fixing this for other reasons was to use EMF files to transport the page print instructions over to the parent process.
As these would be played straight to the printer device, it would mean this wouldn't fix things for Print Edit as it would still be trying to save things from the content process.

I'm looking into a cross-platform approach to transport the instructions that may well work for Print Edit as well.
I'll update here once I've got it working.

If that doesn't work for Print Edit, I'll look into other solutions.

> Will users still be able to set 'security.sandbox.content.level' to '0' to
> make this feature work in future release versions of Firefox?

At some point we will start hard coding a minimum level, as setting the pref lower will be a security risk.
There is an environment variable to turn it off and we'll probably add one to allow a lower pref to be set, but these should only really be used for testing and it would be inadvisable to run with them set all the time.

Comment 14

[dw-dev]

Bob,  Thanks for this update. Hoping that you can find a solution that will work for Print Edit.

Comment 15

[dw-dev]

Bob,

Please can you provide an update.

Is there any progress on finding and implementing a solution for this problem?

The ability to save edited (or original) web pages to PDF is an important feature for users of Print Edit - and I really don't want to lose this featur in future versions of Firefox.

Dave

Comment 16

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #15)
> Bob,
> 
> Please can you provide an update.
> 
> Is there any progress on finding and implementing a solution for this
> problem?
> 
> The ability to save edited (or original) web pages to PDF is an important
> feature for users of Print Edit - and I really don't want to lose this
> featur in future versions of Firefox.
> 
> Dave

Hi Dave,

Sorry this has been hanging for so long, it's taken a long time just to implement printing via the parent process on  Windows (bug 1156742).
That just has a few more reviews to get before I can land it.

Unfortunately, that fix doesn't automatically make Print Edit's use of the hidden print to PDF setting work straight away.
The problem is that Print Edit sends a message to the child process, where it constructs the print settings and sets it to print silently (no prompt to the user).
The way my change works is to construct a new RemotePrintJob in the parent process when the prompt is shown.
Without this you can't print via the parent, which is required to be able to save files in most places because of the sandbox on the child process.
We don't really want to let the child processes create RemotePrintJobs because of security concerns.

So, my idea is to add some sort of PrintWithoutPrompt function that can be called in the parent process for use by addons.
You would have to provide the settings and a RemotePrintJob would be constructed and added into the settings, before sending to be processed by the PrintEngine in the child.

Does this sound like something that would work for Print Edit?

Comment 17

[dw-dev]

Bob,

A few questions:

1. When you say "... when the prompt is shown.", I assume you are referring to the Print dialog box that is displayed after clicking on the Print button in print preview or after pressing Ctrl+P ?

2. The idea of calling a PrintWithoutPrompt function in the parent process and associating a RemotePrintJob sounds fine.  I assume the settings required by the PrintWithoutPrompt function would be the same as those previously provided by the child process (printToFile, printSilent, kOutputFormatPDF, etc) ?

3. When you say "... before sending to be processed by the PrintEngine in the child.", I assume that this sending would be done by the PrintWithoutPrompt function ?

4. Would it still be possible to pass a printProgressListener to the PrintEngine, and to receive onStateChange calls notifying completion ?  If so, would the printProgressListener reside in the the child or the parent process ?

Providing my assumptions are correct, and a printProgressListener is supported, then this sounds like a good solution which would work very well for Print Edit.

Dave

Comment 18

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #17)
> Bob,
> 
> A few questions:
> 
> 1. When you say "... when the prompt is shown.", I assume you are referring
> to the Print dialog box that is displayed after clicking on the Print button
> in print preview or after pressing Ctrl+P ?

Yes.
 
> 2. The idea of calling a PrintWithoutPrompt function in the parent process
> and associating a RemotePrintJob sounds fine.  I assume the settings
> required by the PrintWithoutPrompt function would be the same as those
> previously provided by the child process (printToFile, printSilent,
> kOutputFormatPDF, etc) ?

The idea is it would work as before, so should be the same yes.
Except the printSilent would automatically be set.
Although maybe this could just be a general Print function from the parent and the prompting could be left to whether that was set or not.
Either way it should work for Print Edit.

> 3. When you say "... before sending to be processed by the PrintEngine in
> the child.", I assume that this sending would be done by the
> PrintWithoutPrompt function ?

Correct.

> 4. Would it still be possible to pass a printProgressListener to the
> PrintEngine, and to receive onStateChange calls notifying completion ?  If
> so, would the printProgressListener reside in the the child or the parent
> process ?

Hmm, I need to look into that, I've not dealt much with the print progress listener, but I would definitely want to get that working.
I guess this would live in the parent (but I'm not sure).
Quickly looking at Print Edit for this currently I see that the listener in the child just sends a message to the parent on completion, so I guess it would work either way.

> Providing my assumptions are correct, and a printProgressListener is
> supported, then this sounds like a good solution which would work very well
> for Print Edit.

Excellent, I'll do some more investigation when I get a chance.

Comment 19

[dw-dev]

Bob,

Is there any update on the fix for this problem?

I am becoming concerned that a fix may not be available before e10s arrives in the mainstream Firefox release.

Dave

Comment 20

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #19)
> Bob,
> 
> Is there any update on the fix for this problem?
> 
> I am becoming concerned that a fix may not be available before e10s arrives
> in the mainstream Firefox release.
> 
> Dave

Sorry, still haven't had time to work on this.

We're planning on letting the low integrity sandbox ride the trains in Fx47.
So, I've got a couple of weeks to get this in.

Comment 21

[dw-dev]

Thanks for the update. Fingers crossed that you can find the time!

Comment 22

[Bob Owen (:bobowen)]

Attachment: Part 1: Move PrintSettings serialization into nsIPrintSettingsService

WIP

Comment 23

[Bob Owen (:bobowen)]

Attachment: Part 2: Make Android CreatePrintSettings work like other OS specific ones

WIP

Comment 24

[Bob Owen (:bobowen)]

Attachment: Part 3: Remove CreatePrintSettings nsIPrintOptions and use GetNewPrintSettings instead

WIP

Comment 25

[Bob Owen (:bobowen)]

Attachment: Part 4: Remove getPrinterPrefInt from nsIPrintOptions

WIP

Comment 26

[Bob Owen (:bobowen)]

Attachment: Part 5: Remove displayJobProperties from nsIPrintOptions

WIP

Comment 27

[Bob Owen (:bobowen)]

Attachment: Part 6: Remove GetNativeData from nsIPrintOptions

WIP

Comment 28

[Bob Owen (:bobowen)]

Attachment: Part 7: Create the PPrinting IPDL singleton in the parent not the child

WIP

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 9: Add ability to print from the parent process with settings and progress listener

WIP

Comment 30

[Bob Owen (:bobowen)]

Attachment: [e-mail]

Just added my WIP.

This is my hacked version of Print Edit where I've changed printedit-browser.js to call the new code.

There is no progress listener yet, so even though the print via the parent generally works it then hangs waiting for the response.

Creating the new cross process print progress listener will be Part 8.
I know what I need to do here, just haven't had time before my PTO.

First 6 patches are pretty small clean-up work, some of which I need and some of which I thought I should do while I'm in that area.

Comment 31

[dw-dev]

I've had a look at the hacked version of Print Edit and can see how this will work in future.

Looks good!

Will the new print progress listener be declared in the chrome process or the content process?

Comment 32

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #31)

> Will the new print progress listener be declared in the chrome process or
> the content process?

It'll be in the chrome process, I have this pretty much working.
I need to work out how failures are communicated.

You currently have a try block for failures that come directly from the call to print in the child.
I'm guessing if something goes wrong after that then you don't see it?

Comment 33

[dw-dev]

That's right, if the print doesn't throw an exception, and the print progress listener doesn't receive the (STATE_STOP && STATE_IS_DOCUMENT) flags, then things will hang.  If the print progress listener returned an error state in the flags, that would be easy to handle.

Will the print progress listener have to be declared in Print Edit or will it be handled behind the scenes? (in the latter case, I guess any errors could be communicated through a callback function)

Comment 34

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #33)
> That's right, if the print doesn't throw an exception, and the print
> progress listener doesn't receive the (STATE_STOP && STATE_IS_DOCUMENT)
> flags, then things will hang.  If the print progress listener returned an
> error state in the flags, that would be easy to handle.

The existing STATE_STOP && STATE_IS_DOCUMENT should still be there, but thinking of adding a call to OnStatusChange, when there's an error.
 
> Will the print progress listener have to be declared in Print Edit or will
> it be handled behind the scenes? (in the latter case, I guess any errors
> could be communicated through a callback function)

You'll have to declare it the same as now, just in the chrome script not content.
It'll be passed in that second parameter, where I'm passing null in the uploaded version.

Comment 35

[dw-dev]

That's fine.

Comment 36

[dw-dev]

Bob, Is there any progress on the cross process print progress listener (Part 8) ?

Comment 37

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #36)
> Bob, Is there any progress on the cross process print progress listener
> (Part 8) ?

Yes this is all sorted, I had to switch to something else for a while, but should have the patches up this week.

Comment 38

[dw-dev]

Great!

Comment 39

[Bob Owen (:bobowen)]

Attachment: Part 7: Create the PPrinting IPDL singleton in the parent not the child

MozReview-Commit-ID: 98g7eZUJ6bY

Comment 40

[Bob Owen (:bobowen)]

Attachment: Part 8: Make RemotePrintJob a print progress listener forwarder

MozReview-Commit-ID: 4ADQ6Hx46fZ

Comment 41

[Bob Owen (:bobowen)]

Attachment: Part 9: Add ability to print from the parent process with settings and progress listener

MozReview-Commit-ID: 7IEMByPmC0n

Comment 42

[Bob Owen (:bobowen)]

Attachment: [e-mail]

New modified version of Print Edit, to show example of how to use new functionality.

Comment 43

[Bob Owen (:bobowen)]

Comment on attachment 8733962 [details] [diff] [review]
Part 1: Move PrintSettings serialization into nsIPrintSettingsService

Sending these your way as discussed.

This first one makes things a bit easier later on and I noticed that nsIPrintOptions had some unused things on it.

I think we should be able to get rid of nsIPrintOptions in the not too distant future.

Comment 44

[Bob Owen (:bobowen)]

Comment on attachment 8733963 [details] [diff] [review]
Part 2: Make Android CreatePrintSettings work like other OS specific ones

Thought I'd flag you for this one just so you were aware in case it somehow breaks printing on Android.

Comment 45

[Bob Owen (:bobowen)]

Comment on attachment 8749207 [details] [diff] [review]
Part 9: Add ability to print from the parent process with settings and progress listener

Flagging smaug as well here for the dom/base/ integration bits as I think you've had dealing with printing in the past as well.

Basically the idea here is to allow printing to be properly triggered from parent chrome code with settings.
Initially this will just be used by addons.

At the moment we just send a message manager message to the child and it has to do all the interaction with the print settings and call into the printing code.
We end up accessing the printer and prefs for settings at least twice, which hopefully I can clean up in the future.

For sandboxing I want to stop access to the printer driver in the child, so I'll want to move Firefox's own printing over to this, even though this is first being landed for addon purposes.

Comment 46

[Jim Mathies [:jimm]]

Comment on attachment 8749204 [details] [diff] [review]
Part 7: Create the PPrinting IPDL singleton in the parent not the child

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

::: embedding/components/printingui/ipc/nsPrintingProxy.cpp
@@ +47,5 @@
>    if (!sPrintingProxyInstance) {
> +    ContentChild::GetSingleton()->SendCreatePPrintingSingleton();
> +
> +    while(!sPrintingProxyInstance) {
> +      NS_ProcessNextEvent(nullptr, true);

Hmm, hate these thread event dispatch loops, but I see we need to process ipdl events during this. I wonder if using intr for CreatePPrintingSingleton might be better. This way we would use normal thread event event dispatching. Have you tried that?

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 8749207 [details] [diff] [review]
Part 9: Add ability to print from the parent process with settings and progress listener

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

generally, nice work. Appreciate the way you broke this up.

::: embedding/components/printingui/ipc/PrintingParent.cpp
@@ +270,5 @@
> +    aPrintData->remotePrintJobParent() = remotePrintJob;
> +  } else {
> +    remotePrintJob = new RemotePrintJobParent(aPrintSettings);
> +    aPrintData->remotePrintJobParent() =
> +      SendPRemotePrintJobConstructor(remotePrintJob);

Is this leaking remotePrintJob or does that attach itself to aPrintData?

Comment 48

[Bob Owen (:bobowen)]

Attachment: Part 7: Create the PPrinting IPDL singleton in the parent not the child

Changed this to force the creation of nsPrintingProxy (PPrintingChild) and PrintingParent up front as discussed.
Generally made the patch much more simple.

MozReview-Commit-ID: 98g7eZUJ6bY

Comment 49

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #47)

> ::: embedding/components/printingui/ipc/PrintingParent.cpp
> @@ +270,5 @@
> > +    aPrintData->remotePrintJobParent() = remotePrintJob;
> > +  } else {
> > +    remotePrintJob = new RemotePrintJobParent(aPrintSettings);
> > +    aPrintData->remotePrintJobParent() =
> > +      SendPRemotePrintJobConstructor(remotePrintJob);
> 
> Is this leaking remotePrintJob or does that attach itself to aPrintData?

The lifetime of remotePrintJob should be managed by IPDL code.

We call the delete in either RemotePrintJobParent::RecvFinalizePrint or RemotePrintJobParent::RecvAbortPrint.

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 8751322 [details] [diff] [review]
Part 7: Create the PPrinting IPDL singleton in the parent not the child

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

::: dom/ipc/ContentChild.cpp
@@ +713,5 @@
>  #endif
> +#ifdef NS_PRINTING
> +  // Force the creation of the nsPrintingProxy so that it's IPC counterpart,
> +  // PrintingParent, is always available for printing initiated from the parent.
> +  Unused << nsPrintingProxy::GetInstance();

I think you mentioned this in our 1x1, just confirming, this is an async request?

Comment 51

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #50)

> ::: dom/ipc/ContentChild.cpp

> > +  Unused << nsPrintingProxy::GetInstance();
> 
> I think you mentioned this in our 1x1, just confirming, this is an async
> request?

This causes the following, which is async yes:
https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/embedding/components/printingui/ipc/nsPrintingProxy.cpp#63

Comment 52

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

Comment on attachment 8749207 [details] [diff] [review]
Part 9: Add ability to print from the parent process with settings and progress listener

>+PrintingParent::SerializePrintSettings(nsIPrintSettings* aPrintSettings,
>+                                       nsIWebProgressListener* aListener,
>+                                       layout::RemotePrintJobParent* aRemotePrintJob,
>+                                       PrintData* aPrintData)
This is oddly named method given what actually happens. The method serializes and sends to child, and the latter part is IMO
the more important part. And DeserializePrintSettings doesn't do any IPC stuff.
So perhaps call the method... EnsureRemotePrintJob ?

>   // Create a print session and let the print settings know about it.
>+  // Don't overwrite an existing print session. 
>   // The print settings hold an nsWeakPtr to the session so it does not
>   // need to be cleared from the settings at the end of the job.
>   // XXX What lifetime does the printSession need to have?
>   nsCOMPtr<nsIPrintSession> printSession;
>+  bool remotePrintJobListening = false;
>   if (!aIsPrintPreview) {
>-    printSession = do_CreateInstance("@mozilla.org/gfx/printsession;1", &rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    mPrt->mPrintSettings->SetPrintSession(printSession);
>+    rv = mPrt->mPrintSettings->GetPrintSession(getter_AddRefs(printSession));
>+    if (NS_FAILED(rv) || !printSession) {
So you ensured the same page can be printed multiple times, both in e10s and non-e10s?
What if print settings here is the global one?
Could you test and explain why not-creating print session here is ok?


I'd like to see those two issues addressed.

Comment 53

[Bob Owen (:bobowen)]

Attachment: Part 9: Add ability to print from the parent process with settings and progress listener

Thanks for looking at this smaug, I didn't realise you were on PTO when I requested, so I'm sure you've had even more reviews than normal.

(In reply to Olli Pettay [:smaug] from comment #52)

> >+PrintingParent::SerializePrintSettings(nsIPrintSettings* aPrintSettings,
> >+                                       nsIWebProgressListener* aListener,
> >+                                       layout::RemotePrintJobParent* aRemotePrintJob,
> >+                                       PrintData* aPrintData)
> This is oddly named method given what actually happens. The method
> serializes and sends to child, and the latter part is IMO
> the more important part. And DeserializePrintSettings doesn't do any IPC
> stuff.
> So perhaps call the method... EnsureRemotePrintJob ?

I think the Serialize is important too, the main purpose is to get everything into aPrintData.
Now if we have to create the RemotePrintJob then we also need to construct its counterpart in the child, but it all gets sent after that with the PrintData.

So, how about SerializeAndEnsureRemotePrintJob, it a bit of a mouthful, but probably more fully describes what is going on.

At some point I'd like to change things around, so that everything (like settings) hangs off the RemotePrintJob and we can mainly just send that back and forth.
The serialisation will happen as part of that and this will all be a bit cleaner.
However, I need to do other work before I can get to that point.
 
> >   // Create a print session and let the print settings know about it.
> >+  // Don't overwrite an existing print session. 
> >   // The print settings hold an nsWeakPtr to the session so it does not
> >   // need to be cleared from the settings at the end of the job.
> >   // XXX What lifetime does the printSession need to have?
> >   nsCOMPtr<nsIPrintSession> printSession;
> >+  bool remotePrintJobListening = false;
> >   if (!aIsPrintPreview) {
> >-    printSession = do_CreateInstance("@mozilla.org/gfx/printsession;1", &rv);
> >-    NS_ENSURE_SUCCESS(rv, rv);
> >-    mPrt->mPrintSettings->SetPrintSession(printSession);
> >+    rv = mPrt->mPrintSettings->GetPrintSession(getter_AddRefs(printSession));
> >+    if (NS_FAILED(rv) || !printSession) {
> So you ensured the same page can be printed multiple times, both in e10s and
> non-e10s?

Tested multiple printing of the same page in non-e10s and e10s (both via parent and not).

> What if print settings here is the global one?

GetGlobalPrintSettings doesn't actually do what it says in the IDL.
It recreates each time, but just holds a reference to it, which means it hangs around for no reason until the next call when it gets destroyed and replaced.
I've filed bug 1256014 for this, but from what I could tell from the history, it's been like this for a long time. There are relevant links to code in that bug.
From what I remember, it did once do what it said, but I think it got changed not long afterwards because of problems.

To be honest, I'm not quite sure what the global print settings is trying to achieve, even if it did what it said, they pretty much always get overwritten straight away from prefs and from the printer.
So, it seems like creating a new one each time is not a big deal.

Even if the global print settings did hang around, it only holds a weak reference to the print session, so that should get destroyed each time ... more on this below.

> Could you test and explain why not-creating print session here is ok?

I had to add a check here because if the print has come through the new function and TabChild::RecvPrint, then we'll have already created the print session, so that we can store the RemotePrintJob when we deserialise the PrintData to the nsIPrintSettings.

I'd did some testing around the lifetime of the print session:

non-e10s (both normal and PrintEdit addon Save PDF) and e10s not printing via parent:

  Constructed: nsPrintEngine::DoCommonPrint (code above)
  Destroyed: end of nsPrintEngine::DoCommonPrint (because nothing else holds a strong reference)

e10s printing via parent:

  Constructed: nsPrintEngine::DoCommonPrint (code above)
  Destroyed: ~nsDeviceContextSpecProxy (because we hold a reference to it to keep it alive as it's holding the RemotePrintJob)

e10s PrintEdit addon Save PDF (via new code in this patch):

  Constructed: TabChild::RecvPrint
  Destroyed: ~nsDeviceContextSpecProxy

Comment 54

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

Comment on attachment 8752233 [details] [diff] [review]
Part 9: Add ability to print from the parent process with settings and progress listener

Thanks for explanations. And SerializeAndEnsureRemotePrintJob sounds ok to me.

Comment 55

[Bob Owen (:bobowen)]

Had to #ifdef MOZ_X11 out a now unused function for part 4.

Also fixed leak of nsPrintingProxy from Part 7.

Try push before and after the leak fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c86505b67f80c22abcd12dba3cb914e54c43761
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7c1a99b758bb87b8cda0c6b2cb8b4e0b300aeec

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01f780f3be07
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece4ab032611
https://hg.mozilla.org/integration/mozilla-inbound/rev/e673d44f5aa6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7eb2ca6358
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad6e2df19e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f3f3fbbd35
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd677c353abc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68ad13d4957
https://hg.mozilla.org/integration/mozilla-inbound/rev/5324db36f239

Comment 57

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

https://hg.mozilla.org/mozilla-central/rev/01f780f3be07
https://hg.mozilla.org/mozilla-central/rev/ece4ab032611
https://hg.mozilla.org/mozilla-central/rev/e673d44f5aa6
https://hg.mozilla.org/mozilla-central/rev/7a7eb2ca6358
https://hg.mozilla.org/mozilla-central/rev/7ad6e2df19e4
https://hg.mozilla.org/mozilla-central/rev/67f3f3fbbd35
https://hg.mozilla.org/mozilla-central/rev/fd677c353abc
https://hg.mozilla.org/mozilla-central/rev/a68ad13d4957
https://hg.mozilla.org/mozilla-central/rev/5324db36f239

Comment 58

[dw-dev]

Bob,

Great work!

I am currently updating the latest version of Print Edit to work with this fix in Nightly.

Am I right in thinking that:

1. This fix will not be available in the mainstream public release until Fiefox 49 ?

2. The old way of printing to PDF (as used in previous versions of Print Edit) will continue to work with Firefox 47 and Firefox 48 ?

Comment 59

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #58)
> Bob,
> 
> Great work!

Thanks, sorry it took me so long to get round to this.

> I am currently updating the latest version of Print Edit to work with this
> fix in Nightly.
> 
> Am I right in thinking that:
> 
> 1. This fix will not be available in the mainstream public release until
> Fiefox 49 ?

Yes, the new function will be available in 49, but we won't print via the parent until we roll out the first stage of sandboxing, which will be 49 at the very earliest.
But if sandboxing is disabled, printing in the child should still work, even though it's been triggered via the new function.

Just as a heads up, I'm going to have to change the print function slightly in bug 1270447, by adding an outerWindowID param. This is so I can use the new function for printing frames as well.

For Print Edit I think it would just be a matter of changing (from my hacked version):

  printEdit.previewBrowser.print(printSettings, printEdit.printProgressListener);

to 

  printEdit.previewBrowser.print(printEdit.previewBrowser.outerWindowID, printSettings, printEdit.printProgressListener);

So, you might want to keep an eye on that bug.
It's possible that I might have to change the function again in the future, but I'll try and remember to let you know.

> 2. The old way of printing to PDF (as used in previous versions of Print
> Edit) will continue to work with Firefox 47 and Firefox 48 ?

Yes.

Comment 60

[dw-dev]

Bob,

Thanks for detailed and clear response.

Just one more question - regarding your modified version of Print Edit - Is onStatusChange (in the printProgressListener) only called when an error occurs and never during normal operation?

Comment 61

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #60)
> Bob,
> 
> Thanks for detailed and clear response.
> 
> Just one more question - regarding your modified version of Print Edit - Is
> onStatusChange (in the printProgressListener) only called when an error
> occurs and never during normal operation?

Just on an error (when nsPrintEngine::FirePrintingErrorEvent is called), I needed something to pass errors back up to the parent process, but I didn't want to use the existing functions for fear of breaking other listeners.

However, you should probably check in the listener that it is a failure, just in case this changes.
(Unlike my test version. :-) )

Comment 62

[dw-dev]

Bob,

I will release Print Edit 16.8 in the next couple of days.  This version works with all versions of Firefox from 4.0 thru 49.0a1.

On a slightly different topic, over the last few weeks I have been working on a WebExtensions version of Print Edit, based on the version of Print Edit that I ported to Chrome a couple of years ago, and updated to include all of the functionality in the latest Firefox version.

I now have a single source code that works in both Firefox and Chrome, amazingly!

However, the user experience on Firefox is inferior to that on Chrome for two reasons:

1. There is no way to initiate the Firefox > Print Preview command from within a WebExtensions add-on. This is not a problem on Chrome because window.print() opens the print preview in Chrome, but opens the print dialog in Firefox. I have raised Bug 1269300 for this issue, but there has been little response, and I wondered if you had a view?

2. There is no way to save PDF files in Firefox from within a WebExtensions add-on.  However, if (1) was fixed, and if the Save As PDF functionality in the XUL/XPCOM version of Print Edit was incorporated into the standard Firefox print preview window, then this issue would be solved.  Again, I wondered if youa had a view?

I could send you a copy of Print Edit WE (WebExtensions), if that would help you understand how this generic version works?

Comment 63

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #62)

> 1. There is no way to initiate the Firefox > Print Preview command from
> within a WebExtensions add-on. This is not a problem on Chrome because
> window.print() opens the print preview in Chrome, but opens the print dialog
> in Firefox. I have raised Bug 1269300 for this issue, but there has been
> little response, and I wondered if you had a view?

I don't know too much about our WebExtensions implementation, but I was thinking that we might want to expose this new function somehow.

I'm not sure about whether window.print() should give you a preview or prompt, I think you could argue it either way. In firefox we hide the other tabs, so I don't think we should do it unless we decide to change that.
However, I do think that we should probably give the ability to add-ons to trigger print preview somehow.
 
> 2. There is no way to save PDF files in Firefox from within a WebExtensions
> add-on.  However, if (1) was fixed, and if the Save As PDF functionality in
> the XUL/XPCOM version of Print Edit was incorporated into the standard
> Firefox print preview window, then this issue would be solved.  Again, I
> wondered if youa had a view?

I'm not sure we'd want to add this option into the standard preview window, but if we gave access to this new function, then perhaps add-ons would be able to continue to trigger the use of this hidden functionality.
At least for as long as it is still there and works reasonably well.

I'll look into how we add things to our WebExtensions API and what we want to do here.

Comment 64

[dw-dev]

Thanks.

If there is no desire to change the operation of window.print(), then providing window.printPreview() or window.doCommand("cmd_printPreview") would be alternatives, without changing the WebExtensions API.

Adding the ability to save PDF files in the print preview window has the advantage that you can preview the PDF before saving and it is easy to implement separate page settings for PDF files.

Comment 65

[dw-dev]

Bob,

With regard to your comment #59, I just released Print Edit 16.9, which includes the change to add the outerWindowID parameter into the print() call, for compatibility with Firefox 49.

As you know, to save the preview window contents to a PDF file, I use a call to print() with printSettings.printToFile = true.

I have just noticed that the saved file remains locked until Firefox is exited, which is very inconvenient for users. This behaviour is different to when the file was saved from the content process, where the file was immediately accessible.

Is there any way around this problem?

Regards, Dave

Comment 66

[u350923]

Can you clarify "compatibility with Firefox 49"?

Testing with

Name 	Firefox
Version 	49.0a2
Build ID 	20160726004006
Print Edit	16.9

Print Edit or Preview pulls up the dialog:

https://snag.gy/XE3mbP.jpg

and simply sits there, never completing.  The dialog is cancel-able.

Comment 67

[dw-dev]

I believe that you have already reported this problem to me by e-mail. In which case, you know that I have done some investigation of this problem.  No-one else has reported this problem, so I am assuming that it is limited either to your specific system configuration or to openSUSE. The investigation is stalled at the moment, because I have not received a reply to my e-mail of 13/07/16.

Comment 68

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #65)

> I have just noticed that the saved file remains locked until Firefox is
> exited, which is very inconvenient for users. This behaviour is different to
> when the file was saved from the content process, where the file was
> immediately accessible.

I'd seen an intermittent file locking issue, even with the old version, so I thought it was an issue within Print Edit.

Can you file a separate bug with STR please and I'll track down the problem.

Comment 69

[dw-dev]

This problem has now been raised as Bug 1319360

Comment 70

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #69)
> This problem has now been raised as Bug 1319360

Ah, I thought you'd found the issue.

I should be able to look into this fairly soon.