1357075 - 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 c697e756f738 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

var iterable = {};
var iterator = {
  return: 1
};
iterable[Symbol.iterator] = function() {
  return iterator;
};
for ([ class get {} ().iterator ] of [iterable]) {}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::UnwindEnvironment (cx=cx@entry=0x7ffff6926800, ei=..., pc=<optimized out>) at js/src/vm/Interpreter.cpp:1054
#0  js::UnwindEnvironment (cx=cx@entry=0x7ffff6926800, ei=..., pc=<optimized out>) at js/src/vm/Interpreter.cpp:1054
#1  0x00000000004eec6b in SettleOnTryNote (cx=0x7ffff6926800, tn=0x7ffff69e8908, ei=..., regs=...) at js/src/vm/Interpreter.cpp:1111
#2  0x00000000004fe557 in ProcessTryNotes (regs=..., ei=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:1234
#3  HandleError (regs=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:1315
#4  Interpret (cx=0x7ffff6926800, state=...) at js/src/vm/Interpreter.cpp:4273
[...]
#14 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8683
rax	0x2	2
rbx	0x7fffffffd260	140737488343648
rcx	0x7ffff69e88e0	140737330972896
rdx	0x0	0
rsi	0x1afea00	28305920
rdi	0x7fffffffd298	140737488343704
rbp	0x7ffff6926800	140737330178048
rsp	0x7fffffffcdb0	140737488342448
r8	0x7ffff69e8968	140737330973032
r9	0x7ffff69e88d0	140737330972880
r10	0x7ffff69e88a0	140737330972832
r11	0x0	0
r12	0xe36790	14903184
r13	0x7fffffffffff	140737488355327
r14	0x7fffffffd278	140737488343672
r15	0x7fffffffcdb0	140737488342448
rip	0x4eea15 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+197>
=> 0x4eea15 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+197>:	cmpb   $0xe,(%rdx)
   0x4eea18 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+200>:	ja     0x4eea51 <js::UnwindEnvironment(JSContext*, js::EnvironmentIter&, unsigned char*)+257>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
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)

This iteration took 0.344 seconds to run.

Comment 2

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

Shu-yu, is bug 1147371 a likely regressor?

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Pad a nop to unwind to the scope just before a destructuring iterator close trynote.

This is kind of gross. What do you think, arai?

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8865048 [details] [diff] [review]
Pad a nop to unwind to the scope just before a destructuring iterator close trynote.

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

so, the issue is specific to JSTRY_DESTRUCTURING_ITERCLOSE that may have arbitrary opcode (with/without scope) at the try note, right?

  note                          | opcode at trynote->start
 -------------------------------+--------------------------------------
  JSTRY_CATCH                   | after JSOP_TRY (handled in UnwindEnvironmentToTryPc)
  JSTRY_FINALLY                 | after JSOP_TRY (handled in UnwindEnvironmentToTryPc)
  JSTRY_FOR_OF_ITERCLOSE        | JSOP_DUP at the beginning of emitIteratorClose
  JSTRY_DESTRUCTURING_ITERCLOSE | anything (the beginning of emitter for wrapWithDestructuringIteratorCloseTryNote)
  JSTRY_FOR_OF                  | JSOP_LOOPHEAD
  JSTRY_FOR_IN                  | JSOP_LOOPHEAD
  JSTRY_LOOP                    | JSOP_LOOPHEAD

in that case, I'd prefer having dedicated opcode for it (JSOP_NOP_{somthing}), instead of just JSOP_NOP, so that it's clear what it is for, when looking bytecode dump.
maybe better padding the opcode also to JSTRY_FOR_OF_ITERCLOSE, for consistency, but not sure if it worth the additional bytecode size...

anyway, the approach in this patch looks good :)
r+ with or without adding dedicated opcode.

also, please add the testcase.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> Comment on attachment 8865048 [details] [diff] [review]
> Pad a nop to unwind to the scope just before a destructuring iterator close
> trynote.
> 
> Review of attachment 8865048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> so, the issue is specific to JSTRY_DESTRUCTURING_ITERCLOSE that may have
> arbitrary opcode (with/without scope) at the try note, right?
> 
>   note                          | opcode at trynote->start
>  -------------------------------+--------------------------------------
>   JSTRY_CATCH                   | after JSOP_TRY (handled in
> UnwindEnvironmentToTryPc)
>   JSTRY_FINALLY                 | after JSOP_TRY (handled in
> UnwindEnvironmentToTryPc)
>   JSTRY_FOR_OF_ITERCLOSE        | JSOP_DUP at the beginning of
> emitIteratorClose
>   JSTRY_DESTRUCTURING_ITERCLOSE | anything (the beginning of emitter for
> wrapWithDestructuringIteratorCloseTryNote)
>   JSTRY_FOR_OF                  | JSOP_LOOPHEAD
>   JSTRY_FOR_IN                  | JSOP_LOOPHEAD
>   JSTRY_LOOP                    | JSOP_LOOPHEAD
> 

Right. More specifically, this only comes up when the trynote supports "settling" on it, i.e., resuming execution at the end of the pc range covered by the note. When we "settle" on a trynote, the environment chain is unwound to what it was at the beginning of the pc range covered by the trynote. For CATCH and FINALLY, we unwind just before the try. The only loop trynote we might settle on is FOR_IN, and the bytecode we generate for that so happens to never exhibit this problem. DESTRUCTURING_ITERCLOSE was buggy because anything can be emitted for it.

> in that case, I'd prefer having dedicated opcode for it
> (JSOP_NOP_{somthing}), instead of just JSOP_NOP, so that it's clear what it
> is for, when looking bytecode dump.

Sure, I'll make a new kind of NOP.

> maybe better padding the opcode also to JSTRY_FOR_OF_ITERCLOSE, for
> consistency, but not sure if it worth the additional bytecode size...

FOR_OF_ITERCLOSE trynotes can never be settled on, so we don't need it.

> 
> anyway, the approach in this patch looks good :)
> r+ with or without adding dedicated opcode.
> 
> also, please add the testcase.

Will do.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8756a6262190
Pad a nop to unwind to the scope just before a destructuring iterator close trynote. (r=arai)

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee1367bc91b
Backed out changeset 8756a6262190

Comment 8

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=11f3f002c7f4b8bc55678ddf9a902d8d36e1015b for causing https://treeherder.mozilla.org/logviewer.html#?job_id=97937052&repo=mozilla-inbound that started at least with this push. Not sure which of 3 changes cause it, so backing it out

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42708872146
Pad a nop to unwind to the scope just before a destructuring iterator close trynote. (r=arai)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d42708872146

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance, Shu.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Pad a nop to unwind to the scope just before a destructuring iterator close trynote. Rebased for beta.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8867947 [details] [diff] [review]
Pad a nop to unwind to the scope just before a destructuring iterator close trynote. Rebased for beta.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1147371
[User impact if declined]: crashes in DEBUG, incorrect behavior in non-DEBUG builds when throwing from inside IteratorClose due to destructuring where one of the destructuring items is a class expression
[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?]: No
[Why is the change risky/not risky?]: Bug fix
[String changes made/needed]:

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8867947 [details] [diff] [review]
Pad a nop to unwind to the scope just before a destructuring iterator close trynote. Rebased for beta.

Fix a crash. Beta54+. Should be in 54 beta 9.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Shu-yu Guo [:shu] from comment #13)
> [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

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