1332881 - Crash [@ js::CloseIterator] 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 d5343b0f7e6a (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

(function () {
    for (var x in [0]) {
        try {} finally {
            return 0;
        }
    }
})();

Backtrace:

0   js-dbg-64-dm-clang-darwin-d5343b0f7e6a	0x000000010fbbe87f Interpret(JSContext*, js::RunState&) + 27823 (Value.h:642)
1   js-dbg-64-dm-clang-darwin-d5343b0f7e6a	0x000000010fbb79a2 js::RunScript(JSContext*, js::RunState&) + 482 (Interpreter.cpp:406)
2   js-dbg-64-dm-clang-darwin-d5343b0f7e6a	0x000000010fbc95de js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 942 (Interpreter.cpp:687)
3   js-dbg-64-dm-clang-darwin-d5343b0f7e6a	0x000000010fbc9937 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 439 (RootingAPI.h:795)
/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)]

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?

Comment 3

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

We also see crashes [@ js::CloseIterator] which reduce to this testcase variant.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Handle stack value in correct order when leaving loop and try-finally.

overlooked that npops should be resolved before handling other stack values.
added flushPops for StatementKind::ForOfLoop and StatementKind::ForInLoop cases in NonLocalExitControl::prepareForNonLocalJump.

Comment 5

[Tooru Fujisawa [:arai]]

maybe we could do some more cleanup to merge those JSOP_POPs.
with dedicated emitPopn method that receives the number of values to pop, and emit better opcode sequence for the number.
(it becomes more change so might be better doing in separated bug, to make it easier to uplift this)

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8830203 [details] [diff] [review]
Handle stack value in correct order when leaving loop and try-finally.

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

Doh, thank you very much for taking the bug and fixing it, arai.

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0c4806133ae96c62f4ecc47229d19887934b70
Bug 1332881 - Handle stack value in correct order when leaving loop and try-finally. r=shu

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8830203 [details] [diff] [review]
Handle stack value in correct order when leaving loop and try-finally.

Approval Request Comment
> [Feature/Bug causing the regression]
bug 1147371

> [User impact if declined]
possible crash by executing JavaScript

> [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?]
less risky

> [Why is the change risky/not risky?]
this patch changes the target stack offset of operations.
the testcase confirms the offset is correct in most case.
even if this patch is still wrong in some corner case, it won't cause any issue worse than unpatched, that tries to dereference random value.

> [String changes made/needed]
none

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8830203 [details] [diff] [review]
Handle stack value in correct order when leaving loop and try-finally.

Sounds like a regression in 53, let's take this crash fix for aurora.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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