Bug 757400: When we change a compartment's debug mode, ensure we throw away all the outdated analyses. r=billm

Always do a cleansing GC when we turn debug mode off, as well as when we
turn it on. If we don't, it's possible to have stale analyses (generated in
the wrong debug mode state) stick around, and we'll assert in JM.

Have ShouldCleanUpEverything recognize gcreason::DEBUG_MODE_GC as a reason
to discard all jit code and analyses.

js/src/jit-test/tests/debug/clear-old-analyses-01.js

@@ -0,0 +1,38 @@
+// |jit-test| mjitalways; error:AllDone
+// When we enter debug mode in a compartment, we must throw away all
+// analyses in that compartment (debug mode affects the results of
+// analysis, so they become out of date). This is true even when we would
+// otherwise be retaining jit code and its related data structures for
+// animation timing.
+
+if (typeof gcPreserveCode != "function")
+  throw('AllDone');
+
+var g = newGlobal('new-compartment');
+var dbg = new Debugger;
+
+g.eval("" +
+       function fib(n) {
+         var a = 0, b = 1;
+         while (n-- > 0)
+           b = b+a, a = b-a;
+         return b;
+       });
+
+g.fib(20);                      // Cause g.fib to be jitted. This creates an analysis with
+                                // debug mode off.
+
+gcPreserveCode();               // Tell the gc to preserve JIT code and analyses by
+                                // default. A recent call to js::NotifyAnimationActivity
+                                // could have a similar effect in real life.
+
+dbg.addDebuggee(g);             // Put g in debug mode. This triggers a GC which must
+                                // clear all analyses. In the original buggy code, we also
+                                // release all of g's scripts' JIT code, leading to a
+                                // recompilation the next time it was called.
+
+g.fib(20);                      // Run g.fib again, causing it to be re-jitted. If the
+                                // original analysis is still present, JM will assert,
+                                // because it is not in debug mode.
+
+throw('AllDone');

js/src/jit-test/tests/debug/clear-old-analyses-02.js

@@ -0,0 +1,39 @@
+// |jit-test| mjitalways; error:AllDone
+// When we leave debug mode in a compartment, we must throw away all
+// analyses in that compartment (debug mode affects the results of
+// analysis, so they become out of date). We cannot skip this step when
+// there are debuggee frames on the stack.
+
+var g = newGlobal('new-compartment');
+var dbg = new Debugger();
+var gw = dbg.addDebuggee(g);
+
+g.eval("" +
+       function fib(n) {
+         var a = 0, b = 1;
+         while (n-- > 0)
+           b = b+a, a = b-a;
+         return b;
+       });
+
+
+// Cause g.fib to be jitted. This creates an analysis with debug mode on.
+g.fib(20);
+
+// Setting a breakpoint in g.f causes us to throw away the jit code, but
+// not the analysis.
+gw.makeDebuggeeValue(g.fib).script.setBreakpoint(0, { hit: function (f) { } });
+
+// Take g out of debug mode, with debuggee code on the stack. In older
+// code, this would not trigger a cleansing GC, so the script will
+// retain its analysis.
+dbg.onDebuggerStatement = function (f) {
+  dbg.removeDebuggee(g);
+};
+g.eval('debugger');
+
+// Run g.fib again, causing it to be re-jitted. If the original analysis is
+// still present, JM will assert, because it is not in debug mode.
+g.fib(20);
+
+throw('AllDone');

js/src/jscompartment.cpp

@@ -648,26 +648,27 @@ JSCompartment::updateForDebugMode(FreeOp *fop, AutoDebugModeGC &dmgc)
 #ifdef JS_METHODJIT
     bool enabled = debugMode();
 
-    if (enabled)
-        JS_ASSERT(!hasScriptsOnStack());
-    else if (hasScriptsOnStack())
-        return;
+    JS_ASSERT_IF(enabled, !hasScriptsOnStack());
 
     for (gc::CellIter i(this, gc::FINALIZE_SCRIPT); !i.done(); i.next()) {
         JSScript *script = i.get<JSScript>();
-        mjit::ReleaseScriptCode(fop, script);
         script->debugMode = enabled;
     }
 
-    // Discard JIT code and bytecode analysis for all scripts in this
-    // compartment. Because !hasScriptsOnStack(), it suffices to do a garbage
-    // collection cycle or to finish the ongoing GC cycle. The necessary
-    // cleanup happens in JSCompartment::sweep.
+    // When we change a compartment's debug mode, whether we're turning it
+    // on or off, we must always throw away all analyses: debug mode
+    // affects various aspects of the analysis, which then get baked into
+    // SSA results, which affects code generation in complicated ways. We
+    // must also throw away all JIT code, as its soundness depends on the
+    // analyses.
+    //
+    // It suffices to do a garbage collection cycle or to finish the
+    // ongoing GC cycle. The necessary cleanup happens in
+    // JSCompartment::sweep.
     //
     // dmgc makes sure we can't forget to GC, but it is also important not
     // to run any scripts in this compartment until the dmgc is destroyed.
     // That is the caller's responsibility.
-    //
     if (!rt->gcRunning)
         dmgc.scheduleGC(this);
 #endif

js/src/jscompartment.h

@@ -391,6 +391,10 @@ class js::AutoDebugModeGC
     explicit AutoDebugModeGC(JSRuntime *rt) : rt(rt), needGC(false) {}
 
     ~AutoDebugModeGC() {
+        // Under some circumstances (say, in the midst of an animation),
+        // the garbage collector may try to retain JIT code and analyses.
+        // The DEBUG_MODE_GC reason forces the collector to always throw
+        // everything away, as required for debug mode transitions.
         if (needGC)
             GC(rt, GC_NORMAL, gcreason::DEBUG_MODE_GC);
     }

js/src/jsgc.cpp

@@ -3813,7 +3813,14 @@ IsDeterministicGCReason(gcreason::Reason reason)
 static bool
 ShouldCleanUpEverything(JSRuntime *rt, gcreason::Reason reason)
 {
-    return !rt->hasContexts() || reason == gcreason::CC_FORCED;
+    // During shutdown, we must clean everything up, for the sake of leak
+    // detection. When a runtime has no contexts, or we're doing a forced GC,
+    // those are strong indications that we're shutting down.
+    //
+    // DEBUG_MODE_GC indicates we're discarding code because the debug mode
+    // has changed; debug mode affects the results of bytecode analysis, so
+    // we need to clear everything away.
+    return !rt->hasContexts() || reason == gcreason::CC_FORCED || reason == gcreason::DEBUG_MODE_GC;
 }
 
 static void