487845 - TM: After deep-bailing, we can lirbuf->rewind() and then return to a dead code page

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

(which isn't a good thing)

Comment 1

[Jason Orendorff [:jorendorff]]

Test case.

diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -4893,16 +4893,17 @@ js_OverfullFragmento(Fragmento *frago, s
 
 JS_REQUIRES_STACK void
 js_FlushJITCache(JSContext* cx)
 {
     if (!TRACING_ENABLED(cx))
         return;
     debug_only_v(printf("Flushing cache.\n");)
     JSTraceMonitor* tm = &JS_TRACE_MONITOR(cx);
+    JS_ASSERT(!tm->prohibitRecording);
     if (tm->recorder)
         js_AbortRecording(cx, "flush cache");
     TraceRecorder* tr;
     while ((tr = tm->abortStack) != NULL) {
         tr->removeFragmentoReferences();
         tr->deepAbort();
         tr->popAbortStack();
     }
diff --git a/js/src/trace-test.js b/js/src/trace-test.js
--- a/js/src/trace-test.js
+++ b/js/src/trace-test.js
@@ -4991,16 +4991,34 @@ function testDeepPropertyShadowing()
     var tree = {__proto__: {__proto__: {parent: null}}};
     h(tree);
     h(tree);
     tree.parent = {};
     assertEq(h(tree), 2);
 }
 test(testDeepPropertyShadowing);
 
+// Complicated whitebox test for bug 487845.
+function testGlobalShapeChangeAfterDeepBail() {
+    function f(name) {
+        this[name] = 1;  // may change global shape
+        for (var i = 0; i < 4; i++)
+            ; // MonitorLoopEdge eventually triggers assertion
+    }
+
+    // When i==3, deep-bail, then change global shape enough times to exhaust
+    // the array of GlobalStates.
+    var arr = [[], [], [], ["bug0", "bug1", "bug2", "bug3", "bug4"]];
+    for (var i = 0; i < arr.length; i++)
+        arr[i].forEach(f);
+}
+testGlobalShapeChangeAfterDeepBail();
+for (let i = 0; i < 5; i++)
+    delete this["bug" + i];
+
 /*****************************************************************************
  *                                                                           *
  *  _____ _   _  _____ ______ _____ _______                                  *
  * |_   _| \ | |/ ____|  ____|  __ \__   __|                                 *
  *   | | |  \| | (___ | |__  | |__) | | |                                    *
  *   | | | . ` |\___ \|  __| |  _  /  | |                                    *
  *  _| |_| |\  |____) | |____| | \ \  | |                                    *
  * |_____|_| \_|_____/|______|_|  \_\ |_|                                    *

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1 - WIP

This compiles and passes the test.  I enabled recording after a deep-bail, as it's only flushing we mustn't do.  Need to think it over some more though and run more tests.

Andreas, feel free to steal this and push it in!

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Not much different -- an extra assertion or two is all.  js/tests and `make check` pass with this and the patch in bug 487134.  I'm excited.  Try server next.

Comment 4

[Andreas Gal :gal]

Comment on attachment 372443 [details] [diff] [review]
v2

Nice test case.

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/4c157cfe2289

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4c157cfe2289

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

gc();
(function(){for (var a = 0; a < 3; ++a) { if (a % 6 == 2) { gc() }  }})()

asserts dbg js shell with -j at Assertion failure: !tm->needFlush, at ../jstracer.cpp:2637, seems to work as expected in opt js shell with -j.

I'm on http://hg.mozilla.org/tracemonkey/rev/99143ce38e68 which is TM tip at the time of testing and filing. This is roastin' jsfunfuzz boxes, reopening.

Comment 8

[Andreas Gal :gal]

backed out http://hg.mozilla.org/tracemonkey/rev/1e9079e22c18

Comment 9

[Jason Orendorff [:jorendorff]]

The first hunk below fixes it, for me.

The second hunk just adds another assert.

diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
--- a/js/src/jscntxt.cpp
+++ b/js/src/jscntxt.cpp
@@ -104,16 +104,19 @@ FinishThreadData(JSThreadData *data)
 static void
 PurgeThreadData(JSContext *cx, JSThreadData *data)
 {
 # ifdef JS_TRACER
     JSTraceMonitor *tm = &data->traceMonitor;
     tm->reservedDoublePoolPtr = tm->reservedDoublePool;
     tm->needFlush = JS_TRUE;

+    if (tm->recorder)
+        tm->recorder->deepAbort();
+
     /*
      * We want to keep tm->reservedObjects after the GC. So, unless we are
      * shutting down, we don't purge them here and rather mark them during
      * the GC, see MarkReservedObjects in jsgc.cpp.
      */
     if (cx->runtime->state == JSRTS_LANDING)
         tm->reservedObjects = NULL;
 # endif
diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -4474,17 +4474,17 @@ js_MonitorLoopEdge(JSContext* cx, uintN&
 JS_REQUIRES_STACK JSMonitorRecordingStatus
 TraceRecorder::monitorRecording(JSContext* cx, TraceRecorder* tr, JSOp op)
 {
     /* Process deepAbort() requests now. */
     if (tr->wasDeepAborted()) {
         js_AbortRecording(cx, "deep abort requested");
         return JSMRS_STOP;
     }
-
+    JS_ASSERT(!JS_TRACE_MONITOR(cx).needFlush);
     JS_ASSERT(!tr->fragment->lastIns);

     /*
      * Clear one-shot state used to communicate between record_JSOP_CALL and post-
      * opcode-case-guts record hook (record_FastNativeCallComplete).
      */
     tr->pendingTraceableNative = NULL;

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v3

This is just v2 + the fix above.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Was backed out, clearing flag -- would love to see it land again soon, though!

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/ef324cc3103d

Comment 13

[Brendan Eich [:brendan]]

This added an NDEBUG warning:

Darwin_OPT.OBJ/made:../jstracer.cpp:3655: warning: unused variable ‘tm’

The single-use tm variable's only use is in a JS_ASSERT; should just eliminate the single-use variable.

/be

Comment 14

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/ef324cc3103d

Comment 15

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dded8cfa2a35

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/88343c2bfbba

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/88343c2bfbba

Comment 18

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

in-testsuite? for Gary's test in comment 7.

Comment 19

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js