Running the mochitest for bug 396024 leaks the world: nsGlobalWindow, nsDocument etc. http://localhost:8888/tests/layout/base/tests/test_bug396024.html

Seems like we're missing one DocumentViewerImpl::OnDonePrinting() call somewhere.

I'm also leaking with the testcase from bug 365003, I think probably the same bug.

> Seems like we're missing one DocumentViewerImpl::OnDonePrinting() call > somewhere. Isn't OnDonePrinting only called when print preview (or print) fails? The parts of the testcase needed to hit the leak are printpreview + reload. printpreview + exitprintpreview does not leak. AFAICT, we leak because bug 396024 moved mPrintEngine destruction to a place that ::Destroy never gets to in the case of reload. It's in an |if(mSHEntry)| block and mSHEntry is null (note that it appears to be top-level because it's not properly indented).

I tried testing loading another URL (instead of reloading) via location="about:blank" and that doesn't hit the mPrintEngine destruction code either (mSHEntry is still null). So it looks like the code might never execute. It seems like we'd be better off reverting the bug 396024 patch and (if it can't be fixed better) preventing reload while in print preview or replacing the mPrintEngine descruction code with a "We're leaking the world. kthxbye" assertion.

(this bug also makes tracking leaks from full mochitest runs mostly hopeless)

bug 396024 was crashing because DoCommonPrint calls FinishPrintPreview and FinihsPrintPreview nulls out mPrt. Then nsDocumentViewerImpl calls FinishPrintPreview again and DocumentReadyForPrinting accesses mPrt. This patch a) reverts the patch from bug 396024 b) makes FinishPrintPreview return early if it's already finished. With the bug 396024 mochitest, I go from leaking 343663 bytes to leaking 125172 bytes.

Comment on attachment 314247 [details] [diff] [review] patch a1.9=beltzner

Wouldn't block the release at this point, but we're taking the patch. :)

Comment on attachment 314247 [details] [diff] [review] patch landed. Should we resolve this and attack the other leaks in a different bug?

(In reply to comment #9) > (From update of attachment 314247 [details] [diff] [review]) > landed. Should we resolve this and attack the other leaks in a different bug? Resolving FIXED per drivers' request.

Need automated runs with leak testing as well as bug 433132 fixed, but with those we'll be golden.

V.Fixed (fully), per bug 433132 comment 8. ***** "jwalden+bmo: in‑testsuite=?": You mean you want an additional test to test_bug396024.html !?