1334314 - Crash [@ js::ToBooleanSlow] or Assertion failure: v.isObject(), at js/src/jsbool.cpp:175

Status: RESOLVED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

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

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1106164.js
var g = newGlobal();
g.parent = this;
g.eval("new Debugger(parent).onExceptionUnwind = function () { };");
// jsfunfuzz-generated
function f() {
    [[]] = [];
}
try {
    f();
} catch (e) {};
try {
    f();
} catch (e) {};
f();


Backtrace:

0   js-dbg-64-dm-clang-darwin-d92fd6b6d6bf	0x000000010db38384 js::ToBooleanSlow(JS::Handle<JS::Value>) + 196 (jsbool.cpp:175)
1   js-dbg-64-dm-clang-darwin-d92fd6b6d6bf	0x000000010d8d44f9 js::jit::HandleException(js::jit::ResumeFromException*) + 2761 (Conversions.h:125)
2   ???                           	0x000000010fa38656 0 + 4557342294
3   js-dbg-64-dm-clang-darwin-d92fd6b6d6bf	0x000000010d84b8da js::jit::IonCannon(JSContext*, js::RunState&) + 874 (Ion.cpp:2901)
4   js-dbg-64-dm-clang-darwin-d92fd6b6d6bf	0x000000010d6e04da js::RunScript(JSContext*, js::RunState&) + 378 (Interpreter.cpp:386)
/snip

For detailed crash information, see attachment.

Setting s-s because this assertion (and an upcoming crash signature) has been known to be the source of s-s issues in the past.

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Created attachment 8830956 [details]
Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

Created attachment 8830957 [details]
Opt stack for testcase

Comment 3

[Gary Kwong [:gkw] [:nth10sd]]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0d0db76e4a87
user:        Shu-yu Guo
date:        Wed Jan 18 18:33:45 2017 -0800
summary:     Bug 1331585 - Allow falsy "done" values for IteratorClose due to exception during array destructuring. (r=arai)

Shu-yu, is bug 1331585 a likely regressor?

Comment 4

[Gary Kwong [:gkw] [:nth10sd]]

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

Comment 5

[Shu-yu Guo [:shu]]

Okay, so for destructuring, instead of just the iterator on the stack, we also have the .done value on the stack. This bug is now Ion optimizing out the .done value.

Unlike the iterator value that's pushed onto the stack by a JSOP_CALLITER, the .done value is pushed and popped as we go through the destructuring code. This is much harder to do with IonBuilder's iterators_ mechanism.

Jan, I think we have two realistic options and a bad 3rd option:

1. Introduce a NOP_OBSERVABLE bytecode hint; rename the transitive iterators_ mechanism to observables_; top-of-the-stack SSA values are appended to observables_.

2. Remove the iterators_ mechanism and implement a general mechanism that searches trynotes to figure out observable stack slots. This might be slow.

3. Don't optimize out any phis when there's use of iterators, like when there's a try. I don't want to do this because I'm sure for-of and destructuring are already slow enough and I don't want to make it worse.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #5)
> Okay, so for destructuring, instead of just the iterator on the stack, we
> also have the .done value on the stack. This bug is now Ion optimizing out
> the .done value.
> 
> Unlike the iterator value that's pushed onto the stack by a JSOP_CALLITER,
> the .done value is pushed and popped as we go through the destructuring
> code. This is much harder to do with IonBuilder's iterators_ mechanism.
> 
> Jan, I think we have two realistic options and a bad 3rd option:
> 
> 1. Introduce a NOP_OBSERVABLE bytecode hint; rename the transitive
> iterators_ mechanism to observables_; top-of-the-stack SSA values are
> appended to observables_.
> 
> 2. Remove the iterators_ mechanism and implement a general mechanism that
> searches trynotes to figure out observable stack slots. This might be slow.
> 
> 3. Don't optimize out any phis when there's use of iterators, like when
> there's a try. I don't want to do this because I'm sure for-of and
> destructuring are already slow enough and I don't want to make it worse.

Actually I don't know now if my analysis is correct. I tried approach 1 just now and it didn't fix it. Not sure why it's being optimized out now.

Comment 7

[Shu-yu Guo [:shu]]

Created attachment 8831007 [details] [diff] [review]
Fix debug mode OSR exception handling for IteratorClose trynotes.

Oops, turned out the bug wasn't in Ion but in the baseline bailout special
casing of debug mode OSR for exception unwinding.

Comment 8

[Shu-yu Guo [:shu]]

Also why is this bug s-s?

Comment 9

[Gary Kwong [:gkw] [:nth10sd]]

(In reply to Shu-yu Guo [:shu] from comment #8)
> Also why is this bug s-s?

This is why, from comment 0:

> Setting s-s because this assertion (and an upcoming crash signature) has
> been known to be the source of s-s issues in the past.

Please feel free to open up this one, or other bugs in the future if they're not s-s. We tend to err on the side of caution.

Comment 10

[Shu-yu Guo [:shu]]

This is not s-s. At worst it'll dereference a small int (14, corresponding to JS_OPTIMIZED_OUT).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bf3889601e
Fix debug mode OSR exception handling for IteratorClose trynotes. (r=jandem)

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8831007 [details] [diff] [review]
Fix debug mode OSR exception handling for IteratorClose trynotes.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1331585 
[User impact if declined]: possible rare crashes in JS debugger
[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 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Liz Henry (:lizzard) (needinfo? me)]

Comment on attachment 8831007 [details] [diff] [review]
Fix debug mode OSR exception handling for IteratorClose trynotes.

Crash fix, we're in early aurora, let's take it.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4e9d4072936