Closed
Bug 1321566
Opened 9 years ago
Closed 8 years ago
e10s Crash in nsPrintEngine::PrePrintPage
Categories
(Core :: Printing: Output, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: philipp, Assigned: bobowen)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main52+])
Crash Data
Attachments
(2 files)
7.31 KB,
patch
|
jwatt
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.81 KB,
patch
|
bobowen
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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)
Flags: needinfo?(bobowencode)
Updated•9 years ago
|
Component: General → Printing: Output
Comment 2•9 years ago
|
||
Clearly a UAF, but print preview requires enough user interaction we can call this sec-moderate rather than sec-high.
Blocks: 1279699
Keywords: csectype-uaf,
sec-moderate
Assignee | ||
Comment 3•9 years ago
|
||
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.
Flags: needinfo?(bobowencode)
Assignee | ||
Updated•9 years ago
|
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 4•8 years ago
|
||
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.
Attachment #8819284 -
Flags: review?(jwatt)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
![]() |
||
Updated•8 years ago
|
Attachment #8819284 -
Flags: review?(jwatt) → review+
Updated•8 years ago
|
Group: core-security → layout-core-security
Comment 5•8 years ago
|
||
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•8 years ago
|
||
Need to be a bit careful with cycles. So perhaps nsPagePrintTimer could have nsWeakPtr pointing to nsPrintEngine or so.
Comment 7•8 years ago
|
||
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•8 years ago
|
||
Bob, were you going to take a look at this?
I forgot what we discussed on IRC.
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Flags: needinfo?(bobowencode) → needinfo?(bugs)
Comment 10•8 years ago
|
||
ok, let me test the patch a bit. I think it is still doable somehow if I disable all sandboxing
Assignee | ||
Comment 11•8 years ago
|
||
(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•8 years ago
|
||
Can't test this right now since printing crashes in debug builds even without this patch :/
Comment 13•8 years ago
|
||
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.
Flags: needinfo?(bugs)
Attachment #8819837 -
Flags: review?(jwatt)
Attachment #8819837 -
Flags: review?(bobowencode)
Assignee | ||
Comment 14•8 years ago
|
||
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.
Attachment #8819837 -
Flags: review?(jwatt)
Attachment #8819837 -
Flags: review?(bobowencode)
Attachment #8819837 -
Flags: review+
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/23646223ff69
https://hg.mozilla.org/mozilla-central/rev/e8a7beb2bf1a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Comment 16•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 17•8 years ago
|
||
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
Flags: needinfo?(dveditz)
Flags: needinfo?(bugs)
Flags: needinfo?(bobowencode)
Attachment #8819284 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8819837 -
Flags: approval-mozilla-aurora?
Comment 19•8 years ago
|
||
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).
Flags: needinfo?(bugs)
Assignee | ||
Comment 20•8 years ago
|
||
(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•8 years ago
|
||
Comment on attachment 8819284 [details] [diff] [review]
Use UniquePtr to hold nsPrintData in nsPrintEngine
printing crash fix, aurora52+
Attachment #8819284 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 22•8 years ago
|
||
Comment on attachment 8819837 [details] [diff] [review]
wip
printing crash fix, aurora52+
Attachment #8819837 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 23•8 years ago
|
||
(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.
Flags: needinfo?(dveditz)
Updated•8 years ago
|
Updated•8 years ago
|
Group: layout-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•