nsGlobalWindow leak running browser-chrome Mochitests

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Toolkit
Storage
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Waldo, Assigned: sdwilsh)

Tracking

({mlk, regression, verified1.9.1})

Trunk
mozilla1.9.1b2
mlk, regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Created attachment 347542 [details]
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
(Assignee)

Comment 3

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

Comment 4

9 years ago
Created attachment 347641 [details] [diff] [review]
v1.0
Assignee: nobody → sdwilsh
(Assignee)

Updated

9 years ago
Attachment #347641 - Flags: review?(mrbkap)
(Assignee)

Comment 5

9 years ago
Created attachment 347655 [details] [diff] [review]
v2.0

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+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][has review][can land]

Updated

9 years ago
Blocks: 431558
Shawn, will you update the patch and add it to
https://wiki.mozilla.org/1.9.1_beta_2_checkins ?
(Assignee)

Comment 8

9 years ago
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.
Blocks: 452897
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
Last Resolved: 9 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
Last Resolved: 9 years ago9 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
(Assignee)

Comment 18

9 years ago
(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
:->
(Assignee)

Comment 20

9 years ago
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.
Keywords: fixed1.9.1
fixed1.9.1 -> verified1.9.1, per comment 17.
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.