408355 - Running mochitest for bug 396024 leaks

Status: VERIFIED FIXED

(Core :: Print Preview, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Martijn Wargers (dead)]

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

Comment 3

[Andrew Schultz]

> 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).

Comment 4

[Andrew Schultz]

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.

Comment 5

[Andrew Schultz]

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

Comment 6

[Andrew Schultz]

Attachment: patch

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 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314247 [details] [diff] [review]
patch

a1.9=beltzner

Comment 8

[Damon Sicore (:damons)]

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

Comment 9

[Andrew Schultz]

Comment on attachment 314247 [details] [diff] [review]
patch

landed.  Should we resolve this and attack the other leaks in a different bug?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 11

[Jeff Walden [:Waldo]]

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

Comment 12

[Serge Gautherie (:sgautherie)]

V.Fixed (fully), per bug 433132 comment 8.

*****

"jwalden+bmo: in‑testsuite=?":
You mean you want an additional test to test_bug396024.html !?