1321566 - e10s Crash in nsPrintEngine::PrePrintPage

Status: RESOLVED FIXED

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-34142b46-3cc4-4540-b0ac-aeeaf2161201.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsPrintEngine::PrePrintPage() 	layout/printing/nsPrintEngine.cpp:2647
1 	xul.dll 	nsPagePrintTimer::Notify(nsITimer*) 	layout/printing/nsPagePrintTimer.cpp:152
2 	xul.dll 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp:524
3 	xul.dll 	nsTimerEvent::Run() 	xpcom/threads/TimerThread.cpp:286
4 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1067
5 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:290
6 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
7 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
8 	xul.dll 	_SEH_epilog4

hi, this is another printing related signature that is spiking up in 51.0b5. it's happening across all versions of windows and always in the content process. 
in early data for beta 5 this is accounting for 28% of all content crashes.

pushlog from beta 4 to beta 5: 51.0b4 & beta 5:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_51_0b4_RELEASE&tochange=FIREFOX_51_0b5_RELEASE

might this be related to bug 1279699 as well?

(marking the report as security sensitive due to the crashing address)

Comment 2

[Daniel Veditz [:dveditz]]

Clearly a UAF, but print preview requires enough user interaction we can call this sec-moderate rather than sec-high.

Comment 3

[Bob Owen (:bobowen)]

I'm going to leave this open even though the patches for bug 1279699, have been backed out.

It has been happening at a lower level for some time.
I suspect it happens when the print is aborted for some reason and the last patches for bug 1279699 were causing many more failures.

We should still get this fixed though.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Use UniquePtr to hold nsPrintData in nsPrintEngine

I think you're probably the best person to review this.

The code this is replacing, appears to be doing the right thing with these pointers, so I'm not sure how this is happening.
Using UniquePtr can't hurt though.

I don't think it can be a threading problem as I believe this is all called on the main thread.
I'm not sure UniquePtr will help if it is a threading issue.

As an aside, I'm not at all convinced we need all three of these nsPrintData, I think we could probably get away with one, but I don't want to make more changes until I've got some tests working.

Comment 5

[Olli Pettay [:smaug]]

Isn't the crash more likely that nsPrintEngine is gone when the timer fires
(but using UniquePtr doesn't hurt).

http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/layout/printing/nsPagePrintTimer.h#61 looks very suspicious.

Comment 6

[Olli Pettay [:smaug]]

Need to be a bit careful with cycles. So perhaps nsPagePrintTimer could have nsWeakPtr pointing to nsPrintEngine or so.

Comment 7

[Olli Pettay [:smaug]]

Attachment: wip

I ended up starting to write a patch, but since print-to-file seems to be broken on linux, I couldn't test this.

Comment 8

[Olli Pettay [:smaug]]

Bob, were you going to take a look at this?
I forgot what we discussed on IRC.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Bob, were you going to take a look at this?
> I forgot what we discussed on IRC.

We decided we were going to go with your patch and I asked a few days ago (on IRC) if you wanted to upload another version or whether I could just put a header on the existing one and land them both.

I think it all got lost as it was just before Christmas.

Comment 10

[Olli Pettay [:smaug]]

ok, let me test the patch a bit. I think it is still doable somehow if I disable all sandboxing

Comment 11

[Bob Owen (:bobowen)]

(In reply to Olli Pettay [:smaug] from comment #10)
> ok, let me test the patch a bit. I think it is still doable somehow if I
> disable all sandboxing

OK thanks.
I have also done some testing with your patch on Windows.

Comment 12

[Olli Pettay [:smaug]]

Can't test this right now since printing crashes in debug builds even without this patch :/

Comment 13

[Olli Pettay [:smaug]]

Comment on attachment 8819837 [details] [diff] [review]
wip

Whoever gets a chance to review this first.
The changes should be minimal and patch applies for example to beta too.

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 8819837 [details] [diff] [review]
wip

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

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8819837 [details] [diff] [review]
> wip
> 
> Whoever gets a chance to review this first.
> The changes should be minimal and patch applies for example to beta too.

Thanks Olli, just gone over this again to remind myself, although I'd looked at it before Christmas.

I'll get these two landed, although as you said, your patch is far more likely to fix this problem.

Over uplift, given that this crash is not all that common and the fragility of the printing code, I think I'd say uplift to aurora but not beta, so it gets a full beta cycle just in case it causes problems.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23646223ff69
https://hg.mozilla.org/mozilla-central/rev/e8a7beb2bf1a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 17

[Bob Owen (:bobowen)]

Comment on attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine

smaug, dveditz - I'm only requesting uplift to Aurora as this is a fairly low level crash and the printing code is pretty fragile. Please say if you think we should request for Beta as well.

Approval Request Comment
[Feature/Bug causing the regression]:
Long-term crash.

[User impact if declined]:
Small number of users will still experience this crash.

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

[Has the fix been verified in Nightly?]:
We do not have STR.

[Needs manual test from QE? If yes, steps to reproduce]: 
No STR.

[List of other uplifts needed for the feature/fix]:
Other patch in this bug.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
The changes are relatively straight forward, but the printing code is quite fragile and does not have automated testing.

[String changes made/needed]:
No

Comment 18

[Bob Owen (:bobowen)]

Comment on attachment 8819837 [details] [diff] [review]
wip

See comment 17.

Comment 19

[Olli Pettay [:smaug]]

The initial comment says "hi, this is another printing related signature that is spiking up in 51.0b5.".
I consider the patch relatively safe since it is effectively adding null checks.
So, perhaps beta wouldn't be too risky, at least if we can get at least some manual testing (since the next merge day is soon).

Comment 20

[Bob Owen (:bobowen)]

(In reply to Olli Pettay [:smaug] from comment #19)
> The initial comment says "hi, this is another printing related signature
> that is spiking up in 51.0b5.".

That was because of a problem I introduced that I've since rectified.
It did however highlight this and a couple of other low-level crashes that I thought were worth fixing anyway.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine

printing crash fix, aurora52+

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 8819837 [details] [diff] [review]
wip

printing crash fix, aurora52+

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to Bob Owen (:bobowen) from comment #17)
> smaug, dveditz - I'm only requesting uplift to Aurora as this is a fairly
> low level crash and the printing code is pretty fragile. Please say if you
> think we should request for Beta as well.

Fair enough.

Comment 24

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d7e7bc831d1
https://hg.mozilla.org/releases/mozilla-aurora/rev/873a99553516