Running mochitest for bug 396024 leaks

VERIFIED FIXED

Status

()

Core
Print Preview
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: smaug, Assigned: Andrew Schultz)

Tracking

({memory-leak, testcase})

Trunk
memory-leak, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → Olli.Pettay
(Reporter)

Updated

11 years ago
Assignee: Olli.Pettay → nobody

Updated

11 years ago
Keywords: testcase
Blocks: 396024
I'm also leaking with the testcase from bug 365003, I think probably the same bug.
(Assignee)

Comment 3

10 years ago
> 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).
(Assignee)

Comment 4

10 years ago
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.
Flags: blocking1.9?
(Assignee)

Comment 5

10 years ago
(this bug also makes tracking leaks from full mochitest runs mostly hopeless)
(Assignee)

Comment 6

10 years ago
Created attachment 314247 [details] [diff] [review]
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.
Attachment #314247 - Flags: superreview?(roc)
Attachment #314247 - Flags: review?(roc)
Attachment #314247 - Flags: superreview?(roc)
Attachment #314247 - Flags: superreview+
Attachment #314247 - Flags: review?(roc)
Attachment #314247 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #314247 - Flags: approval1.9?
Comment on attachment 314247 [details] [diff] [review]
patch

a1.9=beltzner
Attachment #314247 - Flags: approval1.9? → approval1.9+
Wouldn't block the release at this point, but we're taking the patch.  :)
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
(Assignee)

Comment 9

10 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Depends on: 433132

Comment 11

10 years ago
Need automated runs with leak testing as well as bug 433132 fixed, but with those we'll be golden.
Flags: in-testsuite?
V.Fixed (fully), per bug 433132 comment 8.

*****

"jwalden+bmo: in‑testsuite=?":
You mean you want an additional test to test_bug396024.html !?
Assignee: nobody → ajschult
Status: RESOLVED → VERIFIED
Component: General → Print Preview
OS: Linux → All
QA Contact: general → printing
Hardware: PC → All
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.