Closed
Bug 362734
Opened 14 years ago
Closed 14 years ago
Crash [@ nsPrintEngine::DocumentReadyForPrinting] with testcase that sets print preview, then reloads the page
Categories
(Core :: Printing: Output, defect)
Core
Printing: Output
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha8
People
(Reporter: martijn.martijn, Assigned: asmith16)
Details
(Keywords: crash, testcase)
Crash Data
Attachments
(4 files, 1 obsolete file)
1.78 KB,
text/html
|
Details | |
526 bytes,
text/html
|
Details | |
2.32 KB,
patch
|
Details | Diff | Splinter Review | |
5.09 KB,
patch
|
sharparrow1
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
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]
Updated•14 years ago
|
OS: Windows XP → All
Hardware: PC → All
Summary: Crash [@ nsPrintEngine::DocumentReadyForPrinting] with testcase that set print preview, then reloads the page → Crash [@ nsPrintEngine::DocumentReadyForPrinting] with testcase that sets print preview, then reloads the page
Reporter | ||
Comment 3•14 years ago
|
||
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.
Reporter | ||
Updated•14 years ago
|
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9beta1
Comment 4•14 years ago
|
||
Both testcases WFM with 20070528 trunk build on Mac OS X.
Comment 5•14 years ago
|
||
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 ?
Reporter | ||
Comment 6•14 years ago
|
||
Still crashes for me with current trunk build.
Comment 7•14 years ago
|
||
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.
Assignee: printing → sharparrow1
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #247418 -
Attachment description: testcase → testcase1
Assignee | ||
Comment 11•14 years ago
|
||
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.
Attachment #277854 -
Attachment is obsolete: true
Attachment #278586 -
Flags: review?
Reporter | ||
Comment 12•14 years ago
|
||
Andrew, did you want to ask review to someone for the patch?
Assignee | ||
Comment 13•14 years ago
|
||
No, I don't know who can/will do it.
Comment 14•14 years ago
|
||
Have you asked roc or Eli?
Comment 15•14 years ago
|
||
I'll take a look at it.
Comment 16•14 years ago
|
||
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.
Attachment #278586 -
Flags: review? → review+
Assignee | ||
Updated•14 years ago
|
Attachment #278586 -
Flags: superreview?(roc)
Attachment #278586 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•14 years ago
|
Assignee: sharparrow1 → asmith15
Reporter | ||
Comment 17•14 years ago
|
||
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!
Reporter | ||
Comment 18•14 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
Crash Signature: [@ nsPrintEngine::DocumentReadyForPrinting]
You need to log in
before you can comment on or make changes to this bug.
Description
•