Closed
Bug 429233
Opened 16 years ago
Closed 16 years ago
Firefox leaks nsTArray_base
Categories
(Core :: General, defect, P1)
Core
General
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: cbook, Assigned: bent.mozilla)
References
Details
(Keywords: memory-leak, regression)
Attachments
(2 files)
3.49 KB,
text/plain
|
Details | |
3.12 KB,
patch
|
jst
:
review+
jst
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041522 Firefox/3.0pre ID:2008041522 Firefox leaks currently per default nsTArray, this can be reproduced with extensions, but also too with a new profile without extensions. As example i was able to reproduce while browsing to http://www.spiegel.de This seems to be a leak regression that is not catched by tinderbox 0 TOTAL 26 20 1267112 5 ( 4032.00 +/- 3970.55) 4281478 0 ( 4941.04 +/- 8051.92) 666 nsTArray_base 4 20 77245 5 ( 8010.27 +/- 2270.32) 0 0 ( 0.00 +/- 0.00)
Flags: blocking1.9?
Would be good to know what type of array we're leaking here, if it's an array of big objects we might want to mark this a blocker. And it does seem like this leak has come up enough lately that it could be making other leak fixing harder.
Comment 2•16 years ago
|
||
python runtests.py \ --setenv=XPCOM_MEM_LOG_CLASSES=nsTArray_base \ --setenv=XPCOM_MEM_ALLOC_LOG=log.txt \ --test-path=q and immediately closing claims leaks 4 bytes right off the bat on OS X and Windows. Strangely, the leak output is whacked -- I get: Serial Numbers of Leaked Objects: 5 @0x2317c58 (0 references; 0 from COMPtrs) 2053 @0x2843200 (0 references; 0 from COMPtrs) 11 @0x2317c94 (0 references; 0 from COMPtrs) 16 @0x2317c04 (0 references; 0 from COMPtrs) ...a bunch more... 470 @0x2843218 (0 references; 0 from COMPtrs) 17 @0x2317cac (0 references; 0 from COMPtrs) 13 @0x2317c1c (0 references; 0 from COMPtrs) WARNING leaked 4 bytes during test execution WARNING leaked 1 instance of nsTArray_base with size 4 bytes It's about two screenfuls or so of "leaked" objects, all with "0 references"; nsTraceRefcntImpl.cpp seems to say MOZ_COUNT_[CD]TOR will cause these 0 reference counts to be reported. Aside from this being a regression (we didn't have this problem a week ago, I think -- don't remember when I last fired up the browser before today), the larger number of objects and serial numbers printed worries me, especially when they don't show up as a corresponding leak count. dbaron, do you know how the number of serial numbers and objects reported as leaked wouldn't equal the number of instances the bloat log would report? I looked at the stacks, and indeed, the addresses I looked at are Ctor'd but not Dtor'd in the log -- I see no reason not to believe they all leak and the leak report is somehow lying. All (four-ish of) the stacks I looked at from the leaked addresses above all had about the following at the bottom of the stack: nsTArray_base::nsTArray_base() (/Users/jwalden/moz/builds/trunk/xpcom/build/nsTArray.cpp:48) nsQueryReferent::nsQueryReferent(nsIWeakReference*, unsigned int*)+0x00000E99 (/Users/jwalden/moz/builds/trunk/xpcom/MoreFiles/../../dist/include/xpcom/nsTArray.h:273) nsQueryReferent::nsQueryReferent(nsIWeakReference*, unsigned int*)+0x000011D4 (/Users/jwalden/moz/trunk/mozilla/xpcom/ds/nsObserverList.h:77) nsQueryReferent::nsQueryReferent(nsIWeakReference*, unsigned int*)+0x0000122C (/Users/jwalden/moz/builds/trunk/xpcom/MoreFiles/../../dist/include/xpcom/nsTHashtable.h:4 00) PL_DHashTableOperate (pldhash.c:643) nsQueryReferent::nsQueryReferent(nsIWeakReference*, unsigned int*)+0x000010C9 (/Users/jwalden/moz/builds/trunk/xpcom/MoreFiles/../../dist/include/xpcom/nsTHashtable.h:1 82) nsObserverService::AddObserver(nsIObserver*, char const*, int) (/Users/jwalden/moz/trunk/mozilla/xpcom/ds/nsObserverService.cpp:133) nsPrefService::Init() (/Users/jwalden/moz/trunk/mozilla/modules/libpref/src/nsPrefService.cpp:161) Subsequent lines differed, but the allocation in all the cases I looked at were nested underneath nsObserverService::AddObserver. So it looks like something is rotten around nsObserverService::mObserverTopicTable, but I can't see a checkin in the last week that makes any sense here.
Comment 3•16 years ago
|
||
Jonas, need your help with a blocking decision here, please.
I'm on the fence on this one. I guess we can do it in a dot-release since the leak is small. But generally we have had 0 leaks in the common case, and this seems to change that. Would be more than great to have it fixed, but i guess we wouldn't hold the release for it.
Assignee: nobody → bent.mozilla
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
Priority: -- → P1
Comment 5•16 years ago
|
||
the mochitest leak in comment 2 started happening in the checkin window: http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=1208229840&maxdate=1208240519
Reporter | ||
Comment 6•16 years ago
|
||
Changing OS -> All, was able to reproduce this also on Windows
OS: Mac OS X → All
Hardware: PC → All
Assignee | ||
Comment 7•16 years ago
|
||
Here's where the leak is coming from, thanks to dbaron for helping me tease the data out of the logs. Definitely caused by bug 359638. We're only leaking an array of nsAlternativeCharCode so not a big deal I don't think. In my testing the array has been empty so far, I think.
Assignee | ||
Comment 8•16 years ago
|
||
Fixed by properly deleting the nsKeyEvent pointer... We need virtual destructors here! See bug 429422.
Attachment #316137 -
Flags: review?(jst)
Comment 9•16 years ago
|
||
Comment on attachment 316137 [details] [diff] [review] Patch Yeah, we need this. r+sr=jst
Attachment #316137 -
Flags: superreview+
Attachment #316137 -
Flags: review?(jst)
Attachment #316137 -
Flags: review+
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 316137 [details] [diff] [review] Patch Drivers, simple fix to keep us from leaking an array (and anything inside it).
Attachment #316137 -
Flags: approval1.9?
Comment 11•16 years ago
|
||
Comment on attachment 316137 [details] [diff] [review] Patch a1.9=beltzner
Attachment #316137 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 12•16 years ago
|
||
I won't be able to check this in until Tuesday, so if someone else could do it before then that would be awesome.
Keywords: checkin-needed
Comment 13•16 years ago
|
||
Done. We'll cover this in automated tests when we get around to enabling leak checking on tinderboxen.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Reporter | ||
Comment 14•16 years ago
|
||
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041822 Firefox/3.0pre, thanks guys :)
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Flags: wanted1.9.0.x+
You need to log in
before you can comment on or make changes to this bug.
Description
•