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

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla55
x86_64
Linux
assertion, crash, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
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>

Updated

9 months ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

9 months ago
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
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;
    }
})()
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?
Blocks: 1147371
Flags: needinfo?(shu)
(Assignee)

Comment 4

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

Also includes a light refactoring of flushPops.
Attachment #8847394 - Flags: review?(arai.unmht)
(Assignee)

Updated

9 months ago
Flags: needinfo?(shu)
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.
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)
(Assignee)

Comment 7

9 months ago
(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.
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 {
  }
}
(Assignee)

Comment 9

9 months ago
(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.
ah, makes sense :D
thank you for the detailed answer
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.
Attachment #8847394 - Flags: review?(arai.unmht) → review+

Comment 12

9 months ago
Pushed by shu@rfrn.org:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6332c1ac93be
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta approval on this when you get a chance.
Assignee: nobody → shu
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(shu)
(Assignee)

Comment 15

9 months ago
Created attachment 8852102 [details] [diff] [review]
Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.
(Assignee)

Comment 16

9 months ago
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
Flags: needinfo?(shu)
Attachment #8852102 - Flags: approval-mozilla-beta?
Attachment #8852102 - Flags: approval-mozilla-aurora?
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+.
Attachment #8852102 - Flags: approval-mozilla-beta?
Attachment #8852102 - Flags: approval-mozilla-beta+
Attachment #8852102 - Flags: approval-mozilla-aurora?
Attachment #8852102 - Flags: approval-mozilla-aurora+
Comment hidden (obsolete)
Looks like this'll need a rebased patch for Beta too.
Flags: needinfo?(shu)
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
status-firefox54: fixed → affected
(Assignee)

Comment 21

8 months ago
Created attachment 8854157 [details] [diff] [review]
Rebased for beta. Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.
(Assignee)

Comment 22

8 months ago
Created attachment 8854170 [details] [diff] [review]
Rebased for aurora. Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of.
(Assignee)

Comment 23

8 months ago
What flags should I set on the rebased patches to get them landed?
Flags: needinfo?(shu)
Comment hidden (obsolete)

Comment 25

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/876f92752b2c
status-firefox54: affected → fixed
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
status-firefox53: fixed → affected
Flags: needinfo?(shu)
(Assignee)

Comment 27

8 months ago
Created attachment 8854287 [details] [diff] [review]
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.
(Assignee)

Updated

8 months ago
Attachment #8854157 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Flags: needinfo?(shu)

Comment 28

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cdb05bd2d1ce
status-firefox53: affected → fixed
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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.