1324064 - [e10s] printing causes content process to crash with Foxit Reader PDF

Status: RESOLVED FIXED

(External Software Affecting Firefox :: Other, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-24be6a2a-e533-4da2-abf1-95f402161216.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	ntdll.dll 	KiRaiseUserExceptionDispatcher 	
1 	ntdll.dll 	KiFastSystemCall 	
2 	kernelbase.dll 	BasepCopyFileExW 	
3 	kernelbase.dll 	CopyFileExW 	
4 	kernelbase.dll 	CopyFileW 	
Ø 5 	frdvpr_ui.dll 	frdvpr_ui.dll@0x2dd8d 	
Ø 6 	frdvpr_ui.dll 	frdvpr_ui.dll@0x2d510 	
Ø 7 	frdvpr_ui.dll 	frdvpr_ui.dll@0x2c495 	
8 	winspool.drv 	DocumentPropertySheets 	
9 	winspool.drv 	DocumentPropertiesWNative(HWND__*, void*, unsigned short*, _devicemodeW*, unsigned long, _devicemodeW*, unsigned long) 	
10 	winspool.drv 	DocumentPropertiesW 	
11 	xul.dll 	nsDeviceContextSpecWin::GetDataFromPrinter(char16ptr_t, nsIPrintSettings*) 	widget/windows/nsDeviceContextSpecWin.cpp:361
12 	xul.dll 	nsPrinterEnumeratorWin::InitPrintSettingsFromPrinter(char16_t const*, nsIPrintSettings*) 	widget/windows/nsDeviceContextSpecWin.cpp:472
13 	xul.dll 	nsPrintOptions::InitPrintSettingsFromPrinter(char16_t const*, nsIPrintSettings*) 	widget/nsPrintOptionsImpl.cpp:1095
14 	xul.dll 	nsGlobalWindow::PrintOuter(mozilla::ErrorResult&) 	dom/base/nsGlobalWindow.cpp:7379
15 	xul.dll 	nsGlobalWindow::Print(mozilla::ErrorResult&) 	dom/base/nsGlobalWindow.cpp:7414
16 	xul.dll 	mozilla::dom::WindowBinding::print 	obj-firefox/dom/bindings/WindowBinding.cpp:2527
17 	xul.dll 	mozilla::dom::WindowBinding::genericMethod 	obj-firefox/dom/bindings/WindowBinding.cpp:13778
18 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:453
19 	xul.dll 	js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) 	js/src/vm/TypeInference.cpp:3268

i am filing a bug report based on this user question at sumo https://support.mozilla.org/questions/1150794 where the reporter experienced a crash of the content process when attempting to print something.

looking through crash stats, there seem to be a couple of signatures related to crashes with the frdvpr_ui.dll module present, which are spiking up in volume since firefox 50 & later builds. 
those crashes are only happening on 32bit versions of firefox on windows and user comments generally refer to printing something when the crashes occur: http://bit.ly/2hXgruk
(i wasn't able to reproduce the crash when playing around with foxit pdf reader and printing in firefox myself)

should we try to blocklist the module or are there other mitigations that may be apparent from the crash stacks?

Comment 1

[Benjamin Smedberg]

This is content; is content sandboxed in FF50? I'm surprised we're running the printer driver in the content process, but I don't know a lot about how e10s+printing is supposed to work today. NI?bobowen since he touched e10s+printing+sandboxing in bug 1156742

Comment 2

[Benjamin Smedberg]

[Tracking Requested - why for this release]: content crash when printing with somewhat popular software.

Comment 3

[Bob Owen (:bobowen)]

The printer driver is currently still used in the child to get the printer settings.

Looks like the Foxit driver is trying to copy a file and that is getting blocked.
Seems a slightly strange thing to do when only retrieving printer properties.

I was planning on working on this next anyway because, we need to stop touching the printer drivers in the child anyway in order to strengthen the sandbox policy.

Given how fragile the printing code is, I don't relish the thought of uplifting them though.

Comment 4

[Gerry Chang [:gchang]]

Track 51+/52+/53+ as crash in all versions.

Comment 5

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2780124013a1f0cd3f65b60a9abb1cb9638c7c0

Comment 6

[Bob Owen (:bobowen)]

Attachment: Stop accessing printer devices in the child when printing via parent

This is the bug I was talking about on Monday.
I need to do some more cross platform testing, but thought I'd get it up for review.

This is a "quick" fix which just stops accessing the printer device settings in the child.
For normal printing, we already get them again in the parent anyway.
For print preview this also includes a bit of a hack to use the same technique as we do for the print silent pref.
We still call ShowPrintDialog in the parent, but just use that call to get the print settings from the printer without showing the dialog.

I've also made a change so that we don't go to the printer when the output format is PDF, because actually there isn't really a print device associated with that and we end up going to the default printer in the child as well.
This is only ued by the Print Edit addon as far as I'm aware and it passes in all the details required in the print settings it creates.
We'll need a small change to that addon with these changes, but they will actually be to remove a work around for the current situation.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8820722 [details] [diff] [review]
Stop accessing printer devices in the child when printing via parent

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

::: embedding/components/printingui/ipc/PrintingParent.cpp
@@ +83,5 @@
>  PrintingParent::ShowPrintDialog(PBrowserParent* aParent,
>                                  const PrintData& aData,
>                                  PrintData* aResult)
>  {
> +  // If aParent is null this call is just being used to getting print settings

nit - 'to get print'

::: embedding/components/printingui/ipc/nsPrintingProxy.cpp
@@ +81,5 @@
> +  if (parent) {
> +    // Get the TabChild for this nsIDOMWindow, which we can then pass up to
> +    // the parent.
> +    nsCOMPtr<nsPIDOMWindowOuter> pwin = nsPIDOMWindowOuter::From(parent);
> +    NS_ENSURE_STATE(pwin);

Do you expect these to return early if the params are null?

::: layout/printing/nsPrintEngine.cpp
@@ +609,5 @@
> +        nsPIDOMWindowOuter* domWin = nullptr;
> +        // We leave domWin as nullptr to indicate a call for print preview.
> +        if (!aIsPrintPreview) {
> +          domWin = mDocument->GetWindow();
> +          NS_ENSURE_TRUE(domWin, NS_ERROR_FAILURE);

use NS_ENSURE_STATE here as well?

http://searchfox.org/mozilla-central/source/xpcom/glue/nsDebug.h#351

Comment 8

[Bob Owen (:bobowen)]

Attachment: [e-mail]

As part of this quick fix to stop accessing the print devices in the child, I had to change some of the Windows Print to PDF code as well.

This now means that you keep using whatever printer name you are using to save settings to prefs (in Print Edit's case "Print Edit PDF"), instead of setting it to blank or the default printer name.
When you specify kOutputFormatPDF as the outputFormat, it no longer tries to go to the print device (which may not exist) and instead assumes that you have passed all the relevant details in the settings.

I think this makes more sense than the old way that ended up retrieving things from the default printer even though they may not be relevant.

This should also fix bug 1322520.

Thought I'd let you know before I landed this, so that you are ready to make the necessary changes to Print Edit.
It's likely that this will get uplifted, assuming that there aren't other problems.

I've uploaded my hacked version, but there may be other places you need to change things.

Comment 9

[dw-dev]

Bob,

Thanks for letting me know.

I will take a look at your hacked version and prepare an updated version of Print Edit (17.7) ready for release.

Do you know which Firefox versions this fix will be uplifted into?  If not, when will you know?

Comment 10

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #9)
> Bob,
> 
> Thanks for letting me know.
> 
> I will take a look at your hacked version and prepare an updated version of
> Print Edit (17.7) ready for release.
> 
> Do you know which Firefox versions this fix will be uplifted into?  If not,
> when will you know?

We are tracking this for 51, so I would say almost certainly 52 and probably 51 as well.
Ultimately though the decision is not mine, so you'll need to watch email from this bug to see what actually happens.

Comment 11

[Bob Owen (:bobowen)]

Thanks Jim.

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

> > +  // If aParent is null this call is just being used to getting print settings
> 
> nit - 'to get print'

Fixed locally, thanks.
 
> ::: embedding/components/printingui/ipc/nsPrintingProxy.cpp
> @@ +81,5 @@
> > +  if (parent) {
> > +    // Get the TabChild for this nsIDOMWindow, which we can then pass up to
> > +    // the parent.
> > +    nsCOMPtr<nsPIDOMWindowOuter> pwin = nsPIDOMWindowOuter::From(parent);
> > +    NS_ENSURE_STATE(pwin);
> 
> Do you expect these to return early if the params are null?

I'm only using null parent, so as this code only happens if it isn't, no.

> ::: layout/printing/nsPrintEngine.cpp
> @@ +609,5 @@
> > +        nsPIDOMWindowOuter* domWin = nullptr;
> > +        // We leave domWin as nullptr to indicate a call for print preview.
> > +        if (!aIsPrintPreview) {
> > +          domWin = mDocument->GetWindow();
> > +          NS_ENSURE_TRUE(domWin, NS_ERROR_FAILURE);
> 
> use NS_ENSURE_STATE here as well?
> 
> http://searchfox.org/mozilla-central/source/xpcom/glue/nsDebug.h#351

That would give a return of NS_ERROR_UNEXPECTED not NS_ERROR_FAILURE.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1794dcb6d743
Stop accessing printer devices in the child when printing via parent. r=jimm

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1794dcb6d743

Comment 14

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

Bob, want to request uplift at least to aurora? How risky do you think this is for beta? We only have a week left before the RC build (release is jan. 23)

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8820722 [details] [diff] [review]
Stop accessing printer devices in the child when printing via parent

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Bob, want to request uplift at least to aurora? How risky do you think this
> is for beta? We only have a week left before the RC build (release is jan.
> 23)

There is some risk here, so I definitely think we should get some QE printing coverage across platforms, with e10s on and off.

Approval Request Comment
[Feature/Bug causing the regression]:
Content process sandbox.

[User impact if declined]:
Some users using Foxit Reader PDF printer driver will keep crashing.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
I have verified that we are no longer accessing the printer driver in the child on Windows.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes.
I couldn't reproduce this, engineer would need to install Foxit Reader PDF to try and reproduce.
General print testing across platforms and with e10s enabled/disabled would be good as well.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Somewhat.

[Why is the change risky/not risky?]:
I've tried to keep the fix fairly simple with uplift in mind, but the printing code is fairly fragile and doesn't have any automated tests.

[String changes made/needed]:
None.

Comment 16

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

Comment on attachment 8820722 [details] [diff] [review]
Stop accessing printer devices in the child when printing via parent

Let's try this in aurora 52. It sounds too risky for the last week of beta though. Thanks for the details!   
During the 52 beta cycle, printing in general will get some manual testing and we have a good chance of hearing user feedback if this causes regressions.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/048ad2874f2c

Comment 18

[Gerry Chang [:gchang]]

Per comment #16, mark 51 won't fix.

Comment 19

[dw-dev]

Bob,

I recently released Print Edit 17.7 to make it compatible with this bug fix.

Since then I have released two more versions of Print Edit (17.8 & 17.9) to fix problems unrelated to this bug.

A few days ago I received a problem report from a Print Edit user who had encountered an error message: "The selected printer could not be found".

This happened when the user was trying to preview a web page before printing to PDF.

In this case, with Firefox 52 and later, Print Edit sets the printerName in printSettings to "Print Edit PDF".

Investigation has shown that:
  - if e10s is enabled and printerName is set to "Print Edit PDF", then printPreview succeeds.
  - if e10s is disabled and printerName is set to "Print Edit PDF", then printPreview FAILS!
  - if e10s is disabled and printerName is set to "", then printPreview succeeds.

Note, print to PDF always succeeds with printerName set to "Print Edit PDF", whether or not e10s is enabled.

I can work around the problem with printPreview by setting printerName to "" if e10s is disabled.

But shouldn't printPreview work with printerName set to "Print Edit PDF", whether or not e10s is enabled ?

Comment 20

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #19)
> Bob,

> A few days ago I received a problem report from a Print Edit user who had
> encountered an error message: "The selected printer could not be found".

> I can work around the problem with printPreview by setting printerName to ""
> if e10s is disabled.

Thanks, sorry about that.
 
> But shouldn't printPreview work with printerName set to "Print Edit PDF",
> whether or not e10s is enabled ?

Yes it should, would you file a bug for this, please.

Comment 21

[dw-dev]

Bob,

Sorry it's taken a bit of time, but I have now filed Bug 1352841 for this problem.

Recently, I have received a bug report from another user who is using Firefox 52.0.1 and Print Edit 17.10 (which includes a workaround for Bug 1352841) running on iMac Sierra 10.12.3. This user says that Print Edit no longer prints to the desired scale. It now always prints to the default 100%. 

Normally, none of Firefox's print preview functionality is available when using Mac OS X, but Print Edit makes that functionality available, and also makes the print to PDF functionality available.

Will the changes that you made in Bug 1324064 also work with Mac OS X ?

Comment 22

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #21)
> Bob,
> 
> Sorry it's taken a bit of time, but I have now filed Bug 1352841 for this
> problem.
> 
> Recently, I have received a bug report from another user who is using
> Firefox 52.0.1 and Print Edit 17.10 (which includes a workaround for Bug
> 1352841) running on iMac Sierra 10.12.3. This user says that Print Edit no
> longer prints to the desired scale. It now always prints to the default
> 100%. 
> 
> Normally, none of Firefox's print preview functionality is available when
> using Mac OS X, but Print Edit makes that functionality available, and also
> makes the print to PDF functionality available.
> 
> Will the changes that you made in Bug 1324064 also work with Mac OS X ?

Some of the changes were Windows specific, so not sure.
I thought Mac had it's own mechanism for printing to PDF.

Fx52 we started printing via the parent on Mac as well, to enable some sandboxing there.
Can you file a separate bug for the scaling issue, please.

Comment 23

[dw-dev]

Bob,

Yes, you are quite right, Print Edit makes Firefox's print preview functionality available on Mac OS X, but does not make the print to PDF functionality available.

With regards to the scaling issue, the user says that printing is always at 100%, but it is not clear whether print previewing is also always at 100%.  Would it help if I asked that question?

I have filed Bug 1353064 for this problem.

Comment 24

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #23)
> Bob,
> 
> Yes, you are quite right, Print Edit makes Firefox's print preview
> functionality available on Mac OS X, but does not make the print to PDF
> functionality available.
> 
> With regards to the scaling issue, the user says that printing is always at
> 100%, but it is not clear whether print previewing is also always at 100%. 
> Would it help if I asked that question?

Yes, I think that would be useful.
I've asked haik, who is working on the Mac sandboxing and worked on getting printing via the parent turned for Mac, to look at that bug.

Comment 25

[dw-dev]

I have asked the question and will let you know the answer.