830236 - crash involving double deletion of PresShell when printing <canvas> with a mozPrintCallback

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

I can cause a crash which I think is double deleting a PresShell.

STR:

1. Have two windows open.
2. In one window, open the attachment.
3. Proceed to print the opened document, and while the printing progress window
   is still showing, close the browser window in which the document is loaded.
4. Get a printing error dialog, and then a crash:

 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
 	msvcr100d.dll!_getptd_noexit()  Line 500	C
 	xul.dll!PresShell::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!PresShell::Release()  Line 753 + 0x18d bytes	C++
 	xul.dll!nsCOMPtr<nsIPresShell>::assign_assuming_AddRef(nsIPresShell * newPtr)  Line 480	C++
 	xul.dll!nsCOMPtr<nsIPresShell>::assign_with_AddRef(nsISupports * rawPtr)  Line 1156	C++
 	xul.dll!nsCOMPtr<nsIPresShell>::operator=(nsIPresShell * rhs)  Line 625	C++
 	xul.dll!nsPrintObject::DestroyPresentation()  Line 120	C++
 	xul.dll!nsPrintObject::~nsPrintObject()  Line 43	C++
 	xul.dll!nsPrintObject::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsPrintData::~nsPrintData()  Line 226 + 0x1f bytes	C++
 	xul.dll!nsPrintData::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsPrintEngine::Destroy()  Line 255 + 0x1f bytes	C++
>	xul.dll!nsDocumentViewer::Destroy()  Line 1651	C++
 	xul.dll!nsPagePrintTimer::~nsPagePrintTimer()  Line 23	C++
 	xul.dll!nsPagePrintTimer::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsRunnable::Release()  Line 31 + 0x8f bytes	C++
 	xul.dll!nsPagePrintTimer::Release()  Line 11 + 0xd bytes	C++
 	xul.dll!nsCOMPtr<nsITimerCallback>::~nsCOMPtr<nsITimerCallback>()  Line 495	C++
 	xul.dll!mozilla::dom::HTMLCanvasPrintState::~HTMLCanvasPrintState()  Line 131 + 0xb bytes	C++
 	xul.dll!mozilla::dom::HTMLCanvasPrintState::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!mozilla::dom::HTMLCanvasPrintState::Release()  Line 141 + 0x174 bytes	C++
 	xul.dll!nsCOMPtr<mozilla::dom::HTMLCanvasPrintState>::assign_assuming_AddRef(mozilla::dom::HTMLCanvasPrintState * newPtr)  Line 480	C++
 	xul.dll!nsCOMPtr<mozilla::dom::HTMLCanvasPrintState>::assign_with_AddRef(nsISupports * rawPtr)  Line 1156	C++
 	xul.dll!nsCOMPtr<mozilla::dom::HTMLCanvasPrintState>::operator=(mozilla::dom::HTMLCanvasPrintState * rhs)  Line 625	C++
 	xul.dll!mozilla::dom::HTMLCanvasElement::ResetPrintCallback()  Line 295	C++
 	xul.dll!nsSimplePageSequenceFrame::ResetPrintCanvasList()  Line 672	C++
 	xul.dll!nsSimplePageSequenceFrame::~nsSimplePageSequenceFrame()  Line 123	C++
 	xul.dll!nsSimplePageSequenceFrame::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 684	C++
 	xul.dll!nsSplittableFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 44	C++
 	xul.dll!nsContainerFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 250	C++
 	xul.dll!nsFrameList::DestroyFramesFrom(nsIFrame * aDestructRoot)  Line 62	C++
 	xul.dll!nsContainerFrame::DestroyFrom(nsIFrame * aDestructRoot)  Line 232	C++
 	xul.dll!nsIFrame::Destroy()  Line 599 + 0x18 bytes	C++
 	xul.dll!nsFrameManager::Destroy()  Line 221	C++
 	xul.dll!nsCSSFrameConstructor::WillDestroyFrameTree()  Line 8603	C++
 	xul.dll!PresShell::Destroy()  Line 1086	C++
 	xul.dll!nsPrintObject::DestroyPresentation()  Line 117	C++
 	xul.dll!nsPrintObject::~nsPrintObject()  Line 43	C++
 	xul.dll!nsPrintObject::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsPrintData::~nsPrintData()  Line 226 + 0x1f bytes	C++
 	xul.dll!nsPrintData::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsPrintEngine::Destroy()  Line 255 + 0x1f bytes	C++
 	xul.dll!nsDocumentViewer::OnDonePrinting()  Line 4269	C++
 	xul.dll!nsPrintCompletionEvent::Run()  Line 3650	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 627 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, bool mayWait)  Line 238 + 0x17 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 82 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 216	C++
 	xul.dll!MessageLoop::RunHandler()  Line 209	C++
 	xul.dll!MessageLoop::Run()  Line 183	C++

The crash happens because during the initial print data cleanup in nsDocumentViewer::OnDonePrinting (since printing is finished), we destroy the frame tree, which involves releasing the callback on the HTMLCanvasElement, which releases the page print timer, which destroys the nsDocumentViewer (whose OnDonePrinting method is still on the stack, mind you), which releases the pres shell a call to that pres shell's Destroy() is also still on the stack.

Comment 1

[Cameron McCormack (:heycam)]

I found this while trying to reproduce the crash from bug 818626.  I'm wondering whether they have the same root cause.

Comment 2

[Cameron McCormack (:heycam)]

We could avoid the crash here by making nsPrintEngine::Destroy check to see whether it is already in the process of being destroyed, and not do anything if so.

Comment 3

[Mats Palmgren (inactive)]

That sounds like the right fix to me.
Additionally I think nsPrintObject::DestroyPresentation() should own the shell on
the stack (and possibly null out mPresShell) before calling Destroy on it (it's a
requirement that the caller of PresShell::Destroy keeps it alive until completion).

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Mats Palmgren [:mats] from comment #3)
> Additionally I think nsPrintObject::DestroyPresentation() should own the
> shell on
> the stack (and possibly null out mPresShell) before calling Destroy on it
> (it's a
> requirement that the caller of PresShell::Destroy keeps it alive until
> completion).

Oh, that's good information.  I think that could be the answer to bug 818626.

Comment 5

[Cameron McCormack (:heycam)]

Hmm, if I put a check in nsPrintEngine::Destroy, this avoids the comment 0 crash but then proceeds to this different crash, which I'm much less certain of (I'm unfamiliar with GC stuff):

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x0000000101b402a4 in CallQueryInterface<nsISupports, nsWrapperCache> (aSource=0x14ca6bca0, aDestination=0x7fff5fbfc630) at nsISupportsUtils.h:148
148	    return aSource->QueryInterface(NS_GET_TEMPLATE_IID(DestinationType),
(gdb) bt
#0  0x0000000101b402a4 in CallQueryInterface<nsISupports, nsWrapperCache> (aSource=0x14ca6bca0, aDestination=0x7fff5fbfc630) at nsISupportsUtils.h:148
#1  0x0000000102add7ab in XPCWrappedNative::FlatJSObjectFinalized (this=0x13f0da570) at /z/mozilla-inbound/js/xpconnect/src/XPCWrappedNative.cpp:1310
#2  0x0000000102af436f in WrappedNativeFinalize (fop=0x7fff5fbfcbd8, obj=0x11f628af0, helperType=WN_NOHELPER) at /z/mozilla-inbound/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:636
#3  0x0000000102aed422 in XPC_WN_NoHelper_Finalize (fop=0x7fff5fbfcbd8, obj=0x11f628af0) at /z/mozilla-inbound/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:642
#4  0x00000001047e9116 in JSObject::finalize (this=0x11f628af0, fop=0x7fff5fbfcbd8) at jsobjinlines.h:239
#5  0x00000001047e8cd9 in js::gc::Arena::finalize<JSObject> (this=0x11f628000, fop=0x7fff5fbfcbd8, thingKind=js::gc::FINALIZE_OBJECT2, thingSize=48) at /z/mozilla-inbound/js/src/jsgc.cpp:349
#6  0x00000001047d010b in FinalizeTypedArenas<JSObject> (fop=0x7fff5fbfcbd8, src=0x7fff5fbfc940, dest=@0x1219b93b0, thingKind=js::gc::FINALIZE_OBJECT2, budget=@0x7fff5fbfc930) at /z/mozilla-inbound/js/src/jsgc.cpp:413
#7  0x00000001047c3f08 in FinalizeArenas (fop=0x7fff5fbfcbd8, src=0x7fff5fbfc940, dest=@0x1219b93b0, thingKind=js::gc::FINALIZE_OBJECT2, budget=@0x7fff5fbfc930) at /z/mozilla-inbound/js/src/jsgc.cpp:450
#8  0x00000001047d2594 in js::gc::ArenaLists::finalizeNow (this=0x1219b9240, fop=0x7fff5fbfcbd8, thingKind=js::gc::FINALIZE_OBJECT2) at /z/mozilla-inbound/js/src/jsgc.cpp:1222
#9  0x00000001047c4157 in js::gc::ArenaLists::queueObjectsForSweep (this=0x1219b9240, fop=0x7fff5fbfcbd8) at /z/mozilla-inbound/js/src/jsgc.cpp:1318
#10 0x00000001047cdb5f in BeginSweepingCompartmentGroup (rt=0x10cb83000) at /z/mozilla-inbound/js/src/jsgc.cpp:3445
#11 0x00000001047cc4b6 in SweepPhase (rt=0x10cb83000, sliceBudget=@0x7fff5fbfccc0) at /z/mozilla-inbound/js/src/jsgc.cpp:3583
#12 0x00000001047cb680 in IncrementalCollectSlice (rt=0x10cb83000, budget=40000, reason=js::gcreason::INTER_SLICE_GC, gckind=js::GC_NORMAL) at /z/mozilla-inbound/js/src/jsgc.cpp:4033
#13 0x00000001047caa8c in GCCycle (rt=0x10cb83000, incremental=true, budget=40000, gckind=js::GC_NORMAL, reason=js::gcreason::INTER_SLICE_GC) at /z/mozilla-inbound/js/src/jsgc.cpp:4160
#14 0x00000001047c927b in Collect (rt=0x10cb83000, incremental=true, budget=40000, gckind=js::GC_NORMAL, reason=js::gcreason::INTER_SLICE_GC) at /z/mozilla-inbound/js/src/jsgc.cpp:4278
#15 0x00000001047c624d in js::GCSlice (rt=0x10cb83000, gckind=js::GC_NORMAL, reason=js::gcreason::INTER_SLICE_GC, millis=40) at /z/mozilla-inbound/js/src/jsgc.cpp:4316
#16 0x00000001047b17da in js::IncrementalGC (rt=0x10cb83000, reason=js::gcreason::INTER_SLICE_GC, millis=40) at /z/mozilla-inbound/js/src/jsfriendapi.cpp:190
#17 0x00000001021ddef1 in nsJSContext::GarbageCollectNow (aReason=js::gcreason::INTER_SLICE_GC, aIncremental=nsJSContext::IncrementalGC, aCompartment=nsJSContext::CompartmentGC, aShrinking=nsJSContext::NonShrinkingGC, aSliceMillis=40) at /z/mozilla-inbound/dom/base/nsJSEnvironment.cpp:2945
#18 0x00000001021e8223 in InterSliceGCTimerFired (aTimer=0x131bcf0b0, aClosure=0x0) at /z/mozilla-inbound/dom/base/nsJSEnvironment.cpp:3262
#19 0x00000001038dd900 in nsTimerImpl::Fire (this=0x131bcf0b0) at /z/mozilla-inbound/xpcom/threads/nsTimerImpl.cpp:482

Comment 6

[Cameron McCormack (:heycam)]

Ah, but if I also hold on to the pres shell during DestroyPresentation, then we get this:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x0000000101720843 in nsDocumentViewer::Release (this=0x125fd6ff0) at /z/mozilla-inbound/layout/base/nsDocumentViewer.cpp:555
555	NS_IMPL_RELEASE(nsDocumentViewer)
(gdb) bt
#0  0x0000000101720843 in nsDocumentViewer::Release (this=0x125fd6ff0) at /z/mozilla-inbound/layout/base/nsDocumentViewer.cpp:555
#1  0x000000010172092c in non-virtual thunk to nsDocumentViewer::Release() () at /z/mozilla-inbound/layout/base/nsDocumentViewer.cpp:555
#2  0x0000000102771ebb in nsCOMPtr<nsIDocumentViewerPrint>::~nsCOMPtr (this=0x112f206b8) at nsCOMPtr.h:494
#3  0x000000010276ffc5 in nsCOMPtr<nsIDocumentViewerPrint>::~nsCOMPtr (this=0x112f206b8) at nsCOMPtr.h:491
#4  0x0000000102772075 in nsPrintCompletionEvent::~nsPrintCompletionEvent (this=0x112f206a0) at /z/mozilla-inbound/layout/printing/nsPrintEngine.cpp:3647
#5  0x0000000102772005 in nsPrintCompletionEvent::~nsPrintCompletionEvent (this=0x112f206a0) at /z/mozilla-inbound/layout/printing/nsPrintEngine.cpp:3647
#6  0x0000000102772029 in nsPrintCompletionEvent::~nsPrintCompletionEvent (this=0x112f206a0) at /z/mozilla-inbound/layout/printing/nsPrintEngine.cpp:3647
#7  0x000000010383b626 in nsRunnable::Release (this=0x112f206a0) at nsThreadUtils.cpp:31
#8  0x00000001011a852b in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x7fff5fbfd1e8) at nsCOMPtr.h:494
#9  0x00000001011a7e65 in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x7fff5fbfd1e8) at nsCOMPtr.h:491
#10 0x00000001038d3575 in nsThread::ProcessNextEvent (this=0x10ba014b0, mayWait=false, result=0x7fff5fbfd283) at /z/mozilla-inbound/xpcom/threads/nsThread.cpp:633

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

nsDocumentViewer::OnDonePrinting() has the same problem with its
mPrintEngine member - it doesn't keep it alive and the null-check
there doesn't really help against re-entrancy.  Fixing it in
the same way as for mPresShell in nsPrintObject::DestroyPresentation
fixes the crash(es).  (we want to null it out *before* calling
Destroy to protect against re-entrancy)

There's a similar problem with nsPrintEngine::Destroy -
nulling out the members after the 'delete' doesn't help
if its destructor calls a bunch of stuff that ends up calling
nsPrintEngine::Destroy again (which is the case here!). 
In this case I opted for a bool flag since it's simpler than adding
code for each member.

Comment 8

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/103e105654e5

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/103e105654e5

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 701751 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): pdf.js triggers this longstanding bug
User impact if declined: Printing a pdf.js document will sometimes cause a crash.
Testing completed (on m-c, etc.): Been on m-c for a few days, no more crashes since it landed.
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: N/A

This seems to fix the crash bug from bug 818626.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 701751 [details] [diff] [review]
fix

Approving this low risk crash fix for branches, we'll want this for pdf.js release.

Comment 12

[Mats Palmgren (inactive)]

While preparing the branch patches I discovered an error in this patch.
In nsDocumentViewer::OnDonePrinting() the patch now nulls out mPrintEngine
on line 4266, before GetIsPrintPreview() is called.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#4261
that makes GetIsPrintPreview() always return false:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#4145
so the result is that we always Destroy() the print engine.

This breaks:
1. load any document
2. Print Preview
3. Print
=> after Print is done, the Print Preview content is blank.  The Close button
   in Print Preview does not work.

Comment 13

[Cameron McCormack (:heycam)]

So we should only null out mPrintEngine in the else branch of that if statement?

Comment 14

[Mats Palmgren (inactive)]

Yeah, and then we should also make nsPrintEngine::DestroyPrintingData()
re-entrancy safe I think, like so:
https://hg.mozilla.org/try/rev/fa5d898c50a8

I'll attach the fix in a moment, I just want to investigate something first...

Comment 15

[Mats Palmgren (inactive)]

Printing from inside Print Preview is weird.  It uses the same nsPrintEngine
and just calls SetIsPrinting(true) (the PE has mIsDoingPrintPreview==true at
that point, so now both are true).  A comment says DocumentViewer is confused
by that so it isn't informed though...
http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#3037

The nsDocumentViewer::OnDonePrinting() has GetIsPrintPreview()==true and
GetIsPrinting()==false  though, because it occurs after SetIsPrinting(false).
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#4249
(the comment there explains it to some degree, but I missed it, my bad)

Comment 16

[Mats Palmgren (inactive)]

Attachment: follow-up fix

Comment 17

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc1c3d2e2c4

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 704222 [details] [diff] [review]
follow-up fix

Correction to the first patch.  Low risk.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5dc1c3d2e2c4

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 704222 [details] [diff] [review]
follow-up fix

Please land today to make it into Beta 3.

Comment 21

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2510eb4709c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/5582b6d395e9

https://hg.mozilla.org/releases/mozilla-beta/rev/da193b95904d
https://hg.mozilla.org/releases/mozilla-beta/rev/68d81a68b234

Comment 22

[Mihai Morar, (:MihaiMorar)]

I could easily reproduce this issue on Ubuntu x86 and Windows 7x64 following STR from Comment 0. Does FF 19b3 contains fix patch created in Comment 21 ?

Crashes reports:

bp-da4d2194-8d11-482a-853e-413422130124
bp-c54caaa9-d4cf-43ea-a169-fc5082130124

Firefox build: 19b2 on Ubuntu x86.

Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0(20130116072953)

Comment 23

[Scoobidiver (away)]

(In reply to MarioMi from comment #22)
> Firefox build: 19b2 on Ubuntu x86.
The fix is in 19.0b3: http://hg.mozilla.org/releases/mozilla-beta/graph

Comment 24

[Mihai Morar, (:MihaiMorar)]

(In reply to Scoobidiver from comment #23)
> The fix is in 19.0b3: http://hg.mozilla.org/releases/mozilla-beta/graph

Verified fixed on FF 19b3 on Windows 7 and Ubuntu x86 following STR from Comment 0.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0

Comment 25

[Mats Palmgren (inactive)]

Thanks for testing.

Comment 26

[Mihai Morar, (:MihaiMorar)]

(In reply to Mats Palmgren [:mats] from comment #25)
> Thanks for testing.

Should I verify on Aurora 20 and Nightly 21 too as long as you set Status as Verified even if flags are still fixed ?

Comment 27

[Mats Palmgren (inactive)]

Sure, if you want to.  I was mostly interested in hearing that it works on Windows
which I haven't tested.

Comment 28

[Mihai Morar, (:MihaiMorar)]

I confirm this fix is verified too, on Latest Aurora and Latest Nightly on Windows 7 x64.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130123 Firefox/20.0(20130123042017)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130123 Firefox/21.0(20130123052720)