1346862 - Crash [@ js::UnwindEnvironment] or Assertion failure: !done(), at vm/Scope.h:1363

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision f9362554866b (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --baseline-eager):

loadFile(`
  iterable = {}
  iterable[Symbol.iterator] = function() {
    return {
        next() {
            return {}
        },
        return() {
            yield;
        }
    }
  }
  for (x of iterable) {
    try {
      return;
    } catch (get) {}
    let f;
  }
`);
function loadFile(lfVarx) {
  Function(lfVarx)();
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::UnwindEnvironment (cx=cx@entry=0x7ffff6926800, ei=..., pc=<optimized out>) at js/src/vm/Interpreter.cpp:1047
#0  js::UnwindEnvironment (cx=cx@entry=0x7ffff6926800, ei=..., pc=<optimized out>) at js/src/vm/Interpreter.cpp:1047
#1  0x00000000006513ee in js::jit::SettleOnTryNote (cx=cx@entry=0x7ffff6926800, tn=tn@entry=0x7ffff69ea9e8, frame=..., ei=..., rfe=rfe@entry=0x7fffffffc268, pc=pc@entry=0x7fffffffbc78) at js/src/jit/JitFrames.cpp:522
#2  0x0000000000655409 in js::jit::ProcessTryNotesBaseline (pc=0x7fffffffbc78, rfe=0x7fffffffc268, ei=..., frame=..., cx=0x7ffff6926800) at js/src/jit/JitFrames.cpp:604
#3  js::jit::HandleExceptionBaseline (pc=0x7ffff438c941 ":", rfe=0x7fffffffc268, frame=..., cx=0x7ffff6926800) at js/src/jit/JitFrames.cpp:724
#4  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:890
#5  0x0000356b9406d15b in ?? ()
[...]
#22 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffbdc0	140737488338368
rcx	0x1ac1460	28054624
rdx	0x0	0
rsi	0x1ac1500	28054784
rdi	0x7fffffffbdc0	140737488338368
rbp	0x7ffff6926800	140737330178048
rsp	0x7fffffffbb20	140737488337696
r8	0x7ffff69eaa28	140737330981416
r9	0x7ffff69ea9b0	140737330981296
r10	0x7ffff69ea980	140737330981248
r11	0x0	0
r12	0xdfb5dc	14661084
r13	0x7fffffffffff	140737488355327
r14	0x7fffffffbdd8	140737488338392
r15	0x7fffffffbb20	140737488337696
rip	0x4cb105 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+197>
=> 0x4cb105 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+197>:	cmpb   $0xe,(%rdx)
   0x4cb108 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+200>:	ja     0x4cb141 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+257>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20170113151323" and the hash "0431a0ab907d53646d359ad10507efe7f89dd487".
The "bad" changeset has the timestamp "20170114194423" and the hash "18ab7878c67ba22b2f98bcc7a1e94b826061dd19".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0431a0ab907d53646d359ad10507efe7f89dd487&tochange=18ab7878c67ba22b2f98bcc7a1e94b826061dd19

Comment 2

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

Slightly more reduced testcase:

(function () {
    z = {}
    z[Symbol.iterator] = function () {
        return {
            next() {
                return {}
            },
            return () {
                yield;
            }
        }
    }
    for (x of z) {
        try {
            return;
        } catch (e) {}
        let f;
    }
})()

Comment 3

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

Using the testcase in comment 2, with --fuzzing-safe --no-threads --no-baseline --no-ion:

Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/757b50c0ee48
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Implement IteratorClose for for-of. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/8ad6c93e5162
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Rename allowContentSpread to allowContentIter. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/d7d332a5b2e8
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/ce293b3c0a8b
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:38 2017 -0800
summary:     Bug 1147371 - Implement IteratorClose for array destructuring. (r=arai)

changeset:   https://hg.mozilla.org/mozilla-central/rev/e0dc4150f8ac
user:        Shu-yu Guo
date:        Sat Jan 14 14:51:39 2017 -0800
summary:     Bug 1147371 - Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)


Shu-yu, is bug 1147371 a likely regressor?

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Also includes a light refactoring of flushPops.

Comment 5

[Tooru Fujisawa [:arai]]

looks good for this specific situation, but I don't yet understand how this change interacts with implicit try-catch for IteratorClose, and try-catch/try-finally outside of the innermost for-of, especially when the local jump is break/continue.

I won't be able to do thorough review today and tomorrow,
if this is urgent, feel free to redirect.
if not, I'll review it this Friday or weekend.

Comment 6

[Tooru Fujisawa [:arai]]

maybe we should try moving IterarotClose code outside of the loop body, and jump there from non-local jump source?
(of course in another bug tho)

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #5)
> looks good for this specific situation, but I don't yet understand how this
> change interacts with implicit try-catch for IteratorClose, and
> try-catch/try-finally outside of the innermost for-of, especially when the
> local jump is break/continue.
> 

The implicit try-catch around IteratorClose should still apply, since the IteratorClose's JSTRY_CATCH would match (and then terminate the try-note iteration in HandleError) before JSTRY_FOR_OF_ITERCLOSE. The THROW op should also be outside of the range of JSTRY_FOR_OF_ITERCLOSE.

|continue| wouldn't call IteratorClose, so that's fine.

As for try-catch and try-finally outside of the innermost for-of, are you thinking of something like the following?

for (x of iter) {
  try {
    try {
      break;
    } catch (e1) {}
  } catch (e2) {}
}

In that case, inside HandleError, it'll look like we threw from IteratorClose inside |break|, setting |inForOfIterClose = true|. That won't be reset to false until we the try-note iter finds the JSTRY_FOR_OF, meaning neither of the two catch blocks will match.

> I won't be able to do thorough review today and tomorrow,
> if this is urgent, feel free to redirect.
> if not, I'll review it this Friday or weekend.

Thanks for letting me know. Not urgent, I'll wait.

Comment 8

[Tooru Fujisawa [:arai]]

thanks :)

what I'm thinking is:

try {
  for (x of y) {
    break/return;
  }
} catch/finally {}

or 2 or more for-of loops and break/continue to label

outer: for (x of y) {
  try { // syntactic, or implicit for for-of
    for (z of w) {
      break outer/continue outer/return;
    }
  } catch/finally {
  }
}

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #8)
> thanks :)
> 
> what I'm thinking is:
> 
> try {
>   for (x of y) {
>     break/return;
>   }
> } catch/finally {}

1. JSTRY_FOR_OF_ITERCLOSE will set inForOfIterClose = true
2. The try-note iterator will see JSTRY_FOR_OF when iterating and set inForOfIterClose = false
3. The try-note iterator will see JSTRY_CATCH, and since inForOfIterClose == false, it will jump to it.

> 
> or 2 or more for-of loops and break/continue to label
> 
> outer: for (x of y) {
>   try { // syntactic, or implicit for for-of
>     for (z of w) {
>       break outer/continue outer/return;
>     }
>   } catch/finally {
>   }
> }

The same thing as above, really. The try-note iterator should see things in the right order.

Comment 10

[Tooru Fujisawa [:arai]]

ah, makes sense :D
thank you for the detailed answer

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8847394 [details] [diff] [review]
Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Review of attachment 8847394 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/JitFrames.cpp
@@ +616,5 @@
>                  continue;
>  
> +            // See corresponding comment in ProcessTryNotes.
> +            if (inForOfIterClose)
> +                continue;

Can we align either to "break" or "continue" between ProcessTryNotesBaseline and  HandleExceptionIon ?
(looks like both have same effect here and there)

::: js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js
@@ +15,5 @@
> +    assertThrowsValue(function() {
> +        for (var x of iterable) {
> +            try {
> +                return;
> +            } catch (e) {

how about doing "catchEntered++" here and assert it's not executed?

::: js/src/vm/Interpreter.cpp
@@ +1184,5 @@
> +            //   2. Try-catch notes cannot be disjoint. That is, we can't have
> +            //   multiple notes with disjoint pc ranges jumping to the same
> +            //   catch block.
> +            if (inForOfIterClose)
> +                break;

here too, this uses "break" but ProcessTryNotesBaseline uses "continue".
it would be nice to use same statement everywhere.

@@ +1191,5 @@
>  
>            case JSTRY_FINALLY:
> +            // See note above.
> +            if (inForOfIterClose)
> +                break;

and here.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6332c1ac93be
Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of. (r=arai)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6332c1ac93be

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Comment 16

[Shu-yu Guo [:shu]]

Comment on attachment 8852102 [details] [diff] [review]
Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1147371
[User impact if declined]: incorrect behavior and crashes when JS code using the Iterator protocol throws from inside of IteratorClose
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: bug fix to a yet still rarely used corner of JS
[String changes made/needed]: none

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8852102 [details] [diff] [review]
Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Fix a crash. Aurora54+ & Beta53+.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fa6fdf65c06

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Looks like this'll need a rebased patch for Beta too.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Turns out the Aurora patch was busted too. Backed out.
https://treeherder.mozilla.org/logviewer.html#?job_id=87284372&repo=mozilla-aurora

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

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Rebased for beta. Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Rebased for aurora. Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Comment 23

[Shu-yu Guo [:shu]]

What flags should I set on the rebased patches to get them landed?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d30946ae7db

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/876f92752b2c

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta for SM test failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=88482271&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=88482252&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=88482081&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/8b38e90b2ec58005e0e2ee08ec46c61a5bd3f166

Comment 27

[Shu-yu Guo [:shu]]

Attachment: Rebased for beta. Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.

Oops, mis-rebased an error handling thing that changed in the shell. The failing tests pass locally.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdb05bd2d1ce

Comment 29

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Shu-yu Guo's assessment on manual testing needs (see Comment 16) and the fact that this fix has automated coverage.