TM: Crash [@ js_HashString] with "continue LABEL;" (in ojay 0.2.1)

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: jorendorff)

Tracking

(4 keywords)

Trunk
x86
Mac OS X
crash, regression, testcase, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 unaffected)

Details

(Whiteboard: fixed-in-tracemonkey, crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Created attachment 395495 [details]
reasonably simple shell testcase
(In reply to comment #0)
> Created an attachment (id=395495) [details]
> reasonably simple shell testcase

autoBisect shows this is probably related to bug 508051:

The first bad revision is:
changeset:   31476:d1171f8cc234
user:        Jason Orendorff
date:        Fri Aug 14 15:47:04 2009 -0500
summary:     Bug 508051 part 1 - Avoid imacros for JSOP_SETELEM. Re-landing per comment 8. r=gal.


Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000002000000d
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-darwin              	0x000c5eb0 js_HashString + 112
1   js-opt-tm-darwin              	0x00026e81 JS_DHashTableOperate + 33
2   js-opt-tm-darwin              	0x0001c0ba js_AtomizeString + 74
3   js-opt-tm-darwin              	0x000f10be SetPropertyByName(JSContext*, JSObject*, JSString**, long*) + 142
4   ???                           	0x001f0b14 0 + 2034452
5   ???                           	0xbfffcb98 0 + 3221212056
6   ???                           	0x001f0cf4 0 + 2034932
7   ???                           	0xbffff1f8 0 + 3221221880
8   js-opt-tm-darwin              	0x00107f4a js_MonitorLoopEdge(JSContext*, unsigned int&) + 2250
9   js-opt-tm-darwin              	0x00057fd7 js_Interpret + 44135
10  js-opt-tm-darwin              	0x0005ce22 js_Execute + 386
11  js-opt-tm-darwin              	0x0000eabc JS_ExecuteScript + 60
12  js-opt-tm-darwin              	0x00004d50 Process(JSContext*, JSObject*, char*, int) + 1616
13  js-opt-tm-darwin              	0x00007ecf main + 879
14  js-opt-tm-darwin              	0x0000244b _start + 209
15  js-opt-tm-darwin              	0x00002379 start + 41
Blocks: 508051
Flags: blocking1.9.2?
Keywords: regression
(Assignee)

Comment 2

9 years ago
Reduced slightly:

  for (var i = 0; i < 9; i++) {
      (function() {
          outer:
          for (var j = 0; j < 1; j++)
              for (var p in {x:1})
                  if (p > "q")
                      continue outer;
          return 0;
       })();
  }

It's pretty sensitive: remove the return statement and it'll stop crashing.

The trace isn't unboxing p in this case, or something.
Assignee: general → jorendorff
(Assignee)

Comment 3

9 years ago
Ah, after I fix a non-crashing bug in how we handle `continue LABEL;`, this reduces to:

  outer:
  for (var j = 0; j < 10; j++)
      for (var p in {x:1})
          if (p > "q")
              continue outer;

Still crashes. I'll file a separate bug for `continue LABEL;` since I have a patch that includes a separate test case.
(Assignee)

Comment 4

9 years ago
Created attachment 395961 [details] [diff] [review]
v1

nanojit is throwing away stack writes before js_CallTree because it doesn't expect js_CallTree to access the stack!

The solution is to emit an xbarrier before js_CallTree.

(This bites us in this particular test case because when we hit `continue label`, the inner tree exits at a pc with sp two words *lower* than its entry point.  nanojit throws away stores in that two-word region of the stack.  I think it could be much more than two words, though, if the continue/break were to leave a let-block or multiple for-in loops.)
Attachment #395961 - Flags: review?(gal)

Updated

9 years ago
Attachment #395961 - Flags: review?(gal) → review+

Comment 5

9 years ago
Comment on attachment 395961 [details] [diff] [review]
v1

Take an OOM_EXIT snapshot at the top and then use that in all 3 locations in the patch (sp and rp stack overflows) by making new guard records for it 3 times.


>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -4188,16 +4188,25 @@ TraceRecorder::prepareTreeCall(Fragment*
>         lir->insStorei(inner_sp_ins = lir->ins2i(LIR_piadd, lirbuf->sp,
>                 - treeInfo->nativeStackBase /* rebase sp to beginning of outer tree's stack */
>                 + sp_adj /* adjust for stack in outer frame inner tree can't see */
>                 + ti->nativeStackBase), /* plus the inner tree's stack base */
>                 lirbuf->state, offsetof(InterpState, sp));
>         lir->insStorei(lir->ins2i(LIR_piadd, lirbuf->rp, rp_adj),
>                 lirbuf->state, offsetof(InterpState, rp));
>     }
>+
>+    /*
>+     * The inner tree will probably access stack slots. So tell nanojit not to
>+     * discard or defer stack writes before calling js_CallTree.
>+     *
>+     * (The ExitType of this snapshot is nugatory. The exit can't be taken.)
>+     */
>+    LIns* guardRec = createGuardRecord(snapshot(OOM_EXIT));
>+    lir->insGuard(LIR_xbarrier, NULL, guardRec);
> }
> 
> static unsigned
> BuildGlobalTypeMapFromInnerTree(Queue<JSTraceType>& typeMap, VMSideExit* inner)
> {
> #if defined DEBUG
>     unsigned initialSlots = typeMap.length();
> #endif
>diff --git a/js/src/trace-test/tests/basic/testContinueWithLabel3.js b/js/src/trace-test/tests/basic/testContinueWithLabel3.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/trace-test/tests/basic/testContinueWithLabel3.js
>@@ -0,0 +1,6 @@
>+// bug 511575 comment 3
>+outer:
>+for (var j = 0; j < 10; j++)
>+    for (var p in {x:1})
>+        if (p > "q")
>+            continue outer;
>diff --git a/js/src/trace-test/tests/basic/testContinueWithLabel4.js b/js/src/trace-test/tests/basic/testContinueWithLabel4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/trace-test/tests/basic/testContinueWithLabel4.js
>@@ -0,0 +1,25 @@
>+// bug 511575
>+
>+Function.prototype.X = 42;
>+
>+function ownProperties() {
>+    var props = {};
>+    var r = function () {};
>+
>+    outer:
>+    for (var a in r) {
>+        for (var b in r) {
>+            if (a == b) {
>+                continue outer;
>+            }
>+        }
>+        print("SHOULD NOT BE REACHED");
>+        props[a] = true;
>+    }
>+    return props;
>+}
>+
>+for (var i = 0; i < 12; ++i) {
>+    print(i);
>+    ownProperties();
>+}
(Assignee)

Updated

9 years ago
Summary: TM: Crash [@ js_HashString] with ojay 0.2.1 → TM: Crash [@ js_HashString] with "continue LABEL;" (in ojay 0.2.1)
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/tracemonkey/rev/1c95347e920c
Whiteboard: fixed-in-tracemonkey

Comment 7

9 years ago
http://hg.mozilla.org/mozilla-central/rev/1c95347e920c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+

Updated

8 years ago
Priority: -- → P1

Comment 9

8 years ago
js/src/trace-test/tests/basic/testContinueWithLabel3.js
js/src/trace-test/tests/basic/testContinueWithLabel4.js
Flags: in-testsuite+

Comment 10

8 years ago
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
status1.9.1: --- → unaffected
Group: core-security
Crash Signature: [@ js_HashString]
You need to log in before you can comment on or make changes to this bug.