1143194 - for-of loops should emit trynotes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

// 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?

Comment 1

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

Attachment: debug stack

(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)

Comment 2

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

Attachment: opt stack

(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

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and this is a JSOP_CALLPROP with the name "next".  So we're iterating.  But why the OPTIMIZED_OUT business?

Comment 5

[Shu-yu Guo [:shu]]

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.

Comment 6

[Shu-yu Guo [:shu]]

What the above also means is that we have never closed iterators properly during exception unwinding for for-of loops.

Comment 7

[Shu-yu Guo [:shu]]

(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.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Emit JSTRY_FOR_OF notes for for-of loops.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Handle JSTRY_FOR_OF when bailing out in-place from Ion for debug mode.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Rename JSTRY_ITER to JSTRY_FOR_IN.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Make trynote checking in BytecodeParse clearer.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Handle JSTRY_FOR_OF in TI.

Comment 13

[Shu-yu Guo [:shu]]

Attachment: Skip JSTRY_FOR_OF when unwinding exceptions.

Comment 14

[Brian Hackett [Laid off!]]

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'

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

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?

Comment 16

[Shu-yu Guo [:shu]]

(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.

Comment 17

[Jason Orendorff [:jorendorff]]

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.

Comment 18

[Shu-yu Guo [:shu]]

(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

[Phil Ringnalda (:philor)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta approval when you get a chance :)

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Rollup for backport

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)

Comment 22

[Shu-yu Guo [:shu]]

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.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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-

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8581906 [details] [diff] [review]
Rollup for backport

See comment 22.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa0d8e209af6

Comment 26

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx38

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/651c36e1d0fc