389757 - leak nsSystemPrincipal running bloatcycle.html

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

We leak an nsSystemPrincipal while running through bloatcycle.html, the leak test on tinderbox.  This leak is due to an imbalance (of 2 references) between calls to JS_HoldPrincipals and JS_DropPrincipals.  These two references are associated with two leaked objects for which we call js_NewScript but do not call js_DestroyScript.  However, the function object for the function corresponding to these scripts do not appear to be leaked, since I see the exact same XPC_SHUTDOWN_HEAP_DUMP loading about:blank (which does not show the leak) and running bloatcycle.html (which does).

Steps to reproduce:
 1. create an empty directory (say, "deleteme")
 2. run ./firefox -profile deleteme /path/to/mozilla/build/bloatcycle.html
 3. when the notification comes down about popups, choose "Allow popups from"
 4. go to about:config, and set dom.allow_scripts_to_close_windows to true
 5. quit the browser
 6. export XPCOM_MEM_LEAK_LOG=leak.log
 7. run ./firefox -profile deleteme /path/to/mozilla/build/bloatcycle.html
 8. after the browser closes, look at leak.log

Actual results: an nsSystemPrincipal object has been leaked

The 2 leaked JSScript objects are for:
 _createTabMenuItem in chrome://global/content/bindings/tabbrowser.xml
 _setBlockingState in chrome://global/content/bindings/notification.xml

I checked in the debugger, and the JSFunction, with the leaked script, did make its way out to nsJSContext::CompileFunction.

I'm not sure that this is a JS engine bug, but the fact that the function object doesn't show up in the leak log and yet the script was not destroyed suggests to me that it is.

Comment 1

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

Attachment: JS <-> nsTraceRefcnt debugging patch

Here's the debugging patch I used to determine a bunch of the above information.  I've been keeping my JS <-> nsTraceRefcnt code in a single patch in my mq stack, but that patch got a good bit larger while debugging this, since I needed to add a new nsTraceRefcnt API, plus the logging for the reference counts of JS_HoldPrincipals and JS_DropPrincipals and the logging of JSScript creation and destruction.

Comment 2

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

Attachment: full stacks of leaked JSPrincipals references (from find-comptr-leakers)

Comment 3

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

Attachment: top of stacks where leaked JSScript objects were allocated

Comment 4

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

Attachment: possible patch

This fixes the leak.

I was looking at the code, and it seemed odd to me that the destruction of the script was done in the function object's finalizer.  That seemed to assume that the function would always be collected in the same GC as its function object.  Apparently that wasn't the case here.  My theory as to why is that the function object was in a newborn root (which fits with both of the methods leaked being the last method on the binding).

So since JSFunction was the only user of GCX_PRIVATE, I changed it from GCX_PRIVATE to GCX_FUNCTION (which seems to be an assumption the tracing code already makes), and gave it a finalizer.

Comment 5

[Brendan Eich [:brendan]]

See bug 375808.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 274093 [details] [diff] [review]
possible patch

Nit about bracing style (rarely, with a multiline if condition, the left brace goes on its own line, but the condition becomes one-line with this patch).

/be

Comment 7

[Igor Bukanov]

Comment on attachment 274093 [details] [diff] [review]
possible patch

The patch got it very right!

Comment 8

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

Attachment: patch

This addresses brendan's bracing comment, fixes the comment in jsgc.c about the ordering of finalization, fixes a comment in nsXPConnect.cpp, and also pulls the comment change in js_NewFunction from igor's patch in bug 375808.

I looked at the patch in bug 375808; this patch basically has the changes from that patch relating to separating the finalization but not the ones related to separating the marking or making the JSFunction mark its funobj.

Comment 9

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

Comment on attachment 274114 [details] [diff] [review]
patch

This fixes a leak that can happen occasionally.  It's not incredibly high frequency, but the patch is pretty simple and should be low risk.

Comment 10

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

Fix checked in to trunk.