Closed
Bug 1143194
Opened 9 years ago
Closed 9 years ago
for-of loops should emit trynotes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox37 | --- | wontfix |
firefox38 | --- | verified |
firefox39 | --- | verified |
firefox-esr31 | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: gkw, Assigned: shu)
References
Details
(5 keywords, Whiteboard: [adv-main38+])
Crash Data
Attachments
(9 files)
2.71 KB,
text/plain
|
Details | |
1.54 KB,
text/plain
|
Details | |
2.32 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
10.80 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.50 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
2.30 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
12.59 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta-
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
// jsfunfuzz-generated code s = newGlobal() function f(code) { try { evalcx(code, s) } catch (e) {} } f(` // Randomly chosen test: js/src/jit-test/tests/debug/bug1116103.js var g = newGlobal() g.parent = this g.eval('new Debugger(parent).onExceptionUnwind=function(){}') // jsfunfuzz-generated code function f() { [...[]] } for (var j = 0; j < 99; ++j) { f() } x = [] g = this Array.prototype.unshift.call(x, '') Array.prototype.unshift.call(x, '') `); f(` for (v of x) { try { Object.defineProperty(g, "", { get: function() {} }) } catch (e) {} } `) asserts js debug shell on m-c changeset 2795a48dfebe with --fuzzing-safe --no-threads --ion-eager at Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h and crashes js opt shell at a weird memory address with js::Invoke on the stack. Configure options: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 2795a48dfebe autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/3127df59e0dc user: Boris Zbarsky date: Fri Dec 12 20:52:40 2014 -0500 summary: Bug 1110871. Ion-compile JSOP_SYMBOL. r=jandem Setting s-s to be safe because lldb seems to show EXC_BAD_ACCESS with bad address 0x10231bf30. Boris, is bug 1110871 a likely regressor?
Flags: needinfo?(bzbarsky)
![]() |
Reporter | |
Comment 1•9 years ago
|
||
(lldb) bt 5 * thread #1: tid = 0x8882a, 0x00000001004c0d37 js-dbg-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::ComputeGetPropResult(JSContext*, js::jit::BaselineFrame*, JSOp, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] JS::Value::isMagic(this=<unavailable>, why=<unavailable>) const + 67 at Value.h:1177, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x00000001004c0d37 js-dbg-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::ComputeGetPropResult(JSContext*, js::jit::BaselineFrame*, JSOp, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] JS::Value::isMagic(this=<unavailable>, why=<unavailable>) const + 67 at Value.h:1177 frame #1: 0x00000001004c0cf4 js-dbg-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::ComputeGetPropResult(JSContext*, js::jit::BaselineFrame*, JSOp, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] js::ValueOperations<JS::MutableHandle<JS::Value> >::isMagic(why=<unavailable>) const at Value.h:1693 frame #2: 0x00000001004c0cf4 js-dbg-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::ComputeGetPropResult(cx=<unavailable>, frame=<unavailable>, op=<unavailable>, name=<unavailable>, val=<unavailable>, res=<unavailable>) + 1284 at BaselineIC.cpp:6856 frame #3: 0x00000001004516f6 js-dbg-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::DoGetPropFallback(cx=0x0000000101f02590, frame=0x00007fff5fbfcc68, stub_=0x0000000104300ba0, val=<unavailable>, res=<unavailable>) + 614 at BaselineIC.cpp:6913 frame #4: 0x0000000101dc5417 (lldb)
![]() |
Reporter | |
Comment 2•9 years ago
|
||
(lldb) bt * thread #1: tid = 0x8661c, 0x000000010231bf30, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x10231bf30) * frame #0: 0x000000010231bf30 frame #1: 0x0000000100132a65 js-64-dm-nsprBuild-darwin-2795a48dfebe`js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [inlined] js::CallJSNative(cx=0x0000000102301fb0, native=0x000000010231bf30, args=0x00007fff5fbfd408)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 66 at jscntxtinlines.h:235 frame #2: 0x0000000100132a23 js-64-dm-nsprBuild-darwin-2795a48dfebe`js::Invoke(cx=0x0000000102301fb0, args=CallArgs at 0x00007fff5fbfd340, construct=<unavailable>) + 275 at Interpreter.cpp:495 frame #3: 0x0000000100121b9e js-64-dm-nsprBuild-darwin-2795a48dfebe`js::Invoke(cx=0x0000000102301fb0, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 510 at Interpreter.cpp:558 frame #4: 0x000000010028c483 js-64-dm-nsprBuild-darwin-2795a48dfebe`js::jit::DoCallFallback(cx=0x0000000102301fb0, frame=0x00007fff5fbfd768, stub_=0x0000000101615d50, argc=0, vp=0x00007fff5fbfd718, res=<unavailable>) + 787 at BaselineIC.cpp:9569 frame #5: 0x00000001017da630 (lldb)
![]() |
||
Comment 3•9 years ago
|
||
> Boris, is bug 1110871 a likely regressor? It's possible, at least. Before that bug, this testcase (or at least parts of it) could not be ion-compiled because of the for-of loop. Bug 1110871 made it so the testcase _is_ ion-compiled. Now why that affects the baseline IC... that I don't know. In any case, js::jit::ComputeGetPropResult is doing this: if (val.isMagic(JS_OPTIMIZED_ARGUMENTS) && IsOptimizedArguments(frame, val)) where val is a value that is isMagic(), but the "why" is JS_OPTIMIZED_OUT, not JS_OPTIMIZED_ARGUMENTS. And the problem is that the one-arg form of isMagic() will fatally assert if the value is in fact magic but not for that reason. That seems ... very odd. Why is that an assert, not a test?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 4•9 years ago
|
||
Oh, and this is a JSOP_CALLPROP with the name "next". So we're iterating. But why the OPTIMIZED_OUT business?
Assignee | ||
Comment 5•9 years ago
|
||
Apparently we have never emitted JSOP_ITER trynotes for for-of loops, only for for-in loops. Compare: js> function f() { for (x of v) { } } js> dis(f) flags: loc op ----- -- main: 00000: getgname "v" 00005: dup 00006: symbol 0 00008: callelem 00009: swap 00010: call 0 00013: undefined 00014: goto 39 (+25) 00019: loophead 00020: dup 00021: getprop "value" 00026: bindgname "x" 00031: pick 1 00033: setgname "x" 00038: pop 00039: loopentry 129 00041: pop 00042: dup 00043: dup 00044: callprop "next" 00049: swap 00050: call 0 00053: dup 00054: getprop "done" 00059: ifeq 19 (-40) 00064: popn 2 00067: retrval Source notes: ofs line pc delta desc args ---- ---- ----- ------ -------- ------ 0: 1 14 [ 14] xdelta 1: 1 14 [ 0] for-of closingjump 45 3: 1 39 [ 25] xdelta 4: 1 39 [ 0] colspan 25 6: 1 50 [ 11] xdelta 7: 1 50 [ 0] colspan -10 12: 1 67 [ 17] xdelta 13: 1 67 [ 0] colspan 14 <= No trynotes! js> function g() { for (x in v) { } } js> dis(g) flags: loc op ----- -- main: 00000: getgname "v" 00005: iter 1 00007: undefined 00008: goto 26 (+18) 00013: loophead 00014: bindgname "x" 00019: pick 1 00021: setgname "x" 00026: loopentry 129 00028: pop 00029: moreiter 00030: isnoiter 00031: ifeq 13 (-18) 00036: pop 00037: enditer 00038: retrval Source notes: ofs line pc delta desc args ---- ---- ----- ------ -------- ------ 0: 3 8 [ 8] xdelta 1: 3 8 [ 0] for-in closingjump 23 3: 3 38 [ 30] xdelta 4: 3 38 [ 0] colspan 29 Exception table: kind stack start end iter 1 13 37 <= JSTRY_ITER note What the above means is that during the in-place debug mode bailout from Ion->Baseline, Ion doesn't think there's a live iterator at a certain stack slot (2), and so writes a JS_OPTIMIZED_OUT magic value into the stack slot.
Assignee | ||
Comment 6•9 years ago
|
||
What the above also means is that we have never closed iterators properly during exception unwinding for for-of loops.
Assignee | ||
Updated•9 years ago
|
Summary: Crash [@ js::Invoke] or Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h → for-of loops should emit JSTRY_ITER trynotes
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #6) > What the above also means is that we have never closed iterators properly > during exception unwinding for for-of loops. So I guess for-of iterators do not actually need to be closed during exception unwinding. Only for-in loops need their iterators closed. Nevertheless, we should emit a note here to say that there is a live iterator object on stack to help the in-place debug mode bailout.
Assignee | ||
Updated•9 years ago
|
Summary: for-of loops should emit JSTRY_ITER trynotes → for-of loops should emit trynotes
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8577576 -
Flags: review?(jorendorff)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8577577 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8577578 -
Flags: review?(jorendorff)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8577579 -
Flags: review?(jorendorff)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8577580 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → shu
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8577581 -
Flags: review?(jorendorff)
Comment 14•9 years ago
|
||
Comment on attachment 8577580 [details] [diff] [review] Handle JSTRY_FOR_OF in TI. Review of attachment 8577580 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ObjectGroup.cpp @@ +209,5 @@ > } > > /* > + * All loops in the script will have a JSTRY_FOR_IN, JSTRY_FOR_OF, or > + * JSTRY_LOOP try note indicating their boundary. How about 'All loops in the script will have a try note indicating their boundary'
Attachment #8577580 -
Flags: review?(bhackett1024) → review+
Updated•9 years ago
|
Attachment #8577577 -
Flags: review?(jdemooij) → review+
Comment 15•9 years ago
|
||
Hey Shu, we're trying to assign this a security rating. Does this only occur if a user is debugging JS on a page? Do you have an opinion on the severity?
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Matt Wobensmith from comment #15) > Hey Shu, we're trying to assign this a security rating. Does this only occur > if a user is debugging JS on a page? Do you have an opinion on the severity? Yeah, sec-medium.
Updated•9 years ago
|
Keywords: sec-moderate
Updated•9 years ago
|
Attachment #8577576 -
Flags: review?(jorendorff) → review+
Updated•9 years ago
|
Attachment #8577578 -
Flags: review?(jorendorff) → review+
Updated•9 years ago
|
Attachment #8577579 -
Flags: review?(jorendorff) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8577581 [details] [diff] [review] Skip JSTRY_FOR_OF when unwinding exceptions. Review of attachment 8577581 [details] [diff] [review]: ----------------------------------------------------------------- Do we have a test that fails if you apply part 1 without this? If not, we probably should.
Attachment #8577581 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #17) > Comment on attachment 8577581 [details] [diff] [review] > Skip JSTRY_FOR_OF when unwinding exceptions. > > Review of attachment 8577581 [details] [diff] [review]: > ----------------------------------------------------------------- > > Do we have a test that fails if you apply part 1 without this? If not, we > probably should. We do, it's covered by tests/for-of/throw.js
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa10cd6d6ea0 https://hg.mozilla.org/mozilla-central/rev/117d8de21ad5 https://hg.mozilla.org/mozilla-central/rev/2f99a9eb19ee https://hg.mozilla.org/mozilla-central/rev/8b1896147f41 https://hg.mozilla.org/mozilla-central/rev/5ff1e40be5f9 https://hg.mozilla.org/mozilla-central/rev/12df53d34b74
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 20•9 years ago
|
||
Please nominate this for Aurora/Beta approval when you get a chance :)
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox-esr31:
--- → unaffected
Flags: needinfo?(shu)
Assignee | ||
Comment 21•9 years ago
|
||
Bug 1143194 - Emit JSTRY_FOR_OF notes for for-of loops. (r=jorendorff) * * * Bug 1143194 - Handle JSTRY_FOR_OF when bailing out in-place from Ion for debug mode. (r=jandem) * * * Bug 1143194 - Rename JSTRY_ITER to JSTRY_FOR_IN. (r=jorendorff) * * * Bug 1143194 - Make trynote checking in BytecodeParse clearer. (r=jorendorff) * * * Bug 1143194 - Handle JSTRY_FOR_OF in TI. (r=bhackett) * * * Bug 1143194 - Skip JSTRY_FOR_OF when unwinding exceptions. (r=jorendorff)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8581906 [details] [diff] [review] Rollup for backport Approval Request Comment [Feature/regressing bug #]: Not sure. Whatever introduced for-of. [User impact if declined]: Possibly rare incorrect behavior when using for-of loops together with the frontend debugger. [Describe test coverage new/current, TreeHerder]: on m-c [Risks and why]: Very low risk; just a bug fix. [String/UUID change made/needed]: None.
Flags: needinfo?(shu)
Attachment #8581906 -
Flags: approval-mozilla-beta?
Attachment #8581906 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8581906 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 23•9 years ago
|
||
Comment on attachment 8581906 [details] [diff] [review] Rollup for backport It's too late to take a non critical fix for 37. We can consider this for 38. Beta-
Attachment #8581906 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•9 years ago
|
Comment 24•9 years ago
|
||
Comment on attachment 8581906 [details] [diff] [review] Rollup for backport See comment 22.
Attachment #8581906 -
Flags: approval-mozilla-b2g37?
Updated•9 years ago
|
Attachment #8581906 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•9 years ago
|
Comment 26•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx38
Updated•9 years ago
|
Whiteboard: [adv-main38+]
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•