Closed Bug 1321566 Opened 9 years ago Closed 8 years ago

e10s Crash in nsPrintEngine::PrePrintPage

Categories

(Core :: Printing: Output, defect)

51 Branch
All
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: philipp, Assigned: bobowen)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main52+])

Crash Data

Attachments

(2 files)

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)
Component: General → Printing: Output
Clearly a UAF, but print preview requires enough user interaction we can call this sec-moderate rather than sec-high.
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)
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: nobody → bobowencode
Status: NEW → ASSIGNED
Attachment #8819284 - Flags: review?(jwatt) → review+
Group: core-security → layout-core-security
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.
Need to be a bit careful with cycles. So perhaps nsPagePrintTimer could have nsWeakPtr pointing to nsPrintEngine or so.
Attached patch wipSplinter Review
I ended up starting to write a patch, but since print-to-file seems to be broken on linux, I couldn't test this.
Bob, were you going to take a look at this? I forgot what we discussed on IRC.
Flags: needinfo?(bobowencode)
(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)
ok, let me test the patch a bit. I think it is still doable somehow if I disable all sandboxing
(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.
Can't test this right now since printing crashes in debug builds even without this patch :/
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(bobowencode)
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?
Attachment #8819837 - Flags: approval-mozilla-aurora?
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)
(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 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 on attachment 8819837 [details] [diff] [review] wip printing crash fix, aurora52+
Attachment #8819837 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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)
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: