Closed Bug 464202 Opened 11 years ago Closed 11 years ago

nsGlobalWindow leak running browser-chrome Mochitests

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: Waldo, Assigned: sdwilsh)

References

Details

(Keywords: memory-leak, regression, verified1.9.1)

Attachments

(2 files, 1 obsolete file)

There's a whole ton of stuff leaked (292727 bytes in the run I just did, zounds), who knows what, but most notably, we're leaking two nsGlobalWindows that'll entrain the world:

find-waldo-now:~/moz/2 jwalden$ cat ../bc-windows.log | perl tools/footprint/leak-gauge.pl
Leaked outer window 3c174ab0 at address 3c174ab0.
Leaked inner window 3c18f4f0 (outer 3c174ab0) at address 3c18f4f0.
 ... with URI "about:blank".
Summary:
Leaked 2 out of 332 DOM Windows
Leaked 0 out of 307 documents
Leaked 0 out of 100 docshells
Leaked content nodes within 0 out of 333 documents

No idea whose fault this is yet (hence the unusual component choice) as the URL isn't especially helpful here, but this must stop.  Browser-chrome tests have been allowed to leak for too long (of the Mochitest-based test suites, only browser-chrome is allowed to leak without turning tinderbox orange), and they need a smackdown.
Attached file Reduced testcase
browser/base/content/test/browser_sanitize-timespans.js is the test that's leaking, but the problem isn't specific to the test.  Here's a nice little reduced testcase (need to tweak the path at top of the file) that demonstrates where the problem is when run in xpcshell; tweak the var at the top of the file as appropriate.  Commenting out the |stmt.params;| line results in no leaks, while as-is leaks a bunch of things.
Shawn says this bug's already filed, marking dependency to dup assuming that indeed holds...
Component: General → SQL
Depends on: 457743
QA Contact: general → irixman+bugzilla
Not the same leak, but I think I have a handle on it!
Assignee: jwalden+bmo → nobody
Component: SQL → Storage
Flags: blocking1.9.1?
Product: Core → Toolkit
QA Contact: irixman+bugzilla → storage
Attached patch v1.0 (obsolete) — Splinter Review
Assignee: nobody → sdwilsh
Attachment #347641 - Flags: review?(mrbkap)
Attached patch v2.0Splinter Review
per comments on irc
Attachment #347641 - Attachment is obsolete: true
Attachment #347655 - Flags: review?(mrbkap)
Attachment #347641 - Flags: review?(mrbkap)
Comment on attachment 347655 [details] [diff] [review]
v2.0

diff --git a/storage/src/mozStorageService.cpp b/storage/src/mozStorageService.cpp
+    if (sXPConnect) {
+        NS_RELEASE(sXPConnect);
+        sXPConnect = nsnull;
+    }

NS_IF_RELEASE(sXPConnect)
Attachment #347655 - Flags: review?(mrbkap) → review+
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [has patch][has review][can land]
Blocks: 431558
Shawn, will you update the patch and add it to
https://wiki.mozilla.org/1.9.1_beta_2_checkins ?
I plan to land this after the beta.
Whiteboard: [has patch][has review][can land] → [has patch][has review][needs approval]
Attachment #347655 - Flags: approval1.9.1b2?
Comment on attachment 347655 [details] [diff] [review]
v2.0

This should go into b2 IMO. We have had zero tolerance for leaks for a while now, this one just slipped by because we can't yet set the leak treshold to 0 for browser-chrome tests. If we could, the patch that caused this (obvious) leak would have been backed out due to orange tinderboxes.
Comment on attachment 347655 [details] [diff] [review]
v2.0

a191b2=beltzner; I suspect this should actually be a b2 blocker
Attachment #347655 - Flags: approval1.9.1b2? → approval1.9.1b2+
Whiteboard: [has patch][has review][needs approval] → [needs landing]
Target Milestone: --- → mozilla1.9.1b2
http://hg.mozilla.org/mozilla-central/rev/964ca72afb55
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [needs landing]
(In reply to comment #6)
> NS_IF_RELEASE(sXPConnect)

This nit was not checked in...

*****

Could it be that this checkin did not fix the issue ?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081119 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/0cd41f599080)

I see a major regression (which I don't remember seeing when I reported bug 462545), which contains
{
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 2 instances of nsGlobalWindow with size 416 bytes each (832 bytes total)
}
(In reply to comment #12)
> Could it be that this checkin did not fix the issue ?

It really looks like so:
{
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 289692 bytes during test execution (threshold set at 64 bytes)
}
This bug isn't fixed.

When I originally filed this bug, Shawn suggested it was a dup of bug 457743; that looked right, so I added the dependency until we could be sure.  His first patch for that bug addressed statement/param leaks but not nsGlobalWindow leaks, so we knew there had to be more.  Investigation pointed out the XPConnect leak, and when I tested the first patch here atop the previous patch, it worked.

However, I didn't test the patch here on a bare tree -- and with just this patch, we still leak nsGlobalWindows in the exact same situation.  Presumably statements hold references to mozStorageService, and since this causes the service's refcount to never reach zero and its dtor to never be called, XPConnect is indirectly leaked again.  A current tree with the patch in bug 457743 applied doesn't leak nsGlobalWindows any more (but other leaks remain).

Bottom line: this bug isn't fixed until bug 457743 is fixed.  If we're serious about this blocking 1.9.1b2, that bug must too, and we have to get its patch in the tree before 1.9.1b2.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: regression
OS: Mac OS X → All
Whiteboard: [Waiting for bug 457743]
I landed the patch for bug 457743; no more nsGlobalWindow leaks!  Whee!

WARNING leaked 293147 bytes during test execution ---> WARNING leaked 73506 bytes during test execution
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [Waiting for bug 457743]
(In reply to comment #12)
> (In reply to comment #6)
> > NS_IF_RELEASE(sXPConnect)
> 
> This nit was not checked in...

http://hg.mozilla.org/mozilla-central/rev/59d84ac24da2
is worse:
actually, the whole |if| block should be replaced by the (new) macro.
(In reply to comment #15)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e8bb73413554)

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 289592 bytes during test execution (threshold set at 64 bytes)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/20245c2d97d0)

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 71870 bytes during test execution (threshold set at 64 bytes)

V.Fixed
Status: RESOLVED → VERIFIED
Hardware: PC → All
(In reply to comment #16)
> http://hg.mozilla.org/mozilla-central/rev/59d84ac24da2
> is worse:
> actually, the whole |if| block should be replaced by the (new) macro.
Fixed in http://hg.mozilla.org/mozilla-central/rev/b923f6b41e01
(In reply to comment #18)
> > actually, the whole |if| block should be replaced by the (new) macro.
> Fixed in http://hg.mozilla.org/mozilla-central/rev/b923f6b41e01

Better, but I still think |sXPConnect = nsnull;| should be removed too:
see
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsUtils.h#169
:->
Neat.  I didn't know that NS_IF_RELEASE did that.  I'm going to leave it - next time I touch the file I'll fix it.
fixed1.9.1 -> verified1.9.1, per comment 17.
You need to log in before you can comment on or make changes to this bug.