Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: njn, Assigned: graydon)

Tracking

({fixed1.9.1})

Trunk
x86
Linux
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.1 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

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

Updated

9 years ago
Flags: blocking1.9.1?

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 2

9 years ago
Created attachment 384500 [details] [diff] [review]
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.
Attachment #384500 - Flags: review?(jorendorff)
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).
> 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.
(Assignee)

Comment 5

9 years ago
(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?
Attachment #384500 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/tracemonkey/rev/9a00eddad36b
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 7

9 years ago
1.9.1 tree appears closed at the moment, but I'm happy to land this there whenever that changes.
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?
Graydon and I talked on IRC, a191=beltzner, please land using "CLOSED TREE" in your commit comment to get past the sentry.
(Assignee)

Comment 10

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

Comment 11

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b77e5e159c8b
Please remember to get this fixed on trunk, as well.
Flags: blocking1.9.2+
Keywords: fixed1.9.1
(Assignee)

Comment 13

9 years ago
(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.
there was a merge recently; did this flow across?
(Assignee)

Comment 15

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

Comment 16

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9a00eddad36b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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)
Keywords: fixed1.9.2
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
You need to log in before you can comment on or make changes to this bug.