Closed Bug 362734 Opened 14 years ago Closed 13 years ago
Crash [@ ns
Print Engine::Document Ready For Printing] with testcase that sets print preview, then reloads the page
1.78 KB, text/html
526 bytes, text/html
2.32 KB, patch
|Details | Diff | Splinter Review|
5.09 KB, patch
|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]
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]
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
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.
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9beta1
Both testcases WFM with 20070528 trunk build on Mac OS X.
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 ?
Still crashes for me with current trunk build.
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
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.
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?
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.
Attachment #247418 - Attachment description: testcase → testcase1
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.
Andrew, did you want to ask review to someone for the patch?
No, I don't know who can/will do it.
Have you asked roc or Eli?
I'll take a look at it.
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+
Attachment #278586 - Flags: superreview?(roc) → superreview+
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!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Crash Signature: [@ nsPrintEngine::DocumentReadyForPrinting]
You need to log in before you can comment on or make changes to this bug.