This is a spin-off of bug 629500, and the patch I will add as soon as I know the bug number used to be part of that patch series. In that bug, I removed the mechanism that the print preview tests (layout/base/tests/chrome/*printpreview*) were using to detect when they wouldn't work, so instead I just ran them unconditionally, on the theory that the print preview infrastructure ought to work even when we don't expose "print preview" as a UI command (OSX). That was working fine, but has since started failing on our Windows XP build robots. My suspicion is that those don't have any configured printers, and for some reason this makes nsIWebBrowserPrint::PrintPreview() hang, which seems like a bug in its own right. But perhaps one to be addressed separately. Hence this spin-off. The patch I'll add doesn't fix the above problem, but it modernizes the print preview tests a bit -- notably going through the docshell to get a nsIWebBrowserPrint that's properly initialized -- and fixes one of them so it actually tests what it's supposed to (a previous mass change accidentally deleted a critical HTML snippet). Hopefully this will make it easier to pin down the problem.

Forgot to mention that this also adds a bunch of defensive try/catch statements to the tests. I was hoping that these would convert the XP timeouts into plain failures, but they don't, alas; still, I think they are worthy. I do not have any computers with Windows XP on them, and even if I did, I suspect the problem may be specific to our build farm's configuration.

The expectation here is that there will be a second code-changing patch that fixes the source of the XP hangs, once we figure out what that is.

Try run for 460971226266 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=460971226266 Results (out of 209 total builds): success: 192 warnings: 17 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/zackw@panix.com-460971226266

Logs for XP hangs from the above build cycle: https://tbpl.mozilla.org/php/getParsedLog.php?id=7409665&tree=Try (opt) https://tbpl.mozilla.org/php/getParsedLog.php?id=7410371&tree=Try (debug) The Ru failure appears to be unrelated (bug 696670, bug 696671, bug 696673, bug 696674).

Comment on attachment 574618 [details] [diff] [review] 1/n: overhaul print preview tests and run them unconditionally So, this bug is still waiting for the fix for the hang problem.

Zack, I think you should submit to Try to check were we stand (wrt the hang). PS: If necessary, I currently have a WinXP with no printer (to run checks/tests)...

Try server is being very, very slow today, but the relevant tests do indeed still fail on our Windows XP testers: 11787 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | Test timed out. 11791 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview_bug396024.xul | Test timed out. 11795 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview_bug482976.xul | Test timed out. Some kind soul has arranged for them to take screen shots when this happens and stick them into the build log; that's the attachment. It looks like the problem is indeed that there are no configured printers and this wedges the test. Tomorrow, I will try to find the code that's producing the offending dialog box.

(In reply to Zack Weinberg (:zwol) from comment #7) > Some kind soul has arranged for them to take screen shots when this happens Bug 589668 :-) > It looks like the problem is indeed that there are no configured printers Yeah: "Print Preview Error // There must be at least one printer available to show Print Preview." Windows alert is quite obvious ;-> > I will try to find the code that's producing the offending dialog box. See http://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_GFX_PRINTER_PRINTPREVIEW&case=on

Comment on attachment 574618 [details] [diff] [review] 1/n: overhaul print preview tests and run them unconditionally Review of attachment 574618 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/chrome/printpreview_bug396024_helper.xul @@ -67,5 @@ > - finish(); > - return; > - } > - > - if (printService.defaultPrinterName != '') { The regression would seem to be that you removed this check... (in the 3 helper files)

(In reply to Serge Gautherie (:sgautherie) from comment #9) > > The regression would seem to be that you removed this check... (in the 3 > helper files) Yeah, because printService.defaultPrinterName is going away in bug 629500. We need to do something else now. The original theory was that we *shouldn't* need any configured printers to do a print preview. > See > http://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_GFX_PRINTER_PRINTPREVIEW&case=on Thanks.

https://tbpl.mozilla.org/?tree=Try&rev=9cbc24038b13 has the complete try results now.

Comment on attachment 574618 [details] [diff] [review] 1/n: overhaul print preview tests and run them unconditionally New patch series coming shortly.

Part 1: run the print preview tests unconditionally, and convert all exceptions into failures. (It seems to me that SimpleTest ought to do this for us, but I guess at least this way I have a chance to make sure everything is torn down properly.)

Hat tip to dolske for the technique (borrowed from the password manager tests).

Having thoroughly buttressed the tests, the actual _fix_ here is just to nuke NS_ERROR_GFX_PRINTER_PRINTPREVIEW and make nsDeviceContextSpecWin::Init succeed even if there is no printer configured, as long as we're just doing print preview. (This is consistent with all the other platforms.)

I locally verified that with parts 1 and 2, the tests still pass when nsDeviceContextSpecG::Init works, and fail promptly and without leaving any detritus when nsDeviceContextSpecG::Init is hacked to always fail. Try server builds are running now, and hopefully will actually post results this time.

Comment on attachment 627302 [details] [diff] [review] 2/3: Make print preview tests fail, rather than hanging, if the print engine's modal error dialog appears. Review of attachment 627302 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/chrome/printpreview_common.js @@ +85,5 @@ > + var childDoc = childDocShell.QueryInterface(Ci.nsIDocShell) > + .contentViewer > + .DOMDocument; > + if (childDoc.location.href === > + "chrome://global/content/commonDialog.xul") So the error dialog you're looking for is a standard commonDialog.xul (ie, the print stuff was just using prompt service's .alert() or whatever). I assumed it was something different, but same approach wfm either way! :)

(In reply to Justin Dolske [:Dolske] from comment #17) > Comment on attachment 627302 [details] [diff] [review] ... > So the error dialog you're looking for is a standard commonDialog.xul (ie, > the print stuff was just using prompt service's .alert() or whatever). I > assumed it was something different, but same approach wfm either way! :) Yes, it is exactly using prompt service's .alert(). Via the deprecated window watcher methods, but I think I'll leave that as is till I get to bug 650960.

Comment on attachment 627304 [details] [diff] [review] when doing print preview, don't fail nsDeviceContextSpecWin::Init just because there is no configured printer. (Makes Windows consistent with all other platforms.) My review powers don't extend into widget land, though this looks like it's _mostly_ just a rubberstamp. Perhaps you can find someone on IRC to do so within minutes. :)

(In reply to Justin Dolske [:Dolske] from comment #19) > ... this looks like it's > _mostly_ just a rubberstamp. Perhaps you can find someone on IRC to do so > within minutes. :) Let's not be too hasty; for all I know that doesn't even compile, let alone solve the problem. I'll worry about getting it reviewed if tryserver is happy with it.

(In reply to Zack Weinberg (:zwol) from comment #13) > It seems to me that SimpleTest ought to do this for us That looks like bug 718205, doesn't it?

Comment on attachment 627302 [details] [diff] [review] 2/3: Make print preview tests fail, rather than hanging, if the print engine's modal error dialog appears. Review of attachment 627302 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/chrome/printpreview_bug396024_helper.xul @@ +52,1 @@ > window.close(); stopDialogMonitor() should be first. SimpleTest.finish() <-> window.close() is "wrong" :-/ See bug 749872 for a possible way to avoid it. NB: Same in the 2 other helpers. ::: layout/base/tests/chrome/printpreview_common.js @@ +53,5 @@ > + > + observe : function (subject, topic, data) { > + var doc = getDialogDoc(); > + if (doc) { > + ok(false, "Printing error: " + Shouldn't there be a specific "no printer" case with a todo(false, ...)? (Or is that handled elsewhere?)

(In reply to Serge Gautherie (:sgautherie) from comment #22) > Shouldn't there be a specific "no printer" case with a todo(false, ...)? (Or > is that handled elsewhere?) Oh, that's patch 3/3: confused by each patch adding an "error" that the next patch fixes :-| You are eventually fixing bug 135664, aren't you?

(In reply to Serge Gautherie (:sgautherie) from comment #21) > (In reply to Zack Weinberg (:zwol) from comment #13) > > It seems to me that SimpleTest ought to do this for us > > That looks like bug 718205, doesn't it? Yes, except this is a chrome mochitest rather than a plain one. (In reply to Serge Gautherie (:sgautherie) from comment #22) > > window.close(); > > stopDialogMonitor() should be first. Duh, of course. Don't know what I was thinking. Will fix. > SimpleTest.finish() <-> window.close() is "wrong" :-/ > See bug 749872 for a possible way to avoid it. Could you be more specific, please? (In reply to Serge Gautherie (:sgautherie) from comment #23) > Oh, that's patch 3/3: confused by each patch adding an "error" that the next > patch fixes :-| It was meant to be easier to review that way. Sorry for the confusion. > You are eventually fixing bug 135664, aren't you? I may in fact have fixed it right here :) Will experiment and report.

(In reply to Zack Weinberg (:zwol) from comment #24) > Yes, except this is a chrome mochitest rather than a plain one. I added comments there. > > SimpleTest.finish() <-> window.close() is "wrong" :-/ > > See bug 749872 for a possible way to avoid it. > > Could you be more specific, please? They are two exclusive constraints: *Nothing should execute after ST.finish(), not even w.close(). *Nothing "can" actually execute after w.close(), not even ST.finish(). The current code obviously succeeds, but it's not sane. Yet, as this is not your code, I'm fine if you just file a follow-up bug about that. > It was meant to be easier to review that way. Sorry for the confusion. That's fine as long as they get pushed as a group and with an explicit comment: just unusual (to me). > I may in fact have fixed it right here :) Will experiment and report. That's what I meant.

Comment on attachment 627304 [details] [diff] [review] when doing print preview, don't fail nsDeviceContextSpecWin::Init just because there is no configured printer. (Makes Windows consistent with all other platforms.) This turns out not to work, in a really unhelpful way, too: 11778 INFO TEST-START | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul 11779 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | Access Ci 11780 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | Access Cc 11781 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | Printing error: An unknown error occurred while printing. 11782 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | exception in runTests: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPrint.printPreview]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://mochitests/content/chrome/layout/base/test/chrome/printpreview_helper.xul :: printpreview :: line 60" data: no] 11783 INFO TEST-END | chrome://mochitests/content/chrome/layout/base/test/chrome/test_printpreview.xul | finished in 179ms ... so we go from having .printPreview throw a specific error that I can grep for, to a generic error that could come from anywhere. I'm trying to set up an XP virtual machine so I can debug a little more easily, but that will take some time. If anyone with convenient access to an XP box with no printers whatsoever configured wants to have a look at this, that would be much appreciated. Meanwhile I'm going to address Serge's concerns with patch 2.

Since --post-to-bugzilla doesn't seem to be working, these are the tryserver runs: https://tbpl.mozilla.org/?tree=Try&rev=d26bb452fe68 (patch 2) https://tbpl.mozilla.org/?tree=Try&rev=f8e858f5f3fb (patch 3)

Comment on attachment 627304 [details] [diff] [review] when doing print preview, don't fail nsDeviceContextSpecWin::Init just because there is no configured printer. (Makes Windows consistent with all other platforms.) Review of attachment 627304 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIDeviceContextSpec.h @@ +81,5 @@ > NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GFX,NS_ERROR_GFX_PRINTER_BASE+11) > /* print-to-file: I/O error while printing to file */ > #define NS_ERROR_GFX_PRINTER_FILE_IO_ERROR \ > NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GFX,NS_ERROR_GFX_PRINTER_BASE+12) > +/* code 13 no longer used */ Nit: "Code NS_ERROR_GFX_PRINTER_BASE + 13 no longer used." ::: widget/windows/nsDeviceContextSpecWin.cpp @@ +364,5 @@ > } > return rv; > } > > +NS_IMETHODIMP nsDeviceContextSpecWin::Init(nsIWidget* aWidget, Nit: no need to remove the separator comment line.

Resetting owner to default per Zack's request.