Closed
Bug 453146
Opened 16 years ago
Closed 16 years ago
<entry_xhtml_baseURI_with_amp.xml> test leaks 450 'nsStringBuffer'
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b1
People
(Reporter: sgautherie, Assigned: peterv)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
I removed all but this test from <...\toolkit\components\feeds\test\xml\>
|clear && env XPCOM_MEM_LEAK_LOG=1 make -C .../toolkit/components/feeds check|
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080825125202 Minefield/3.1a2pre] (home, optim default) (W2Ksp4)
[
154 nsStringBuffer 8 3624 16966 453 ( 4827,21 +/- 3266,99) 27484 453 ( 6484,84 +/- 4463,57)
]
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
[
154 nsStringBuffer 8 3600 17462 450 ( 4476,82 +/- 3310,71) 28649 450 ( 6025,43 +/- 4544,52)
]
Reporter | ||
Comment 1•16 years ago
|
||
Ah, I had already noticed this in bug 448980 comment 0.
NB: I don't know what this file has different than all the other feed test files to trigger this.
Depends on: 448980
Reporter | ||
Comment 2•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901143156 SeaMonkey/2.0a1pre] (home, debug default) (W2Ksp4)
Fwiw,
[
nsStringStats
=> mFreeCount: 18345 -- LEAKED 450 !!!
]
Comment 3•16 years ago
|
||
Comment 4•16 years ago
|
||
Something going wrong with the nsScriptNameSpaceManager, maybe?
Comment 5•16 years ago
|
||
With this debugging patch, you can see that the nsScriptNameSpaceManager is being leaked. If you set XPCOM_MEM_ALLOC_LOG=alloc.log and XPCOM_MEM_LOG_CLASSES=nsScriptNameSpaceManager you can even see the stack to its allocation ;)
I don't think I'll be able to get much further in debugging this bug.
Attachment #337173 -
Flags: review?(bzbarsky)
Updated•16 years ago
|
Attachment #337173 -
Flags: review?(bzbarsky) → review+
Comment 6•16 years ago
|
||
Comment on attachment 337173 [details] [diff] [review]
patch to instrument nsScriptNameSpaceManager
[Checkin: Comment 13]
Sure. The real issue here is that we never call nsJSRuntime::Shutdown in this case....
Comment 7•16 years ago
|
||
What would normally call nsJSRuntime::Shutdown? I don't see any obvious callers in the tree.
Comment 8•16 years ago
|
||
I guess nsDOMScriptObjectFactory::Observe is supposed to call it. I wonder why it's not. Is there a way to get this shutdown in a debugger?
Updated•16 years ago
|
Attachment #337173 -
Flags: superreview+
Assignee | ||
Comment 9•16 years ago
|
||
This looks like a regression from DOM_AGNOSTIC (bug 255942). We never create the nsDOMScriptObjectFactory, so we never register the observer for XPCOM shutdown. nsJSRuntime::GetNameSpaceManager does get called though, and creates and caches the script namespace manager. I'd rather remove the Shutdown method on nsIScriptRuntime completely, after this patch both JS and Python would have an empty Shutdown method. Other language runtimes (hah!) can just use an XPCOM shutdown observer to release stuff.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attachment #337903 -
Flags: superreview?
Attachment #337903 -
Flags: review?
Comment 10•16 years ago
|
||
Peter, did you mean to ask jst for review?
Assignee | ||
Comment 11•16 years ago
|
||
Grr, yes. And it looks like I also forgot to qrefresh before attaching. Trying again.
Attachment #337903 -
Attachment is obsolete: true
Attachment #337929 -
Flags: superreview?(jst)
Attachment #337929 -
Flags: review?(jst)
Attachment #337903 -
Flags: superreview?
Attachment #337903 -
Flags: review?
Assignee | ||
Comment 12•16 years ago
|
||
This doesn't leak for me, but it might be safer to still release mLanguageArray in nsDOMScriptObjectFactory::Observer.
Updated•16 years ago
|
Attachment #337173 -
Attachment description: patch to instrument nsScriptNameSpaceManager → patch to instrument nsScriptNameSpaceManager (checked in)
Attachment #337173 -
Attachment is obsolete: true
Reporter | ||
Comment 13•16 years ago
|
||
Comment on attachment 337173 [details] [diff] [review]
patch to instrument nsScriptNameSpaceManager
[Checkin: Comment 13]
http://hg.mozilla.org/mozilla-central/rev/4a57478cce2a
Attachment #337173 -
Attachment description: patch to instrument nsScriptNameSpaceManager (checked in) → patch to instrument nsScriptNameSpaceManager
[Checkin: Comment 13]
Attachment #337173 -
Attachment is obsolete: false
Reporter | ||
Updated•16 years ago
|
Blocks: dom-agnostic
Updated•16 years ago
|
Attachment #337929 -
Flags: superreview?(jst)
Attachment #337929 -
Flags: superreview+
Attachment #337929 -
Flags: review?(jst)
Attachment #337929 -
Flags: review+
Reporter | ||
Comment 14•16 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080911004812 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)
New leak report :-)
[
1 nsScriptNameSpaceManager
450 nsStringBuffer
]
Reporter | ||
Comment 15•16 years ago
|
||
(In reply to comment #14)
Moreover, 40 of the 69 |make check| tests which leak leak (1) nsScriptNameSpaceManager !
We have a winner here ;->
Component: RSS Discovery and Preview → Layout
Product: Firefox → Core
QA Contact: rss.preview → layout
Reporter | ||
Comment 16•16 years ago
|
||
Comment on attachment 337929 [details] [diff] [review]
v1
[Checkin: Comment 16]
http://hg.mozilla.org/mozilla-central/rev/a530d5f2f15a
with additional
[
/layout/build/nsLayoutStatics.cpp
7 -#include "nsDOMScriptObjectFactory.h"
]
Attachment #337929 -
Attachment description: v1 → v1
[Checkin: Comment 16]
Reporter | ||
Updated•16 years ago
|
Severity: normal → major
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: regression
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Reporter | ||
Comment 17•16 years ago
|
||
(In reply to comment #15)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080911163138 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)
Compiled m-c up-to-before comment 16 checkin:
*36 tests report exactly this leak.
*test_bug299716.js:
+ 1 nsStringBuffer
*test_placesTxn.js:
+ yet a few more objects
*test_0030_general.js and test_0110_general.js
+ yet 1 XPCWrappedNative and 1 XPCWrappedNativeProto
Reporter | ||
Comment 18•16 years ago
|
||
(In reply to comment #17)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080911182230 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)
Compiled m-c up-to-including comment 16 checkin:
*The 36 tests are fixed.
*The other 4 have their other leaks remaining only.
V.Fixed
Jesse, Boris, Peter: thanks :-))
No longer blocks: mlkTests
Status: RESOLVED → VERIFIED
Reporter | ||
Updated•16 years ago
|
Reporter | ||
Comment 19•16 years ago
|
||
(In reply to comment #17)
> *test_bug299716.js:
> + 1 nsStringBuffer
I filed
Bug 454842 - test_bug299716.js leaks 1 nsStringBuffer, due to (2) |env.set("XPCOM_DEBUG_BREAK", "...");|
> *test_placesTxn.js:
> + yet a few more objects
> *test_0030_general.js and test_0110_general.js
> + yet 1 XPCWrappedNative and 1 XPCWrappedNativeProto
I updated
Bug 449240 - |make check|: 30 tests report the same 6 leaked objects (plus others)
Reporter | ||
Updated•16 years ago
|
Reporter | ||
Updated•15 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•