1390082 - Crash [@ JS::Value::toInt32] or Assertion failure: isObject(), at dist/include/js/Value.h:642 involving async and yield

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

// Adapted from randomly chosen test: js/src/tests/test262/built-ins/Array/prototype/reduceRight/15.4.4.22-8-b-iii-1-22.js
Object.defineProperty(Array.prototype, "2", {
    set: function () {}
});
// Adapted from randomly chosen test: js/src/tests/test262/language/statements/async-generator/yield-identifier-spread-non-strict.js
async function* f() {
    yield {}
}
var x = f();
x.next();
x.next();
x.next();


Backtrace:

#0  0x000000000046e130 in JS::Value::toObject (this=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-3bfcbdf5c6c3/objdir-js/dist/include/js/Value.h:642
#1  0x0000000000af59fa in js::WrappedPtrOperations<JS::Value, JS::Rooted<JS::Value> >::toObject (this=0x7ffd67c2fd30) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-3bfcbdf5c6c3/objdir-js/dist/include/js/Value.h:1335
#2  js::AsyncGeneratorObject::peekRequest (cx=cx@entry=0x7f0283055000, asyncGenObj=..., asyncGenObj@entry=...) at js/src/vm/AsyncIteration.cpp:380
#3  0x00000000005c8fb7 in AsyncGeneratorResumeNext (cx=cx@entry=0x7f0283055000, asyncGenObj=asyncGenObj@entry=...) at js/src/builtin/Promise.cpp:2597
#4  0x00000000005c998f in js::AsyncGeneratorResolve (cx=cx@entry=0x7f0283055000, asyncGenObj=..., asyncGenObj@entry=..., value=..., value@entry=..., done=<optimized out>) at js/src/builtin/Promise.cpp:2540
#5  0x0000000000af87e7 in AsyncGeneratorReturned (value=..., asyncGenObj=..., cx=<optimized out>) at js/src/vm/AsyncIteration.cpp:416
/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)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a91dd87054d3
user:        Tooru Fujisawa
date:        Fri Aug 04 13:04:31 2017 +0900
summary:     Bug 1379525 - Part 1: Await on the value before yielding or returning inside async generator. r=shu,till

Arai-san, is bug 1379525 a likely regressor?

Comment 3

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

// Adapted from randomly chosen testcase: js/src/tests/test262/built-ins/Object/defineProperty/15.2.3.6-3-256-1.js
Object.prototype.set = function () {};
// Adapted from randomly chosen testcase: js/src/tests/test262/language/expressions/async-generator/named-yield-spread-arr-multiple.js
let f = async function* () {
    yield
};
let x = f();
x.next();
x.next().then(function () {
    x.next();
});
// Adapted from randomly chosen testcase: js/src/tests/test262/built-ins/Array/prototype/map/15.4.4.19-8-c-i-6.js
Object.defineProperty(Array.prototype, "0", {
    get: function () {
        return 1
    }
});

crashes opt shell with --fuzzing-safe --no-threads --no-baseline --no-ion at JS::Value::toInt32 with the following configuration command:

AR=ar sh ./configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This gives the same assertion failure with debug builds.

Comment 4

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

Attachment: opt crash stack

Opt crash stack for testcase in comment 3.

Comment 5

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

Setting s-s. I have a testcase that seems to access a weird memory address 0x6e6f6c6f6369:

(gdb) x/i $pc
=> 0x553e05 <FulfillMaybeWrappedPromise(JSContext*, JS::HandleObject, JS::HandleValue)+101>:	mov    (%rdi),%rax
(gdb) x/b $rdi
0x6e6f6c6f6369:	Cannot access memory at address 0x6e6f6c6f6369
(gdb)

that reduces to the testcases here.

Comment 6

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

Attachment: unreduced testcase

Also tested on an opt shell from m-c rev 3bfcbdf5c6c3 with --fuzzing-safe --no-threads --no-baseline --no-ion :

(gdb) bt
#0  FulfillMaybeWrappedPromise (cx=cx@entry=0x7ffff6952000, promiseObj=promiseObj@entry=..., value_=value_@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:655
#1  0x00000000005557df in ResolvePromiseInternal (cx=cx@entry=0x7ffff6952000, promise=promise@entry=..., resolutionVal=resolutionVal@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:430
#2  0x0000000000557f57 in js::AsyncGeneratorResolve (cx=cx@entry=0x7ffff6952000, asyncGenObj=..., asyncGenObj@entry=..., value=value@entry=..., done=done@entry=true)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2536
#3  0x000000000092736c in AsyncGeneratorReturned (value=..., asyncGenObj=..., cx=0x7ffff6952000) at /home/ubuntu/trees/mozilla-central/js/src/vm/AsyncIteration.cpp:416
#4  js::AsyncGeneratorResume (cx=cx@entry=0x7ffff6952000, asyncGenObj=asyncGenObj@entry=..., completionKind=completionKind@entry=(js::CompletionKind::Return | unknown: 76), 
    argument=..., argument@entry=...) at /home/ubuntu/trees/mozilla-central/js/src/vm/AsyncIteration.cpp:504
#5  0x0000000000557907 in AsyncGeneratorResumeNext (cx=cx@entry=0x7ffff6952000, asyncGenObj=asyncGenObj@entry=...)
    at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2666
#6  0x00000000005582a5 in js::AsyncGeneratorEnqueue (cx=0x7ffff6952000, asyncGenVal=..., completionKind=completionKind@entry=js::CompletionKind::Normal, 
    completionValue=..., result=...) at /home/ubuntu/trees/mozilla-central/js/src/builtin/Promise.cpp:2714
/snip

Comment 7

[Tooru Fujisawa [:arai]]

thanks.

the issue is that we use array_push/array_shift for internal queue object, but it's affected by monkey-patched Array prototype...
I'll re-implement the queue with better way.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Implement AsyncGeneratorQueue with simpler array operations.

So far, here's simplified implementation that does push/shift with [[Define]] instead of [[Set]], and also ignoring holes and some exceptions.
Maybe we could add some fast path like array_push/array_shift tho,
not sure if it worth adding them (not sure how much this path can become hot...)

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8897048 [details] [diff] [review]
Implement AsyncGeneratorQueue with simpler array operations.

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

Ugh, yes.

I'm fine with landing this as-is, but another option would be to take the queue handling stuff from Stream.cpp and generalize them - see {AppendTo,Peek,ShiftFrom}List.

Those should be more efficient because they use the optimized machinery jandem introduced in bug 1348772. Not sure how important that is here, but it doesn't seem entirely irrelevant.

Anyway, feel free to land this as-is, but perhaps file a follow-up bug for a more efficient implementation?

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing and letting me know about List code in Stream.cpp :)
I'll fix the patch to use them, since in that way the patch looks more like just an optimization.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Use optimized List operation for AsyncGeneratorQueue.

Moved List functions to vm/List-inl.h, and used them in AsyncIteration.cpp

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8897311 [details] [diff] [review]
Use optimized List operation for AsyncGeneratorQueue.

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

Thank you, looks great!

We could land this as an optimization as a new bug, yes. I'm not sure it's worth the hassle here: this is only on Nightly, so let's just land it, with a test case.

::: js/src/vm/List-inl.h
@@ +17,5 @@
> +#include "vm/NativeObject-inl.h"
> +
> +namespace js {
> +
> +inline static MOZ_MUST_USE NativeObject*

Nit: remove "static" here and below.

Comment 13

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b446dfdf82183eaebdee02db6e89b20218cce8
Bug 1390082 - Use optimized List operation for AsyncGeneratorQueue. r=till

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/11b446dfdf82

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.