Closed Bug 839313 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: decoder, Assigned: jimb)

References

Details

(Keywords: assertion, memory-leak, testcase, Whiteboard: [jsbugmon:update])

Attachments

(4 files, 2 obsolete files)

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


var dbg = new Debugger;
it.customNative = assertEq;
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
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.
Keywords: mlk
(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.
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.
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.)
Attached patch work-in-progress (obsolete) — Splinter Review
Assignee: general → jimb
Status: NEW → ASSIGNED
Rather than rooting the value, this just stores it in a reserved slot of the 'it' object, which seems more like the intent anyway.
Attachment #714678 - Attachment is obsolete: true
Attachment #714712 - Flags: review?(jwalden+bmo)
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.
Attachment #714712 - Flags: review?(jwalden+bmo) → review+
(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.
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.
Attachment #714712 - Attachment is obsolete: true
Attachment #716751 - Flags: review?(jwalden+bmo)
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.
Attachment #716750 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #716751 - Flags: review?(jwalden+bmo) → review+
I botched the patches I posted here; they were meant to include the regression test from the bug. Rapidly reviewed by Waldo on IRC.
You need to log in before you can comment on or make changes to this bug.