585686 - Assertion failure: JS_THREAD_DATA(acx)->conservativeGC.isEnabled()

Status: RESOLVED DUPLICATE of bug 477999

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

Comment 1

[Igor Bukanov]

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.

Comment 2

[Igor Bukanov]

Attachment: v1

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.

Comment 3

[Igor Bukanov]

Attachment: v1 refreshed

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

Comment 4

[Igor Bukanov]

Comment on attachment 464787 [details] [diff] [review]
v1 refreshed

I will fold this patch into the patch for bug 477999.

Comment 5

[Gregor Wagner [:gwagner]]

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";
>   }

Comment 6

[Luke Wagner [:luke]]

This still seems to be failing.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> This still seems to be failing.

Do you mean the tests fail with the patch applied?

Comment 8

[Luke Wagner [:luke]]

No, on TM.

Comment 9

[Luke Wagner [:luke]]

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

Comment 10

[Igor Bukanov]

(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.

Comment 11

[Luke Wagner [:luke]]

Ah, missed that, thanks!