Closed Bug 396024 Opened 13 years ago Closed 13 years ago

Crash [@ nsPrintEngine::DocumentReadyForPrinting] with testcase that set print preview, then reloads the page, part 2

Categories

(Core :: Printing: Output, defect, P2, critical)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(5 files)

Attached file testcase
See testcase, when clicking on the button, Mozilla crashes after 500ms.
You need to download the testcase to your computer, because of the use of enhanced privileges.

http://crash-stats.mozilla.com/report/index/40e75a45-61f7-11dc-9dee-001a4bd43ef6
0  	nsPrintEngine::DocumentReadyForPrinting()  	 mozilla/layout/printing/nsPrintEngine.cpp:1389
1 	nsPrintEngine::FinishPrintPreview() 	mozilla/layout/printing/nsPrintEngine.cpp:3014
2 	DocumentViewerImpl::Destroy() 	mozilla/layout/base/nsDocumentViewer.cpp:1471
3 	DocumentViewerImpl::Show() 	mozilla/layout/base/nsDocumentViewer.cpp:1798
4 	nsPresContext::EnsureVisible(int) 	mozilla/layout/base/nsPresContext.cpp:1438
5 	PresShell::UnsuppressAndInvalidate() 	mozilla/layout/base/nsPresShell.cpp:4199
6 	PresShell::UnsuppressPainting() 	mozilla/layout/base/nsPresShell.cpp:4259
7 	DocumentViewerImpl::LoadComplete(unsigned int) 	mozilla/layout/base/nsDocumentViewer.cpp:993
8 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:4930
9 	nsWebShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsWebShell.cpp:1014
10 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:4830
etc..

I'm setting this to blocking1.9? flag, because with the steps to reproduce in bug 299555, I crash now with current trunk build. I think that's basically what this bug is about.
Flags: blocking1.9?
Blocks: 299555
The patch I attached in bug 299555 didn't fix this crash, fwiw.
confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre ID:2007092605 with the testcase. I crash also with the same steps to reproduce
Attached file testcase2
This is a testcase, that swithces on print preview directly.
When closing this document, Mozilla should not crash.
Attached patch patch?Splinter Review
Moving some code around seems to make the crashes go away.
Attachment #283392 - Flags: review?(asmith15)
Blocks: 362711
Dumb question -- can we just hook this into the cycle collector?
Comment on attachment 283392 [details] [diff] [review]
patch?

I don't know, you mean that's the right solution?
Attachment #283392 - Flags: review?(asmith15)
Comment on attachment 283392 [details] [diff] [review]
patch?

eli can you take a look at this?
Attachment #283392 - Flags: review?(sharparrow1)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment on attachment 283392 [details] [diff] [review]
patch?

Looks okay, I guess... not especially pretty, but none of this code is very nice.

Please put in a comment as to why you're moving it, though, so someone doesn't accidentally reintroduce this crash.
Attachment #283392 - Flags: review?(sharparrow1) → review+
Assignee: nobody → martijn.martijn
Attachment #283392 - Flags: superreview?(roc)
Attachment #283392 - Flags: superreview?(roc) → superreview+
Flags: blocking1.9+ → blocking1.9-
Keywords: checkin-needed
Comment on attachment 283392 [details] [diff] [review]
patch?

Uh, since schrep changed blocking status of this, can't really check it in since it no longer has approval!
Attachment #283392 - Flags: approval1.9?
schrep: how come you minused this?
Greasemonkey script gone wrong - sorry about that - r.e. +
Flags: blocking1.9- → blocking1.9+
Attachment #283392 - Flags: approval1.9?
I filed bug 405555 on an issue I still see after this bug.
Sorry for the delay, I now have added a comment as was suggested by Eli, I also added a Mochitest.
Could you review that, roc?

I'm doing some strange stuff in the mochitests, but I have to do that because of some issues in Mozilla, I filed those as bugs and mentioned the bug numbers in the mochitest.
Attachment #290346 - Flags: superreview?(roc)
Attachment #290346 - Flags: review?(roc)
Attachment #290346 - Attachment is patch: true
Attachment #290346 - Attachment mime type: application/octet-stream → text/plain
Attachment #290346 - Flags: superreview?(roc)
Attachment #290346 - Flags: superreview+
Attachment #290346 - Flags: review?(roc)
Attachment #290346 - Flags: review+
Checking in nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.556; previous revision: 1.555
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/layout/base/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvsroot/mozilla/layout/base/tests/test_bug396024.html,v
done
Checking in tests/test_bug396024.html;
/cvsroot/mozilla/layout/base/tests/test_bug396024.html,v  <--  test_bug396024.ht
ml
initial revision: 1.1
done

Checked in to trunk.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I disabled the mochitest for now to see if it fixes orange on MacOSX Darwin 8.8.4 qm-xserve01 dep unit test and WINNT 5.1 qm-winxp01 dep unit test.
Flags: in-testsuite+ → in-testsuite?
I tested now locally all of the tests of Mochikit (at least, until after the point of my test). They run fine here locally.
I'll try again tomorrow to see what is going on with my test.
Ok, it turns out those machines don't have any printers installed, that's why an error dialog as mentioned in bug 398342 comes up.
I can fix my test to not throw when there are no printers installed, but at least one of the machines needs to have a printer installed, otherwise it won't be doing anything useful.

So I filed bug 406063 as an IT request.
Checking in Makefile.in;
/cvsroot/mozilla/layout/base/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
Checking in test_bug396024.html;
/cvsroot/mozilla/layout/base/tests/test_bug396024.html,v  <--  test_bug396024.ht
ml
new revision: 1.3; previous revision: 1.2
done

I changed the mochitest so that it doesn't cause tinderboxes to go orange, it will now add a Todo when no printer is installed on that machine.
So this is now in-testsuite+, although it will only be really tested when bug 406063 is fixed.
Flags: in-testsuite? → in-testsuite+
I had to disable the mochitest again, because Mac failed.
This error appears:
*** 17893 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIPrintSettingsService.defaultPrinterName]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: http://localhost:8888/tests/layout/base/tests/test_bug396024.html :: run :: line 59"  data: no] | got 0, expected 1 | /tests/layout/base/tests/test_bug396024.html


On IRC:
Mossop>	mw22: The code attempts to use @mozilla.org/gfx/printerenumerator;1 which doesnt appear to exist here.

So  I'm not sure currently how to test on Mac that there isn't a printer installed.
Flags: in-testsuite+ → in-testsuite?
Ok, it turns out this test can never work on the Mac (print preview is done in some native way), so I disabled this test for the Mac completely now and checked it in again.

I've disabled the Mac in this way:
  try {
     var printerEnumerator = Components.classes["@mozilla.org/gfx/printerenumerator;1"]
                             .getService(Components.interfaces.nsIPrinterEnumerator);
  } catch(e) {
     //don't try to test on Mac, since the print preview code doesn't work there
     SimpleTest.finish();
  }

I tested that this errors out on the Mac (thanks tchung!).

So I think the test will stick this time.
Flags: in-testsuite? → in-testsuite+
Unfortunately, the test didn't stick :(
So apparently, I did something wrong still.
I guess someone with a Mac should try and find out how and why the test is going wrong and improve it.
Flags: in-testsuite+ → in-testsuite?
Sorry for the spam.
I missed a return; statement, an simple mistake.
This time it should work (last try, I promise!)
Flags: in-testsuite? → in-testsuite+
Depends on: 405555
Depends on: 408355
The patch for bug 407080 broke this mochitest, but that's because the mochitest is flawed (perhaps kind of depends on what you expect to happen?). It's causing the "The browser cannot print preview right now.
Please try again when the page has finished loading." dialog to appear, which is very annoying. 
Depends on: 407080
Attached file updated mochitest
This doesn't break with the fixed bug 407080 and still does crash builds before this bug was fixed.
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre - no crash on testcases 

--> Verified fixed
Status: RESOLVED → VERIFIED
Depends on: 433132
NB: Bug 408335 reverted the fix part. (Test part kept.)
Target Milestone: --- → mozilla1.9
(In reply to comment #26)
> NB: Bug 408335 reverted the fix part. (Test part kept.)

Bug 408355 !
No longer blocks: 492476
Depends on: 492476
No longer blocks: 556686
Depends on: 556686
Depends on: 561048
Crash Signature: [@ nsPrintEngine::DocumentReadyForPrinting]
You need to log in before you can comment on or make changes to this bug.