362734 - Crash [@ nsPrintEngine::DocumentReadyForPrinting] with testcase that sets print preview, then reloads the page

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, you need to download the testcase locally to get the crash (because of the use of enhanced privileges).

Talkback ID: TB26872610W
nsPrintEngine::DocumentReadyForPrinting  [mozilla\layout\printing\nsprintengine.cpp, line 1857]
XPCWrappedNative::CallMethod  [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2162]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase1

Comment 2

[-fullmetaljacket-]

confirmed!

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=TB26899770X

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a1) Gecko/20061204 Minefield/3.0a1 ID:2006120418 [cairo]

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2

I can also reproduce this crash with this testcase, to reproduce:
Right-click in the iframe
Choose This Frame, choose Print Frame
Then print the frame, Mozilla should not crash doing that.

Comment 4

[Matthew Middleton (:zzxc)]

Both testcases WFM with 20070528 trunk build on Mac OS X.

Comment 5

[Carsten Book [:Tomcat]]

both testcases works for me with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a6pre) Gecko/20070612 Minefield/3.0a6pre ID:2007061212 [cairo]

Is this maybe fixed now ?

Comment 6

[Martijn Wargers (dead)]

Still crashes for me with current trunk build.

Comment 7

[Stuart Parmenter]

This looks like it is happening due to the print engine being destroyed by the document viewer code before the print engine's observe function gets called to end printing.  I don't understand any of this code really, but looks like eli was the last one to touch it.

Comment 8

[Andrew Smith]

The problem is that mPreparingForPrint and mDocWasToBeDestroyed are basically unused (they're either only ever set to defaults or their value is always ignored).

So when the document is destroyed while the print dialog is up nsPrintEngine::CheckBeforeDestroy() checks mPreparingForPrint and sees it's false. And bad things happen after that.

I didn't quite figure out what can be done about this, but thought it would be a good idea to record what I found so far before I forget about it.

Also if anyone knows when the use of these variables was discontinued - please share, that will help.

Comment 9

[Andrew Smith]

Attachment: preliminary patch

I wonder if the two test cases are the same bug. This patch solves the problem with the second testcase by simply setting mPrt->mPreparingForPrint to true when printing starts.

This way the DocumentViewerImpl isn't destroyed in the first call to Destroy() but some time later via the destructor (I don't actually know what triggers the destructor, but no matter). That takes care of the crash and also the cleanup. It also causes two assertions: 

- the one in DocumentViewerImpl::~DocumentViewerImpl(), which sounds more like a statement of fact than a warning, and
- the one in DocumentViewerImpl::Destroy() which is understandable seing how the document has been destroyed already in the destructor

I don't think I'll bother to fix the assertions. Now I'll try to figure out the other testcase.

Comments anyone?

Comment 10

[Andrew Smith]

Attachment: possible fix for first testcase

Hm.. I declare the two testcases are triggering different bugs :)

The second testcase has to do with printing and the first with print previews. The two are quite different, even though they seem to both live in nsPrintEngine.cpp. But i figure they'd both be blockers anyway and I'm too lazy to file another bug.

The print preview crash (testcase 1) happens because nsPrintEngine::Observe() is called after the nsPrintEngine is destroyed. It can't (I don't think) be fixed the same way as the print crash... for complicated reasons I can't remember now.

This patch solves the print preview crash by calling mPrintEngine->FinishPrintPreview() in DocumentViewerImpl::Destroy(). But it also looks like (according to the output at the end of a debugging session) that after running testcase 1 and quitting there is memory leaked.

Tracking a memory leak in code I worked on for less than a week is hard. I could really use some advice. But no matter, I'll do what I can.

Sorry I left all the debug crap in the patch, it's work in progress.

Comment 11

[Andrew Smith]

Attachment: fix for both testcases

This is the bet I can do. The patch fixes both crashes, but there is still a memory leak.

I believe the patch exposes, rather than creates the memory leak for testcase 1, and that ought to be yet a third bug. Regardless, this is all I can do without more feedback.

Comment 12

[Martijn Wargers (dead)]

Andrew, did you want to ask review to someone for the patch?

Comment 13

[Andrew Smith]

No, I don't know who can/will do it.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Have you asked roc or Eli?

Comment 15

[Eli Friedman]

I'll take a look at it.

Comment 16

[Eli Friedman]

Comment on attachment 278586 [details] [diff] [review]
fix for both testcases

This code is a real mess.  I'm basically the owner of this code, and I don't completely understand the ownership model myself.  The patch looks reasonable, though.

Comment 17

[Martijn Wargers (dead)]

Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.544; previous revision: 1.543
done
Checking in layout/printing/nsPrintEngine.h;
/cvsroot/mozilla/layout/printing/nsPrintEngine.h,v  <--  nsPrintEngine.h
new revision: 1.26; previous revision: 1.25
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.153; previous revision: 1.152
done

Checked into trunk.

Andrew, thanks for fixing this!

Comment 18

[Martijn Wargers (dead)]

I can still crash with the first testcase, when clicking times on the button.
I filed bug 396024 for that (with testcase that reliably crashes).
This bug is verified fixed for the single click case on the first testcase and for the second testcase, using a 2007-09-12 trunk build.