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

VERIFIED FIXED in Firefox 38

Status

()

Toolkit
Printing
--
major
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug, {regression})

unspecified
mozilla39
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox35 unaffected, firefox36 unaffected, firefox37 unaffected, firefox38+ verified, firefox39 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

1.86 KB, patch
mconley
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
39 bytes, text/x-review-board-request
mconley
: review+
Details | Review
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.
Flags: firefox-backlog+
[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.
status-firefox35: --- → unaffected
status-firefox36: --- → unaffected
status-firefox37: --- → unaffected
status-firefox38: --- → affected
tracking-firefox38: --- → ?
tracking-firefox-esr38: --- → ?
Hopefully I'm doing the right thing with these flags.
status-firefox-esr38: --- → affected

Comment 3

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

Comment 4

3 years ago
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).
Flags: needinfo?(mconley)

Comment 5

3 years ago
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.
(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.
Flags: needinfo?(mconley)
Oh snap - it _IS_ busted on Windows. This just got more serious.
OS: Linux → All
Note that one needs to be running with e10s disabled.

Comment 9

3 years ago
Created attachment 8567140 [details] [diff] [review]
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? :-)
Attachment #8567140 - Flags: review?(mconley)

Updated

3 years ago
Keywords: leave-open
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.
Attachment #8567140 - Flags: review?(mconley) → review+
Created attachment 8567262 [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=?
Created attachment 8567265 [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=?
Attachment #8567262 - Attachment is obsolete: true
This does the job on Linux, testing on Windows now...
Created 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=?
Assignee: nobody → mconley
Attachment #8567265 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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?
Attachment #8567285 - Flags: review?(benjamin)

Comment 16

3 years ago
Thanks for the quick review!

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

Updated

3 years ago
Attachment #8567140 - Flags: checkin+
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.
Attachment #8567285 - Flags: review?(benjamin) → review?(bugs)
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.
Attachment #8567285 - Flags: review?(bugs) → review+
Thanks! Filed bug 1135901 for removing that dead code.
See Also: → bug 1135901
Blocks: 1090448
Created attachment 8568743 [details]
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
Attachment #8567285 - Attachment is obsolete: true
Attachment #8568743 - Attachment description: MozReview Request: bz://1134891/mconley → MozReview Request: bz://1134891/mconley (r+'d by smaug)
Attachment #8568743 - Flags: review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec627a9f91c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b021e23ff21
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.
Flags: needinfo?(mconley)
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
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
Flags: needinfo?(mconley)

Comment 26

3 years ago
(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
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/613f2ec644b9
https://hg.mozilla.org/mozilla-central/rev/6c6f4873629d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(mconley)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

3 years ago
Iteration: --- → 39.1 - 9 Mar
Flags: qe-verify?
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.
Attachment #8568743 - Flags: approval-mozilla-aurora?

Comment 29

3 years ago
(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...
Flags: needinfo?(philip.chee)
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.
status-firefox39: --- → fixed
status-firefox-esr38: affected → ---
tracking-firefox38: ? → +
tracking-firefox-esr38: ? → ---
Flags: needinfo?(kairo)
Marking for verification and additional exploratory testing for printing.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(kairo)
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.
Attachment #8568743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks!

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b063310a4e36
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/63e88b0c9000
status-firefox38: affected → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified

Updated

3 years ago
See Also: → bug 1148807

Comment 35

3 years ago
(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

Updated

3 years ago
Flags: needinfo?(philip.chee)
Comment on attachment 8568743 [details]
MozReview Request: bz://1134891/mconley (r+'d by smaug)
Attachment #8568743 - Attachment is obsolete: true
Attachment #8619545 - Flags: review+
Attachment #8619546 - Flags: review+
Created attachment 8619545 [details]
MozReview Request: Bug 1134891 - Print progress code paths for OS X should be unreachable. r=trivial.
Created attachment 8619546 [details]
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.
Blocks: 778106
You need to log in before you can comment on or make changes to this bug.