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

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: 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.

Comment 1

[Martijn Wargers (dead)]

The patch I attached in bug 299555 didn't fix this crash, fwiw.

Comment 2

[Carsten Book [:Tomcat]]

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

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2

This is a testcase, that swithces on print preview directly.
When closing this document, Mozilla should not crash.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch?

Moving some code around seems to make the crashes go away.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Dumb question -- can we just hook this into the cycle collector?

Comment 6

[Martijn Wargers (dead)]

Comment on attachment 283392 [details] [diff] [review]
patch?

I don't know, you mean that's the right solution?

Comment 7

[Stuart Parmenter]

Comment on attachment 283392 [details] [diff] [review]
patch?

eli can you take a look at this?

Comment 8

[Eli Friedman]

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.

Comment 9

[Reed Loden [:reed]]

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!

Comment 10

[Stuart Parmenter]

schrep: how come you minused this?

Comment 11

[Mike Schroepfer]

Greasemonkey script gone wrong - sorry about that - r.e. +

Comment 12

[Martijn Wargers (dead)]

I filed bug 405555 on an issue I still see after this bug.

Comment 13

[Martijn Wargers (dead)]

Attachment: patch with mochitest

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.

Comment 14

[Martijn Wargers (dead)]

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.

Comment 15

[Martijn Wargers (dead)]

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.

Comment 16

[Martijn Wargers (dead)]

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.

Comment 17

[Martijn Wargers (dead)]

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.

Comment 18

[Martijn Wargers (dead)]

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.

Comment 19

[Martijn Wargers (dead)]

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.

Comment 20

[Martijn Wargers (dead)]

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.

Comment 21

[Martijn Wargers (dead)]

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.

Comment 22

[Martijn Wargers (dead)]

Sorry for the spam.
I missed a return; statement, an simple mistake.
This time it should work (last try, I promise!)

Comment 23

[Martijn Wargers (dead)]

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. 

Comment 24

[Martijn Wargers (dead)]

Attachment: updated mochitest

This doesn't break with the fixed bug 407080 and still does crash builds before this bug was fixed.

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Serge Gautherie (:sgautherie)]

NB: Bug 408335 reverted the fix part. (Test part kept.)

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)
> NB: Bug 408335 reverted the fix part. (Test part kept.)

Bug 408355 !