Closed Bug 487845 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b4

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

(which isn't a good thing)
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];
+
 /*****************************************************************************
  *                                                                           *
  *  _____ _   _  _____ ______ _____ _______                                  *
  * |_   _| \ | |/ ____|  ____|  __ \__   __|                                 *
  *   | | |  \| | (___ | |__  | |__) | | |                                    *
  *   | | | . ` |\___ \|  __| |  _  /  | |                                    *
  *  _| |_| |\  |____) | |____| | \ \  | |                                    *
  * |_____|_| \_|_____/|______|_|  \_\ |_|                                    *
Attached patch v1 - WIP (obsolete) — Splinter Review
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!
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #372141 - Attachment is obsolete: true
Attachment #372443 - Flags: review?(gal)
Comment on attachment 372443 [details] [diff] [review]
v2

Nice test case.
Attachment #372443 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/4c157cfe2289
Whiteboard: fixed-in-tracemonkey
Flags: blocking1.9.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b4
http://hg.mozilla.org/mozilla-central/rev/4c157cfe2289
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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;
Attached patch v3Splinter Review
This is just v2 + the fix above.
Attachment #372443 - Attachment is obsolete: true
Attachment #372868 - Flags: review?(gal)
Attachment #372868 - Flags: review?(gal) → review+
Was backed out, clearing flag -- would love to see it land again soon, though!
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/tracemonkey/rev/ef324cc3103d
Whiteboard: fixed-in-tracemonkey
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
http://hg.mozilla.org/mozilla-central/rev/ef324cc3103d
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

in-testsuite? for Gary's test in comment 7.
Flags: in-testsuite?
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
You need to log in before you can comment on or make changes to this bug.