Closed Bug 1333946 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 + fixed
firefox54 + fixed

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, jsbugmon, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(3 files)

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.
The patch in bug 1332881 does not seem to fix this issue.
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?
Blocks: 1147371
Crash Signature: [@ js::UnwindIteratorForUncatchableException]
Flags: needinfo?(shu)
Flags: needinfo?(arai.unmht)
Summary: Assertion failure: isObject(), at dist/include/js/Value.h:642 → Crash [@ js::UnwindIteratorForUncatchableException] or Assertion failure: isObject(), at dist/include/js/Value.h:642
An unreduced testcase crashing [@ js::UnwindIteratorForUncatchableException] reduced to this testcase.
the iterator is optimized out.
sounds similar issue as bug 1331444
See Also: → 1331444
Attached patch bug1333946.patchSplinter Review
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.
Assignee: nobody → shu
Status: NEW → ASSIGNED
Flags: needinfo?(shu)
Flags: needinfo?(arai.unmht)
Attachment #8830891 - Flags: review?(jdemooij)
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.
Flags: needinfo?(shu)
(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?
(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?
Flags: needinfo?(shu)
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
Attachment #8830891 - Flags: review?(jdemooij) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2314298c556
Make IonBuilder::processIterators transitive. (r=jandem)
https://hg.mozilla.org/mozilla-central/rev/c2314298c556
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Tracking 53/54+ for this crash.
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
Attachment #8833122 - Flags: approval-mozilla-aurora?
Comment on attachment 8833122 [details] [diff] [review]
Uplift version with comments addressed. Carrying r=jandem.

Fix a crash. Aurora53+.
Attachment #8833122 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1341339
You need to log in before you can comment on or make changes to this bug.