464202 - nsGlobalWindow leak running browser-chrome Mochitests

Status: VERIFIED FIXED

(Toolkit :: Storage, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 2

[Jeff Walden [:Waldo]]

Shawn says this bug's already filed, marking dependency to dup assuming that indeed holds...

Comment 3

[Shawn Wilsher :sdwilsh]

Not the same leak, but I think I have a handle on it!

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

per comments on irc

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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)

Comment 7

[Serge Gautherie (:sgautherie)]

Shawn, will you update the patch and add it to
https://wiki.mozilla.org/1.9.1_beta_2_checkins ?

Comment 8

[Shawn Wilsher :sdwilsh]

I plan to land this after the beta.

Comment 9

[Peter Van der Beken [:peterv]]

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 10

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

Comment on attachment 347655 [details] [diff] [review]
v2.0

a191b2=beltzner; I suspect this should actually be a b2 blocker

Comment 11

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/mozilla-central/rev/964ca72afb55

Comment 12

[Serge Gautherie (:sgautherie)]

(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)
}

Comment 13

[Serge Gautherie (:sgautherie)]

(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)
}

Comment 14

[Jeff Walden [:Waldo]]

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.

Comment 15

[Jeff Walden [:Waldo]]

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

Comment 16

[Serge Gautherie (:sgautherie)]

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

Comment 17

[Serge Gautherie (:sgautherie)]

(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

Comment 18

[Shawn Wilsher :sdwilsh]

(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

Comment 19

[Serge Gautherie (:sgautherie)]

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

Comment 20

[Shawn Wilsher :sdwilsh]

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.

Comment 21

[Serge Gautherie (:sgautherie)]

fixed1.9.1 -> verified1.9.1, per comment 17.