Closed Bug 429233 Opened 12 years ago Closed 12 years ago

Firefox leaks nsTArray_base

Categories

(Core :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: cbook, Assigned: bent.mozilla)

References

Details

(Keywords: memory-leak, regression)

Attachments

(2 files)

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.
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.
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
the mochitest leak in comment 2 started happening in the checkin window:
http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=1208229840&maxdate=1208240519
Changing OS -> All, was able to reproduce this also on Windows
OS: Mac OS X → All
Hardware: PC → All
Attached file Leaking stack
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.
Attached patch PatchSplinter Review
Fixed by properly deleting the nsKeyEvent pointer... We need virtual destructors here! See bug 429422.
Attachment #316137 - Flags: review?(jst)
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+
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 on attachment 316137 [details] [diff] [review]
Patch

a1.9=beltzner
Attachment #316137 - Flags: approval1.9? → approval1.9+
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
Done.  We'll cover this in automated tests when we get around to enabling leak checking on tinderboxen.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
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
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.