1333946 - Crash [@ js::UnwindIteratorForUncatchableException] or Assertion failure: isObject(), at dist/include/js/Value.h:642

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 6dccae211ae5 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads):

for (var x of [0]) {
    timeout(0.001);
    for (;;) {}
}

Backtrace:

0   js-dbg-64-dm-clang-darwin-6dccae211ae5	0x0000000109dd08a9 js::jit::HandleException(js::jit::ResumeFromException*) + 7529 (Value.h:642)
1   ???                           	0x000000010bf38656 0 + 4495476310
2   ???                           	0x000000010bf38e1d 0 + 4495478301
3   js-dbg-64-dm-clang-darwin-6dccae211ae5	0x000000010a5e2c18 EnterBaseline(JSContext*, js::jit::EnterJitData&) + 696 (BaselineJIT.cpp:159)
4   js-dbg-64-dm-clang-darwin-6dccae211ae5	0x000000010a5e3469 js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) + 1305 (BaselineJIT.cpp:263)
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

The patch in bug 1332881 does not seem to fix this issue.

Comment 3

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

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)

Arai-san/Shu-yu, is bug 1147371 a likely regressor?

Comment 4

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

An unreduced testcase crashing [@ js::UnwindIteratorForUncatchableException] reduced to this testcase.

Comment 5

[Tooru Fujisawa [:arai]]

the iterator is optimized out.
sounds similar issue as bug 1331444

Comment 6

[Shu-yu Guo [:shu]]

Attachment: bug1333946.patch

As arai says, the problem is again with the iterator being optimized out.

processIterators did not consider phis that transitively hold the iterator alive, only those that directly hold the iterator alive. I don't know the historical reasons for this and why it should have ever worked. For for-of loops, whose iterators come from JSOP_CALLs or inlined function return values then used by JSOP_CHECKOBJs, we need the transitivity.

Comment 7

[Jan de Mooij [:jandem]]

Could we also fix this by removing the following else branch (so we always execute the code there, even in the def->isPhi() case):

http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/js/src/jit/IonBuilder.cpp#873

If not, I'm not sure I understand the problem.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Could we also fix this by removing the following else branch (so we always
> execute the code there, even in the def->isPhi() case):

Hm nevermind, that makes no sense because the loop after that will visit these uses.

I still don't understand the bug though. Could you give an example of an MPhi we now mark as iterator but didn't before?

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> (In reply to Jan de Mooij [:jandem] from comment #7)
> > Could we also fix this by removing the following else branch (so we always
> > execute the code there, even in the def->isPhi() case):
> 
> Hm nevermind, that makes no sense because the loop after that will visit
> these uses.
> 
> I still don't understand the bug though. Could you give an example of an
> MPhi we now mark as iterator but didn't before?

The bug as I understand it is as follows. Consider the relevant MIR nodes in this example:

- call (from the JSOP_CALLITER)
- checkisobj call
- all the phis that use the checkisobj

Since the call is emitted by JSOP_CALLITER, it's appended to the iterators_ list. The old processIterators would then do the following algorithm:

1. Look for all uses of call that were phis, then add the phis to the worklist.
2. Process the worklist until fixpoint, adding new phis reachable from the phis in the worklist.

But in this case, call isn't used by any phis. It's used by checkisobj, which is then used by phis. Since the uses of call doesn't have any phis, the original processIterators_ code wouldn't add any phis to the worklist.

My patch changes the algorithm to doing a depth-first walk of all transitive uses of the definitions in the iterators_ list. Those that are phis are marked as implicitly used.

My original confusion was why did the original algorithm suffice for the old-style iterators -- I guess because the MIR generated for old-style iterators was always an MIteratorStart that was directly used by phis.

Does that explanation make sense?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8830891 [details] [diff] [review]
bug1333946.patch

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

Ah that makes sense.

::: js/src/jit/IonBuilder.cpp
@@ +864,5 @@
>  IonBuilder::processIterators()
>  {
> +    // Find and mark phis that must transitively hold an iterator live.
> +
> +    Vector<MDefinition*, 0, SystemAllocPolicy> worklist;

Pre-existing nit: s/0/8/ or so while you're here to avoid some unnecessary malloc/realloc calls.

@@ +881,5 @@
> +            MPhi* phi = def->toPhi();
> +            phi->setIterator();
> +            phi->setImplicitlyUsedUnchecked();
> +        }
> +        

Nit: trailing whitespace

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2314298c556
Make IonBuilder::processIterators transitive. (r=jandem)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Marcia Knous [:marcia]]

Tracking 53/54+ for this crash.

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Uplift version with comments addressed. Carrying r=jandem.

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8833122 [details] [diff] [review]
Uplift version with comments addressed. Carrying r=jandem.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1147371 
[User impact if declined]: Rare crashes when using ES6 iterators with a close method in hot code
[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 only
[String changes made/needed]: None

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8833122 [details] [diff] [review]
Uplift version with comments addressed. Carrying r=jandem.

Fix a crash. Aurora53+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/022d0370a2be