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

Status: VERIFIED FIXED

(Toolkit :: Printing, defect)

Description

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

.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

So is that specific to about:newtab or rather the case for all internal XUL pages we have?

Comment 2

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

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.

Comment 3

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

FF24 at least was ok.

Comment 4

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

ESR31 is also ok.

Comment 5

[Alice0775 White]

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

Suspect: Bug 1082579, Bug 1082575

Comment 6

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

I agree that bug 1082579 and or bug 1082575 is the likely cause here.

I'll take a look.

Comment 7

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

Attachment: Patch

Comment 8

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

Attachment: Patch v2

Comment 9

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

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?

Comment 10

[Dave Townsend [:mossop]]

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.

Comment 11

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

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

Comment 12

[Dave Townsend [:mossop]]

(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.

Comment 13

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

Thanks Mossop!

remote:   https://hg.mozilla.org/integration/fx-team/rev/9c4843953f64

Comment 14

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

Filed follow-up bug 1119562 and bug 1119563.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c4843953f64

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Mike - 36 is the last release marked as affected. Do you think this can be uplifted?

Comment 17

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

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 18

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

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Setting qawanteed since we had some breakages on this in the page. Please also test with an actual printer!

Comment 21

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/2fd253435fe4

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Thanks for the real life tests :)

Comment 24

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1117936.html