574745 - fatvals: memory leak in browser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

This was detected on the cedar tinderbox, but it can be replicated simply by starting up and shutting down the browser with XPCOM_MEM_BLOAT_LOG set:

== BloatView: ALL (cumulative) LEAK STATISTICS, default process 2828

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          30      832   201570       11 (  890.91 +/-  1482.75)   564473        6 ( 1215.29 +/-  2148.55)
 100 XPCJSStackFrame                                32       32      190        1 (   43.33 +/-    27.48)      375        1 (   44.99 +/-    28.33)
 661 nsXPCException                                 80      800       78       10 (   19.39 +/-    14.91)      205        5 (   18.19 +/-    13.40)

nsTraceRefcntImpl::DumpStatistics: 698 entries

Comment 1

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/users/lwagner_mozilla.com/fatval/rev/a3afccd160a2

This appears to have been latent in m-c and TM. Not sure why it wasn't turning boxes orange there.

Comment 2

[Brendan Eich [:brendan]]

nsCOMPtrs auto-construct with null default value.

Cc'ing XPCOM/C++ mavens.

/be

Comment 3

[Robert Sayre]

(In reply to comment #2)
> nsCOMPtrs auto-construct with null default value.

I looked at this with dmandelin today. Afterward, I pointed out the nsCOMPtr initialization behavior when the patch rolled in. He pointed out that he had copied from a couple lines below, and he fixed it right up:

http://hg.mozilla.org/users/lwagner_mozilla.com/fatval/rev/9838d13eae0c

It looks like a straightforward refcount bug to me. I can't believe it doesn't bite on mozilla-central.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, just looks like a simple reference counting bug.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #3)
> It looks like a straightforward refcount bug to me. I can't believe it doesn't
> bite on mozilla-central.

Maybe because we don't actually have case errors on trunk. Starting up a browser built from fatval throws:

**************************************************
ERROR: JS code at [JS frame :: chrome://browser/content/browser.js :: <TOP_LEVEL> :: line 4462]
tried to access nonexistent property called
'FullScreen' on interface of type 'nsIDOMWindowInternal'.
That interface does however have a property called
'fullScreen'. Did you mean to access that lowercase property?
Please fix the JS code as appropriate.
**************************************************

I see no such error when starting the browser built from mozilla-central. There are a bunch of changes to browser.js, but nothing affecting FullScreen afaics. So maybe there is a change in behaviour in the JS engine that we should worry about? FullScreen is declared as a var here: http://hg.mozilla.org/mozilla-central/annotate/9c85f9aaec8c/browser/base/content/browser.js#l3505