Closed Bug 453146 Opened 11 years ago Closed 11 years ago

<entry_xhtml_baseURI_with_amp.xml> test leaks 450 'nsStringBuffer'

Categories

(Core :: Layout, defect, major)

defect
Not set
major

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)

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)
]
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
[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 !!!
]
Blocks: mlkTests
Something going wrong with the nsScriptNameSpaceManager, maybe?
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)
Attachment #337173 - Flags: review?(bzbarsky) → review+
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....
What would normally call nsJSRuntime::Shutdown?  I don't see any obvious callers in the tree.
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?
Attached patch v1 (obsolete) — Splinter Review
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?
Peter, did you mean to ask jst for review?
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?
This doesn't leak for me, but it might be safer to still release mLanguageArray in nsDOMScriptObjectFactory::Observer.
Attachment #337173 - Attachment description: patch to instrument nsScriptNameSpaceManager → patch to instrument nsScriptNameSpaceManager (checked in)
Attachment #337173 - Attachment is obsolete: true
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
Blocks: dom-agnostic
Attachment #337929 - Flags: superreview?(jst)
Attachment #337929 - Flags: superreview+
Attachment #337929 - Flags: review?(jst)
Attachment #337929 - Flags: review+
[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
]
(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
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]
Severity: normal → major
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: regression
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
(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
(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
Blocks: 448980
No longer depends on: 448980
Blocks: 449240
(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)
Depends on: 464389
Blocks: mlkTests
Depends on: 456414
Depends on: 469523
No longer depends on: 456414
Depends on: 512154
No longer depends on: 469523
You need to log in before you can comment on or make changes to this bug.