After trying to print preview about:newtab, print preview fails on other pages.

VERIFIED FIXED in Firefox 36

Status

()

Toolkit
Printing
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: mconley)

Tracking

({regression})

36 Branch
mozilla37
x86_64
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
.
So is that specific to about:newtab or rather the case for all internal XUL pages we have?
Flags: needinfo?(bugs)
(Reporter)

Comment 2

3 years ago
Looks like about:preferences cause the same issue.

This is a regression.
Could be something in Gecko's print code - or FF's print UI code.
Flags: needinfo?(bugs)
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 3

3 years ago
FF24 at least was ok.
(Reporter)

Comment 4

3 years ago
ESR31 is also ok.

Updated

3 years ago
status-firefox35: --- → unaffected
status-firefox36: --- → affected
status-firefox37: --- → affected
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?

Comment 5

3 years ago
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=53d84829b2b8&tochange=a9530d7f005a

Suspect: Bug 1082579, Bug 1082575
Blocks: 1082579, 1082575
Component: New Tab Page → Printing
Flags: needinfo?(mconley)
Keywords: regressionwindow-wanted
OS: Linux → All
Product: Firefox → Toolkit
Version: unspecified → 36 Branch
I agree that bug 1082579 and or bug 1082575 is the likely cause here.

I'll take a look.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Created attachment 8546137 [details] [diff] [review]
Patch
Created attachment 8546141 [details] [diff] [review]
Patch v2
Attachment #8546137 - Attachment is obsolete: true
Comment on attachment 8546141 [details] [diff] [review]
Patch v2

So the problem here was that docShell.printPreview.printPreview was throwing because printing XUL documents is very much not supported.

The old behaviour was to catch that error and then enter/exit print preview mode, which I've emulated here.

One thing to note - this patch causes an error modal dialog to appear when attempting to print preview a XUL document. This is because nsPrintEngine::CommonPrint calls ShowPrintErrorDialog, which latches onto the active window (the progress dialog in this case), and opens a modal on it.

We didn't used to show this error dialog before, because by the time the modal attempted to be opened, the progress dialog was in the process of being closed (since nsPrintEngine::CommonPrint calls CloseProgressDialog before ShowPrintErrorDialog).

With the refactoring of our printing code to use the message manager, even in non-e10s, we've changed the order of events here, and effectively made closing the print progress dialog asynchronous. This means that the dialog hasn't been asked to close by the time ShowPrintErrorDialog is asked to show itself.

What's worse, with ShowPrintErrorDialog showing, we're still processing events in the modal event loop, and so eventually the progress dialog hears that it's been asked to close. However, because the print progress dialog is now in a modal state, it fails to close.

This means that after pressing "OK" in the error dialog, the print progress dialog sticks around until the user closes it. :/

It's not a great situation. This patch fixes things so that we don't continue to be broken for other documents if we try to print preview.

My suggestion is to file a follow-up bug so that nsPrintEngine attempts to display any errors _inside_ the print progress dialog if the dialog has been opened. That way, keeping the progress dialog open makes sense in order to show an error message.

How do you feel about this stuff, Mossop?
Attachment #8546141 - Flags: review?(dtownsend)
Comment on attachment 8546141 [details] [diff] [review]
Patch v2

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

Ugh. Ok I guess as a wallpaper fix.

Your suggestion to make the print engine only show errors inside the dialog is reasonable. I think we should also file a bug to give us an API that that we can call to determine if we are likely to be able to print without having to go into a full print. It could be as simple at first as just checking the mimetype of the document and only saying yes for HTML. We should then hook that up to disable the print menu options in those cases.
Attachment #8546141 - Flags: review?(dtownsend) → review+
(Reporter)

Comment 11

3 years ago
well we can print text/plain and such. 

We do have explicit check for xul documents, 
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?mark=3635-3637,3746-3750&rev=9d593597df5f#3631
(In reply to Olli Pettay [:smaug] from comment #11)
> well we can print text/plain and such. 
> 
> We do have explicit check for xul documents, 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.
> cpp?mark=3635-3637,3746-3750&rev=9d593597df5f#3631

True. Whatever it is we shouldn't be letting users try to print documents that we know can't be printed.
Thanks Mossop!

remote:   https://hg.mozilla.org/integration/fx-team/rev/9c4843953f64
See Also: → bug 1119562
See Also: → bug 1119563
Filed follow-up bug 1119562 and bug 1119563.
https://hg.mozilla.org/mozilla-central/rev/9c4843953f64
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Mike - 36 is the last release marked as affected. Do you think this can be uplifted?
status-firefox37: affected → fixed
status-firefox38: --- → fixed
tracking-firefox36: ? → +
tracking-firefox37: ? → +
Flags: needinfo?(mconley)
(Reporter)

Comment 17

3 years ago
This very much should be. Otherwise it is very easy to get Firefox into a state where only restarting it let's one to print anything.
Comment on attachment 8546141 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]:

e10s, bug 1082575

[User impact if declined]:

Users can get into a state where print preview does not work for the entire browsing window, regardless of which tab they're looking at.

[Describe test coverage new/current, TBPL]:

Test coverage for printing in general is quite poor. This patch landed without causing a ripple on TBPL, but that's just because we don't have much in the way of automated printing tests.

[Risks and why]: 

I think this is low risk. What we've essentially done is made the content script more resilient to failure by wrapping an operation in a try/catch block, and then giving the catch the capability of informing the parent process that something has gone wrong. This lets the parent reset its state.

Also, the case where the user can actually get into this state is mostly an edge one - the user needs to attempt to print preview a XUL document, like about:newtab or about:preferences. Experience tells me that print preview is not something a user triggers very often, and when they do trigger it, it tends to be on websites, and not our built-in pages.

So to sum, I think the actual bug here is pretty small and not too many users can get into it - but the severity of the bug is pretty awful because it affects the whole window. This patch wallpapers over that latter problem. So we've got a likely-rare bug, and wallpaper to get rid of the awful symptom. So I think this is low-medium risk.


[String/UUID change made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8546141 - Flags: approval-mozilla-beta?
Attachment #8546141 - Flags: approval-mozilla-aurora?
Comment on attachment 8546141 [details] [diff] [review]
Patch v2

Aurora is 37 and it is marked as fixed (looks like the patch landed before the merge)
Attachment #8546141 - Flags: approval-mozilla-beta?
Attachment #8546141 - Flags: approval-mozilla-beta+
Attachment #8546141 - Flags: approval-mozilla-aurora?
Attachment #8546141 - Flags: approval-mozilla-aurora-
Setting qawanteed since we had some breakages on this in the page. Please also test with an actual printer!
Keywords: qawanted
https://hg.mozilla.org/releases/mozilla-beta/rev/2fd253435fe4
status-firefox36: affected → fixed
status-firefox38: fixed → ---
Flags: qe-verify+
Verified fixed using:
1. Latest Aurora, build ID: 20150121004011
2. Firefox 36 beta 2, build ID: 20150120155007

Tested on Windows 7 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.
Also tested with a real printer and pages were printed correctly.
Did not encounter any new issues except for the follow-up bugs (1119562 and 1119563).
Also, on Mac OS, the XUL pages cannot be printed/previewed.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
status-firefox37: fixed → verified
Keywords: qawanted
Thanks for the real life tests :)
Created attachment 8556287 [details]
Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1117936.html
You need to log in before you can comment on or make changes to this bug.