Closed
Bug 464202
Opened 16 years ago
Closed 16 years ago
nsGlobalWindow leak running browser-chrome Mochitests
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
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)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mrbkap
:
review+
beltzner
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
Shawn says this bug's already filed, marking dependency to dup assuming that indeed holds...
Assignee | ||
Comment 3•16 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 | ||
Updated•16 years ago
|
Attachment #347641 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•16 years ago
|
||
per comments on irc
Attachment #347641 -
Attachment is obsolete: true
Attachment #347655 -
Flags: review?(mrbkap)
Attachment #347641 -
Flags: review?(mrbkap)
Comment 6•16 years ago
|
||
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+
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][has review][can land]
Comment 7•16 years ago
|
||
Shawn, will you update the patch and add it to https://wiki.mozilla.org/1.9.1_beta_2_checkins ?
Assignee | ||
Comment 8•16 years ago
|
||
I plan to land this after the beta.
Whiteboard: [has patch][has review][can land] → [has patch][has review][needs approval]
Updated•16 years ago
|
Attachment #347655 -
Flags: approval1.9.1b2?
Comment 9•16 years ago
|
||
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•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [has patch][has review][needs approval] → [needs landing]
Target Milestone: --- → mozilla1.9.1b2
Comment 11•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/964ca72afb55
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [needs landing]
Comment 12•16 years ago
|
||
(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•16 years ago
|
||
(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) }
Reporter | ||
Comment 14•16 years ago
|
||
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 → ---
Updated•16 years ago
|
Keywords: regression
OS: Mac OS X → All
Updated•16 years ago
|
Whiteboard: [Waiting for bug 457743]
Reporter | ||
Comment 15•16 years ago
|
||
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: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [Waiting for bug 457743]
Comment 16•16 years ago
|
||
(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•16 years ago
|
||
(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•16 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
Comment 19•16 years ago
|
||
(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•16 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.
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 21•16 years ago
|
||
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.
Description
•