fatvals: memory leak in browser

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
nsCOMPtrs auto-construct with null default value.

Cc'ing XPCOM/C++ mavens.

/be

Comment 3

8 years ago
(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.
Yeah, just looks like a simple reference counting bug.
(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
You need to log in before you can comment on or make changes to this bug.