I just noticed this on a TM tip linux x86 debug build on the two tests: js1_8/extensions/regress-394709.js js1_8/extensions/regress-422269.js /moz/safe/js/src/tests $ python jstests.py ../objdbg/js -g regress-394709.js Reading symbols from /moz/safe/js/src/objdbg/js...done. (gdb) r Starting program: /moz/safe/js/src/objdbg/js -f shell.js -f js1_8/shell.js -f js1_8/extensions/shell.js -f ./js1_8/extensions/regress-394709.js [Thread debugging using libthread_db enabled] BUGNUMBER: 394709 STATUS: Do not leak with object.watch and closure Assertion failure: JS_THREAD_DATA(acx)->conservativeGC.isEnabled(), at ../jsgc.cpp:2465 Program received signal SIGABRT, Aborted. 0xb7fe2430 in __kernel_vsyscall () (gdb) bt #0 0xb7fe2430 in __kernel_vsyscall () #1 0xb7fbe230 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42 #2 0x0817be31 in JS_Assert (s=0x8246948 "JS_THREAD_DATA(acx)->conservativeGC.isEnabled()", file=0x8245381 "../jsgc.cpp", ln=2465) at ../jsutil.cpp:84 #3 0x080ca048 in js_TraceRuntime (trc=0xbfffe528) at ../jsgc.cpp:2465 #4 0x08068e98 in JS_TraceRuntime (trc=0xbfffe528) at ../jsapi.cpp:2041 #5 0x0804e0ca in CountHeap (cx=0x82d3b58, argc=0, vp=0xb77b7190) at ../../shell/js.cpp:1393 #6 0x0822302f in js::Interpret (cx=0x82d3b58) at ../jsinterp.cpp:4715 #7 0x080d8889 in js::Execute (cx=0x82d3b58, chain=0xb7502000, script=0x82f0398, down=0x0, flags=0, result=0x0) at ../jsinterp.cpp:907 #8 0x0806f833 in JS_ExecuteScript (cx=0x82d3b58, obj=0xb7502000, script=0x82f0398, rval=0x0) at ../jsapi.cpp:4751 #9 0x0804bfe8 in Process (cx=0x82d3b58, obj=0xb7502000, filename=0xbffff5cd "./js1_8/extensions/regress-394709.js", forceTTY=0) at ../../shell/js.cpp:440 #10 0x0804cbb7 in ProcessArgs (cx=0x82d3b58, obj=0xb7502000, argv=0xbffff3d8, argc=8) at ../../shell/js.cpp:786 #11 0x08055589 in shell (cx=0x82d3b58, argc=8, argv=0xbffff3d8, envp=0xbffff3fc) at ../../shell/js.cpp:5047 #12 0x080556a5 in main (argc=8, argv=0xbffff3d8, envp=0xbffff3fc) at ../../shell/js.cpp:5134

The bug is in in JS_TraceRuntime implementation. It should not just call js_TraceRuntime. Rather it should follow js_GC and properly ensure serialized access the the runtime.

The patch makes JS_TraceRuntime to go through the same setup that js_GC and SetProtoCheckingForCycles uses to serialize access to the runtime. Since JS_TraceRuntime can be called from inside the GC the code has to distinguish a normal GC invocation from runtime navigation. For the patch adds JSRuntime::gcMarkAndSweep flag.

Here is a refreshed patch that applies cleanly against TM f84b470314a8.

Comment on attachment 464787 [details] [diff] [review] v1 refreshed I will fold this patch into the patch for bug 477999.

Comment on attachment 464787 [details] [diff] [review] v1 refreshed > /* >- * Start a new GC session assuming no GC is running on this or other threads. >- * Together with LetOtherGCFinish this function contains the rendezvous >- * algorithm by which we stop the world for GC. >+ * Start a new GC session waiting, if necessary, for a GC session on other >+ * threads to finish. Together with LetOtherGCFinish this function contains >+ * the rendezvous algorithm by which we stop the world for GC. > * Please rephrase the first sentence. > do { > rt->gcPoke = false; > > AutoUnlockGC unlock(rt); > if (firstRun) { > PreGCCleanup(cx, gckind); > TIMESTAMP(startMark); > firstRun = false; > } >- GC(cx GCTIMER_ARG); >+ MarkAndSwep(cx GCTIMER_ARG); nit Sweep... not swep > extern void >diff --git a/js/src/tests/js1_8/extensions/regress-422269.js b/js/src/tests/js1_8/extensions/regress-422269.js >--- a/js/src/tests/js1_8/extensions/regress-422269.js >+++ b/js/src/tests/js1_8/extensions/regress-422269.js >@@ -38,41 +38,65 @@ > //----------------------------------------------------------------------------- > var BUGNUMBER = 422269; > var summary = 'Compile-time let block should not capture runtime references'; > var actual = 'No leak'; > var expect = 'No leak'; > > > //----------------------------------------------------------------------------- > test(); >+ > //----------------------------------------------------------------------------- > >+var slot; >+ >+function run_at_deap_native_stack(f) deep... >+{ >+ var n = 50; >+ var obj = { get x() { >+ if (n == 0) { >+ return f(); >+ } >+ --n; >+ return obj.x; >+ }}; >+ return obj.x; >+} >+ > function test() > { > enterFunc ('test'); > printBugNumber(BUGNUMBER); > printStatus (summary); > > function f() > { > let m = {sin: Math.sin}; > (function() { m.sin(1); })(); > return m; > } > >+ function get_null() >+ { >+ return 1; >+ } >+ Where does this come from? > if (typeof countHeap == 'undefined') > { > expect = actual = 'Test skipped'; > print('Test skipped. Requires countHeap function.'); > } > else > { >+ // Use eval of eval so the result of f is captured deep inside the machine stack >+ // and would not influence > var x = f(); >+ f(); // overwrite the machine stack with new objects > gc(); > var n = countHeap(); > x = null; > gc(); > > var n2 = countHeap(); > if (n2 >= n) > actual = "leak is detected, something roots the result of f"; > }

This still seems to be failing.

(In reply to comment #6) > This still seems to be failing. Do you mean the tests fail with the patch applied?

No, on TM.

Is the patch ready to be committed? I can push it.

(In reply to comment #9) > Is the patch ready to be committed? I can push it. See comment 4 - in that bug I needed to change the functionality in that area again plus the patch attached here had some undesirable effect of capturing needless stack space. So I fixed the issues in that bug.

Ah, missed that, thanks!