429233 - Firefox leaks nsTArray_base

Status: VERIFIED FIXED

(Core :: General, defect, P1)

Description

[Carsten Book [:Tomcat]]

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)

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Jeff Walden [:Waldo]]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

Jonas, need your help with a blocking decision here, please.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Andrew Schultz]

the mochitest leak in comment 2 started happening in the checkin window:
http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=1208229840&maxdate=1208240519

Comment 6

[Carsten Book [:Tomcat]]

Changing OS -> All, was able to reproduce this also on Windows

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

Fixed by properly deleting the nsKeyEvent pointer... We need virtual destructors here! See bug 429422.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 316137 [details] [diff] [review]
Patch

Yeah, we need this. r+sr=jst

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 316137 [details] [diff] [review]
Patch

Drivers, simple fix to keep us from leaking an array (and anything inside it).

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316137 [details] [diff] [review]
Patch

a1.9=beltzner

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I won't be able to check this in until Tuesday, so if someone else could do it before then that would be awesome.

Comment 13

[Jeff Walden [:Waldo]]

Done.  We'll cover this in automated tests when we get around to enabling leak checking on tinderboxen.

Comment 14

[Carsten Book [:Tomcat]]

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 :)