857050 - [jsdbg2] Assertion failure: isEmpty(), at ./dist/include/mozilla/LinkedList.h:269 with timeout

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision aae004a3c5d9 (no options required):


function timeoutfunc() {}
timeout(1, timeoutfunc);
var g = newGlobal('new-compartment');
var dbg = Debugger(g);

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

This assertion might look a little familiar... ;-)

Arguably this should go after the DestroyContext call.  But it happens that JS_DestroyRuntime doesn't GC right now, which means putting it there still hits the assert.  Given we want to unify context/runtime eventually, probably JS_DestroyContext should do a final GC.  But that's soooo much a change for another bug.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 732466 [details] [diff] [review]
Patch

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

::: js/src/shell/js.cpp
@@ +5350,4 @@
>          printf("OOM max count: %u\n", OOM_counter);
>  #endif
>  
> +    JS_RemoveValueRootRT(rt, &gTimeoutFunc);

Do we know for a fact that the operation callback can't possibly fire between here and the destruction of the runtime? I'd be a little more comfortable if we set gTimeoutFunc to null before removing its value root, so that it can't possibly matter.

Comment 3

[Jeff Walden [:Waldo]]

Good point -- set it to null before removing the root.  I think the callback only happens while script is running, and at that point none should be, in theory, but better safe than sorry.  And this is much clearer anyway about giving up GC-safe ownership of the timeout function.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4e08f88780

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a4e08f88780