839313 - [jsdbg2] Assertion failure: isEmpty(), at mozilla/LinkedList.h:269

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision a46bc920998d (run with --ion-eager -a):


var dbg = new Debugger;
it.customNative = assertEq;

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   114718:bf0617c52ecc
user:        Ms2ger
date:        Sun Dec 02 09:59:39 2012 +0100
summary:     Bug 810679 - Move rt->debuggerList to mozilla::LinkedList; r=Waldo

This iteration took 79.904 seconds to run.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch to use (with valgrind) showing we're leaking the js::Debugger

If I apply this patch, I get this in valgrind spew:

==30857== 6,760 (872 direct, 5,888 indirect) bytes in 1 blocks are definitely lost in loss record 33 of 33
==30857==    at 0x4A0881C: malloc (vg_replace_malloc.c:270)
==30857==    by 0x459A2C: js::MallocProvider<JSContext>::malloc_(unsigned long) (Utility.h:156)
==30857==    by 0x647863: NewDebugger(JSContext*, JSObject*) (jscntxt.h:566)
==30857==    by 0x64E809: js::Debugger::construct(JSContext*, unsigned int, JS::Value*) (Debugger.cpp:2065)
==30857==    by 0x7EF1FE: js::mjit::CallCompiler::generateNativeStub() (jscntxtinlines.h:327)
==30857==    by 0x7EFA12: js::mjit::ic::NativeNew(js::VMFrame&, js::mjit::ic::CallICInfo*) (MonoIC.cpp:1395)
==30857==    by 0x4C14FA9: ???
==30857==    by 0x753C25: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1042)
==30857==    by 0x75436D: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1100)
==30857==    by 0x5373A9: js::RunScript(JSContext*, js::StackFrame*) (jsinterp.cpp:321)
==30857==    by 0x5376F2: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (jsinterp.cpp:514)
==30857==    by 0x537B28: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:554)

Given both the NewDebugger symbol there, and that sizeof(js::Debugger) in gdb is 872, I'm pretty sure we're never removing the Debugger added here, the Debugger destructor's never firing, the element's never being removed, and we get the empty-list assertion failing.

The other result (besides the failed assertion) of not removing the Debugger is that we're leaking it, and everything it keeps alive.  The summary of that run is:

==30857== LEAK SUMMARY:
==30857==    definitely lost: 11,720 bytes in 123 blocks
==30857==    indirectly lost: 8,462 bytes in 14 blocks
==30857==      possibly lost: 0 bytes in 0 blocks
==30857==    still reachable: 0 bytes in 0 blocks
==30857==         suppressed: 0 bytes in 0 blocks

Most of that (6760 bytes) is attributable to the js::Debugger and the indirect bits mentioned in the valgrind stuff.  It wouldn't surprise me if the other bits are also partially attributable to failure to remove the debugger.

Anyway.  The change that regressed this isn't at fault; it's just exposing the pre-existing leak.  Probably the debugger people should be looking at this.

Comment 3

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Anyway.  The change that regressed this isn't at fault; it's just exposing
> the pre-existing leak.  Probably the debugger people should be looking at
> this.

Is it really true that the shell finalizes all objects before it exits? That Debugger instance is still referenced by the global variable 'dbg' at exit. The shell's 'it' object's finalizer doesn't get run on exit, either.

Comment 4

[Jim Blandy :jimb]

This isn't a Debugger issue; it's more of a JSAPI policy issue.

Storing a value on it.customNative actually mallocs a fresh jsval and uses JS_AddValueRoot to root it. If that value is something that entrains the global object, as assertEq evidently does, then the final shutdown GC will not collect the Debugger, or anything else reachable from the global. If you run this program:

it.noisy = true;
it.customNative = assertEq;

you can see that the 'it' finalizer never runs. The extant Debugger instance is just random global-entrained carnage.

So:

Does the JSAPI require all roots to be dropped before one calls JS_DestroyRuntime?

If that requirement is not present, then there's no reason to assume that linked list will be empty when JS_DestroyRuntime is called; ~JSRuntime should probably zap the list before the member destructors run.

If that requirement is present, then the shell is not using the JSAPI correctly: it calls JS_DestroyRuntime with roots outstanding.

Without that requirement, I don't see how one could possibly call JS_DestroyRuntime without leaking essentially any malloc'ed memory held by JS objects.

Comment 5

[Jim Blandy :jimb]

A little more elaboration: if you:

s/assertEq/null/

in that code fragment, you can see that the 'it' finalizer does run. (In both cases, you need to exit the shell after running the fragment.)

Comment 6

[Jim Blandy :jimb]

Attachment: work-in-progress

Comment 7

[Jim Blandy :jimb]

Attachment: Don't use a runtime value root to implement the 'customNative' property of 'it'.

Rather than rooting the value, this just stores it in a reserved slot of the 'it' object, which seems more like the intent anyway.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 714712 [details] [diff] [review]
Don't use a runtime value root to implement the 'customNative' property of 'it'.

Review of attachment 714712 [details] [diff] [review]:
-----------------------------------------------------------------

So basically we've decided that the current code, using the JSAPI as it does, is wrong?  I think I can buy that.  This is just a cycle outside the purview of the GC and of SpiderMonkey more generally; either the embedder has to fix it, or he has to do something else.  Doing this seems like a good way to me.

I'm increasingly unconvinced that |it| in the shell offers much value at all, and I think we should remove it like we removed split objects from the shell.  But that's another concern for another day, probably -- best to just eliminate the leak and move on.

::: js/src/shell/js.cpp
@@ +4160,1 @@
>          vp.set(JSVAL_VOID);

Could you get rid of JSVAL_VOID here and everywhere else and just use UndefinedValue()?  We have JSVAL_VOID right now, for compatibility, but I think we should remove it at some point, because there's no real point to it, and it has bad effects like requiring memory loads, runtime initialization, and so on.

Comment 9

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> So basically we've decided that the current code, using the JSAPI as it
> does, is wrong?  I think I can buy that.  This is just a cycle outside the
> purview of the GC and of SpiderMonkey more generally; either the embedder
> has to fix it, or he has to do something else.  Doing this seems like a good
> way to me.

Yeah. I mean, it *should* be a cycle, because we're imagining that 'customNative' is a property of 'it'. But in fact it's a fresh root, independent of any particular instance of 'it's class; and that discontinuity between concept and implementation is the source of the problem. There shouldn't be a root there; it should be just an edge.

> Could you get rid of JSVAL_VOID here and everywhere else and just use
> UndefinedValue()?

Certainly.

Comment 10

[Jim Blandy :jimb]

Attachment: Replace JSVAL_VOID and similar constants with their JS::MumbleValue() equivalents.

Comment 11

[Jim Blandy :jimb]

Attachment: Don't use a runtime value root to implement the 'customNative' property of 'it'.

Asking for review again because this adds a test, which I'd forgotten to include in the old patch. This applies on top of the JSVAL_MUMBLE replacement patch.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 716750 [details] [diff] [review]
Replace JSVAL_VOID and similar constants with their JS::MumbleValue() equivalents.

Review of attachment 716750 [details] [diff] [review]:
-----------------------------------------------------------------

Technically all the vp[N] are deprecated, and they "should" be using JS_ARGV(cx, vp)[N - 2].  But since we're going to replace argc/vp with CallArgs eventually, doing that would be rearranging deck chairs. So this is good enough.

Assigning to *vp is also deck-chairish, but I only saw two-ish of those (and most of the shell already correctly uses JS_SET_RVAL), so the burden of that change is much smaller.  :-)

::: js/src/shell/js.cpp
@@ +1279,4 @@
>  
>      /* Treat the empty string specially. */
>      if (buflength == 0) {
> +        *vp = feof(from) ? NullValue() : JS_GetEmptyStringValue(cx);

This should have been a JS_SET_RVAL(cx, vp, ...).  Could you change this location and the other success cases to use that instead of assigning to *vp?

@@ +2264,4 @@
>      sprintf(version, " for version %d\n", JS_VERSION);
>  #endif
>      fprintf(gOutFile, "built on %s at %s%s", __DATE__, __TIME__, version);
> +    *vp = UndefinedValue();

JS_SET_RVAL please.

@@ +4205,2 @@
>      } else {
> +        vp.set(UndefinedValue());

This could be vp.setUndefined().  But since shell excluding SpiderMonkey-guts-testing is a public JSAPI user, either's fine.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 716751 [details] [diff] [review]
Don't use a runtime value root to implement the 'customNative' property of 'it'.

Review of attachment 716751 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +4224,3 @@
>      }
>  
> +    *vp = JS_GetReservedSlot(obj, 0);

JS_SET_RVAL(cx, vp, ...) for both of these.

@@ +4237,4 @@
>      if (JS_GetClass(obj) != &its_class)
>          return true;
>  
> +    JS_SetReservedSlot(obj, 0, argc >= 1 ? JS_ARGV(cx, vp)[0] : JSVAL_VOID);

Add a JS_SET_RVAL(cx, vp, UndefinedValue()) after this so that it returns a reasonable value, rather than the its_set_customNative JS function.

Comment 14

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8b199ed597
https://hg.mozilla.org/integration/mozilla-inbound/rev/e150e804f902
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8d0b73600e

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c8b199ed597
https://hg.mozilla.org/mozilla-central/rev/e150e804f902
https://hg.mozilla.org/mozilla-central/rev/2b8d0b73600e

Comment 16

[Jim Blandy :jimb]

Attachment: Add regression test for JS shell 'it.customNative' fix. r=waldo on IRC

I botched the patches I posted here; they were meant to include the regression test from the bug. Rapidly reviewed by Waldo on IRC.

Comment 17

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5602b5e9ba

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d5602b5e9ba