Closed
Bug 1346862
Opened 7 years ago
Closed 7 years ago
Crash [@ js::UnwindEnvironment] or Assertion failure: !done(), at vm/Scope.h:1363
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
People
(Reporter: decoder, Assigned: shu)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(4 files, 1 obsolete file)
14.40 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
14.41 KB,
patch
|
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
14.61 KB,
patch
|
Details | Diff | Splinter Review | |
14.61 KB,
patch
|
Details | Diff | Splinter Review |
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•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•7 years 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•7 years ago
|
||
Also includes a light refactoring of flushPops.
Attachment #8847394 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(shu)
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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.
Comment 8•7 years ago
|
||
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•7 years 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.
Comment 10•7 years ago
|
||
ah, makes sense :D thank you for the detailed answer
Comment 11•7 years ago
|
||
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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6332c1ac93be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 14•7 years ago
|
||
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•7 years ago
|
||
Assignee | ||
Comment 16•7 years 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 17•7 years ago
|
||
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) |
Comment 20•7 years ago
|
||
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
Assignee | ||
Comment 21•7 years ago
|
||
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Comment 23•7 years ago
|
||
What flags should I set on the rebased patches to get them landed?
Flags: needinfo?(shu)
Comment hidden (obsolete) |
Comment 25•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/876f92752b2c
Comment 26•7 years ago
|
||
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
Flags: needinfo?(shu)
Assignee | ||
Comment 27•7 years ago
|
||
Oops, mis-rebased an error handling thing that changed in the shell. The failing tests pass locally.
Assignee | ||
Updated•7 years ago
|
Attachment #8854157 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(shu)
Comment 28•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/cdb05bd2d1ce
Comment 29•7 years ago
|
||
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.
Description
•