499603 - TM: leak in getAnchor()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Running Valgrind like this on 'js' on my Linux box:

  valgrind --smc-check=all --leak-check=full js-debug/js -j -e 'gSkipSlowTests=true' trace-test.js 

I get this:

==29456== 164 bytes in 1 blocks are definitely lost in loss record 1 of 1
==29456==    at 0x4AF0F3E: calloc (vg_replace_malloc.c:414)
==29456==    by 0x813B1DA: avmplus::GCObject::operator new(unsigned int, avmplus::GC*) (avmplus.h:166)
==29456==    by 0x8198F89: getAnchor(JSTraceMonitor*, void const*, JSObject*, unsigned int, unsigned int) (jstracer.cpp:644)
==29456==    by 0x81A45D2: js_MonitorLoopEdge(JSContext*, unsigned int&) (jstracer.cpp:4824)
==29456==    by 0x80BBC02: js_Interpret (jsinterp.cpp:3875)
==29456==    by 0x80DE9CD: js_Execute (jsinterp.cpp:1622)
==29456==    by 0x8055931: JS_ExecuteScript (jsapi.cpp:5036)
==29456==    by 0x80517DB: Process(JSContext*, JSObject*, char*, int) (js.cpp:407)
==29456==    by 0x80524FD: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:767)
==29456==    by 0x80528C4: main (js.cpp:4696)

It occurs both on tracemonkey/ and mozilla-1.9.1/, and so might be worth consideration as a 1.9.1 blocker.  I haven't checked if it occurs with a full Firefox build.  Graydon volunteered to look at it.

Comment 1

[Graydon Hoare :graydon]

Yeah, this is actually bad. Unfortunately it seems bug 496391 was mis-fixed and it winds up leaking every VMFragment by prematurely disconnecting them on the first (trashing, not deleting) pass over the VMFragments hashtable. And I reviewed that! Ach.

Anyway, patch forthcoming in a few minutes. Meanwhile I'll nominate this to block, unfortunately. It's a guaranteed and reasonably rapid leak. It should be plugged.

Comment 2

[Graydon Hoare :graydon]

Attachment: Fix the bug

I'm not sure using 'bool' to indicate 'yeah, go ahead and disconnect the fragment' is the most obvious interface, but it's easy and works. Valgrind reports no leaks with this patch. Running through try servers now.

Comment 3

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

Are we sure this blocks? FWIW, this leak has not popped up in the membuster testing we've been running (loads webpages in 30 tabs, cycles through them, closes all tabs).

Comment 4

[David Mandelin [:dmandelin]]

> I'm not sure using 'bool' to indicate 'yeah, go ahead and disconnect the
> fragment' is the most obvious interface, but it's easy and works. Valgrind
> reports no leaks with this patch. Running through try servers now.

A possibly more explicit alternative would be to add a boolean template parameter.

Comment 5

[Graydon Hoare :graydon]

(In reply to comment #3)
> Are we sure this blocks? FWIW, this leak has not popped up in the membuster
> testing we've been running (loads webpages in 30 tabs, cycles through them,
> closes all tabs).

Ok, 'rapid' in a manner of speaking: it leaks a single 164-byte structure for every loop that was traced and executed, when the originating script unloads. So this could be as few as 0 per page or as many as hundreds, but it's unlikely to be noticed at broad granularity. A few kb per page?

Comment 6

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/9a00eddad36b

Comment 7

[Graydon Hoare :graydon]

1.9.1 tree appears closed at the moment, but I'm happy to land this there whenever that changes.

Comment 8

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

What is the regression-risk of this patch? Is the code path localized (as in, all that could happen is it doesn't actually stop the leak, or expose another) or would we need to re-run our entire beta/QA period again to make sure we didn't alter other things?

Comment 9

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

Graydon and I talked on IRC, a191=beltzner, please land using "CLOSED TREE" in your commit comment to get past the sentry.

Comment 10

[Graydon Hoare :graydon]

The code path is pretty localized. The leaked objects are headers for subgraphs of the JIT cache that (a) are already nulled-out and cannot be used if found, and (b) are already disconnected from the object graph anyways, so there should be no way of even noticing the leak short of an allocation-tracker like valgrind. There should be no other observable affect.

The only caveat I'd provide is the word "should". Because, you know, it's software. And because we were sure we got this path right .. last time :(

Comment 11

[Graydon Hoare :graydon]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b77e5e159c8b

Comment 12

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

Please remember to get this fixed on trunk, as well.

Comment 13

[Graydon Hoare :graydon]

(In reply to comment #12)
> Please remember to get this fixed on trunk, as well.

Landed in tracemonkey first; it'll flow to trunk on the next merge.

Comment 14

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

there was a merge recently; did this flow across?

Comment 15

[Graydon Hoare :graydon]

I'm not seeing it in the m-c logs yet, no.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9a00eddad36b

Comment 17

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

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)