1134891 - Printing results in the progress dialog never going away and the print never happening

Status: VERIFIED FIXED

(Toolkit :: Printing, defect)

Description

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

STR:

1) On Linux in a non-e10s window, open some web page
2) Go to File > Print to print the document
3) Set some print settings and hit "Print"

ER: This should briefly show the print progress dialog as the document-to-print gets rendered, and then it should print.

AR: We show the print progress dialog permanently, and show an error in this console:

JavaScript error: chrome://global/content/dialogOverlay.js, line 74: TypeError: opener is null

A quick mozregression session and a local backout points the finger at the patch for bug 1115333.

Comment 1

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

[Tracking Requested - why for this release]:

This is a particularly nasty regression that I'd hate for us to propagate up the trains. Especially since 38 is an ESR.

Comment 2

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

Hopefully I'm doing the right thing with these flags.

Comment 3

[:Gijs (he/him)]

Delightful. I should have known that we have no tests for printing code... :-\

Comment 4

[:Gijs (he/him)]

comment #0 specifically says "non-e10s window". Does the same not happen on e10s?

All this code (in dialogOverlay.js) is trying to do is center a dialog on top of the opener window. Seems to me like it should always be opened from the chrome window, and that will fix this (the reason opener is now null is because it's being opened from a content window).

Comment 5

[:Gijs (he/him)]

Also, inasmuch as this is breaking printing, the trivial fix that will forgo the centering would be to nullcheck opener before trying to use it.

Comment 6

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

(In reply to :Gijs Kruitbosch from comment #4)
> comment #0 specifically says "non-e10s window". Does the same not happen on
> e10s?
> 

Until bug 1090448 is fixed, printing is just flat-out disabled on Linux with e10s, so I guess it doesn't matter there - but if printing was enabled, it would also be affected.

> All this code (in dialogOverlay.js) is trying to do is center a dialog on
> top of the opener window. Seems to me like it should always be opened from
> the chrome window, and that will fix this (the reason opener is now null is
> because it's being opened from a content window).

I see. This is the code that, eventually, opens the dialog:

https://dxr.mozilla.org/mozilla-central/source/embedding/components/printingui/unixshared/nsPrintingPromptService.cpp#127

So I suppose nsPrintingPromptService::ShowProgress is getting passed the content window for whatever's being printed.

Strange - I don't see much variation between what we do on Linux with what we do on Windows. I don't see why this shouldn't have broken the dialog on Windows as well, tbh.

Comment 7

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

Oh snap - it _IS_ busted on Windows. This just got more serious.

Comment 8

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

Note that one needs to be running with e10s disabled.

Comment 9

[:Gijs (he/him)]

Attachment: stopgap: don't break if opener is null,

This should unbreak stuff for the moment, although really printing should always be passing the toplevel (browser, rather than content) window, even in non-e10s. Mike said he could do that. In the meantime, does this look sane and does it unbreak printing, even if the dialog might be weirdly positioned? :-)

Comment 10

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

Comment on attachment 8567140 [details] [diff] [review]
stopgap: don't break if opener is null,

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

So while this might fix things for other dialogs that make use of dialogOverlay.js, it doesn't fix the printing case as there are still references to opener in printProgress.js. :/

I think we should still take this for anything else that might use dialogOverlay, but we need to make it so that the dialogs are all being opened such that the parent is the browser window, and not the content window for what's being printed.

Comment 11

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

Attachment: Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

Comment 12

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

Attachment: Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

Comment 13

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

This does the job on Linux, testing on Windows now...

Comment 14

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

Attachment: Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

Comment 15

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

Comment on attachment 8567285 [details] [diff] [review]
Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

bsmedberg - you're listed as the module owner for embedding, which is why I'm r?ing you.

I have no idea if you're the right person to review this, but if not, perhaps you know who I should redirect it to?

Comment 16

[:Gijs (he/him)]

Thanks for the quick review!

remote:   https://hg.mozilla.org/integration/fx-team/rev/2b1d7ebb8325

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/2b1d7ebb8325

Comment 18

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

Comment on attachment 8567285 [details] [diff] [review]
Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

smaug has heroically stepped up to the plate.

Comment 19

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

Comment on attachment 8567285 [details] [diff] [review]
Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=?

Could you please add a MOZ_ASSERT or some such to
mac's nsPrintProgress::OpenProgressDialog, since that is not supposed to be called, and file a followup to remote nsPrintProgress.h|cpp from mac/
(Probably also nsPrintProgressParams.h|cpp)

>+    // We will set the opener of the dialog to be the nsIDOMWindow for the
>+    // browser XUL window itself, as opposed to the content. That way, the
>+    // progress window has access to the opener.
>+    nsCOMPtr<nsPIDOMWindow> pParentWindow = do_QueryInterface(parent);
>+    NS_ENSURE_STATE(pParentWindow);
>+
>+    nsCOMPtr<nsIDocShell> docShell = pParentWindow->GetDocShell();
>+    NS_ENSURE_STATE(docShell);
>+
>+    nsCOMPtr<nsIDocShellTreeOwner> owner;
>+    rv = docShell->GetTreeOwner(getter_AddRefs(owner));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIXULWindow> ownerXULWindow = do_GetInterface(owner);
do_GetInterface is null safe so you don't need NS_ENSURE_SUCCESS(rv, rv);

>+    NS_ENSURE_STATE(ownerXULWindow);
>+
>+    nsCOMPtr<nsIDOMWindow> ownerWindow = do_GetInterface(ownerXULWindow);
do_GetInterface is null safe so you don't need NS_ENSURE_STATE(ownerXULWindow)

>+    // We will set the opener of the dialog to be the nsIDOMWindow for the
>+    // browser XUL window itself, as opposed to the content. That way, the
>+    // progress window has access to the opener.
>+    nsCOMPtr<nsPIDOMWindow> pParentWindow = do_QueryInterface(parent);
>+    NS_ENSURE_STATE(pParentWindow);
>+
>+    nsCOMPtr<nsIDocShell> docShell = pParentWindow->GetDocShell();
>+    NS_ENSURE_STATE(docShell);
>+
>+    nsCOMPtr<nsIDocShellTreeOwner> owner;
>+    rv = docShell->GetTreeOwner(getter_AddRefs(owner));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIXULWindow> ownerXULWindow = do_GetInterface(owner);
>+    NS_ENSURE_STATE(ownerXULWindow);
>+
>+    nsCOMPtr<nsIDOMWindow> ownerWindow = do_GetInterface(ownerXULWindow);
>+    NS_ENSURE_STATE(ownerWindow);
Same here.

Comment 20

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

Thanks! Filed bug 1135901 for removing that dead code.

Comment 21

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

Attachment: MozReview Request: bz://1134891/mconley (r+'d by smaug)

/r/4171 - Bug 1134891 - Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=smaug.
/r/4169 - Bug 1134891 - Print progress code paths for OS X should be unreachable. r=trivial.

Pull down these commits:

hg pull review -r 7b021e23ff21c9cc010cd8410e3940b51dad9390

Comment 22

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec627a9f91c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b021e23ff21

Comment 23

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

https://reviewboard.mozilla.org/r/4167/#review3433

::: embedding/components/printingui/unixshared/nsPrintProgress.cpp
(Diff revision 1)
> +    MOZ_ASSERT_UNREACHABLE("This is a test to see if this can even compile. "
> +                           "I don't remember if C++ is this cool.");

Oh ffs, this was not supposed to land with that stuff.

I have a patch to remove this that I can land once the tree re-opens.

Comment 24

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

Well, you can reland the patches without that once the tree reopens. I just backed both patches out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cef690bc3c

Comment 25

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

Re-landed without that silly MOZ_ASSERT_UNREACHABLE in there. Sorry about that.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/613f2ec644b9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6f4873629d

Comment 26

[Philip Chee]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> Comment on attachment 8567140 [details] [diff] [review]
> stopgap: don't break if opener is null,
> 
> Review of attachment 8567140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So while this might fix things for other dialogs that make use of
> dialogOverlay.js, it doesn't fix the printing case as there are still
> references to opener in printProgress.js. :/
> 
> I think we should still take this for anything else that might use
> dialogOverlay, but we need to make it so that the dialogs are all being
> opened such that the parent is the browser window, and not the content
> window for what's being printed.

Does this implementation of moveToAlertPosition() need to be fixed as well?
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/dialog.xml?rev=0c9ed31fcb96#98

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/613f2ec644b9
https://hg.mozilla.org/mozilla-central/rev/6c6f4873629d

Comment 28

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

Comment on attachment 8568743 [details]
MozReview Request: bz://1134891/mconley (r+'d by smaug)

Approval Request Comment
[Feature/regressing bug #]:

Fixing regression caused by bug 1115333.

[User impact if declined]:

Printing is broken on Windows and Linux. When attempting to print, the user will see the print progress dialog appear, but then not disappear. On Linux, at least, this means the print job never occurs. It's possible that the print job will still occur on Windows despite the progress dialog not going away.

So for the affected systems, in the best case, printing will occur and the dialog will never go away (which isn't a great experience), and in the worst case, printing will never occur and the dialog will never go away (which is a horrible experience).

[Describe test coverage new/current, TreeHerder]:

The fix has been manually tested on Linux and Windows - both with e10s enabled and disabled.

[Risks and why]: 

We're changing some native code to retarget the window to be opened such that the parent is the browser window itself, instead of the content window. I don't believe there's much risk - though QA can (and probably should) kick the tires on printing on these systems just to be sure.

[String/UUID change made/needed]:

None.

Comment 29

[:Gijs (he/him)]

(In reply to Philip Chee from comment #26)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> > Comment on attachment 8567140 [details] [diff] [review]
> > stopgap: don't break if opener is null,
> > 
> > Review of attachment 8567140 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So while this might fix things for other dialogs that make use of
> > dialogOverlay.js, it doesn't fix the printing case as there are still
> > references to opener in printProgress.js. :/
> > 
> > I think we should still take this for anything else that might use
> > dialogOverlay, but we need to make it so that the dialogs are all being
> > opened such that the parent is the browser window, and not the content
> > window for what's being printed.
> 
> Does this implementation of moveToAlertPosition() need to be fixed as well?
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/
> dialog.xml?rev=0c9ed31fcb96#98

Only if the dialog in question ever gets called with a content opener... Can you file a followup bug to investigate the remaining callers and assign to me? It's hard to tell from MXR whether the opener will always be OK or not...

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking 38 and approving the uplift, Kairo can you ensure this is on the test plan for 38?   No need to track or mark for ESR since the first ESR 38 is exactly FF38.

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking for verification and additional exploratory testing for printing.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8568743 [details]
MozReview Request: bz://1134891/mconley (r+'d by smaug)

Approving for aurora uplift since Lukas already said she would accept it.

Comment 33

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

Thanks!

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b063310a4e36
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/63e88b0c9000

Comment 34

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 38.0a2 (2015-03-01) using Windows 7 x64.

Verified fixed on Firefox 39.0a1 (2015-03-05) non-e10s, Firefox 39.0a1 (2015-03-06) non-e10s and Firefox 38.0a2 (2015-03-06) using Windows 7 x64 and Ubuntu 14.04 x32.

Comment 35

[Philip Chee]

(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #29)

> Only if the dialog in question ever gets called with a content opener... Can
> you file a followup bug to investigate the remaining callers and assign to
> me? It's hard to tell from MXR whether the opener will always be OK or not...

I filed Bug 1148807

Comment 37

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

Attachment: MozReview Request: Bug 1134891 - Print progress code paths for OS X should be unreachable. r=trivial.

Comment 38

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

Attachment: MozReview Request: Bug 1134891 - Make the print progress dialog for Windows and Linux be opened by the browser window's nsIDOMWindow instead of the content window. r=smaug.