Closed Bug 1317824 Opened 3 years ago Closed 3 years ago

Assertion failure: isObject(), at dist/include/js/Value.h:657

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gkw, Assigned: arai)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

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

Attachments

(2 files, 1 obsolete file)

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

// Adapted from randomly chosen test: js/src/jit-test/tests/modules/bug-1233915.js
g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    Debugger(parent).onExceptionUnwind = function(frame) frame.eval("");
} + ")()");
// Adapted from randomly chosen test: js/src/jit-test/tests/xdr/async-lazy.js
async function f() {
    e;
}
f();


Backtrace:

0   js-dbg-64-dm-clang-darwin-71fd23fa0803	0x00000001101f15b9 AsyncFunctionResume(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, ResumeKind, JS::Handle<JS::Value>) + 1337 (Value.h:657)
1   js-dbg-64-dm-clang-darwin-71fd23fa0803	0x00000001101f0baa WrappedAsyncFunction(JSContext*, unsigned int, JS::Value*) + 1178 (AsyncFunction.cpp:82)
2   js-dbg-64-dm-clang-darwin-71fd23fa0803	0x000000011031d55e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 94 (jscntxtinlines.h:240)
3   js-dbg-64-dm-clang-darwin-71fd23fa0803	0x000000011031d174 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 548 (Interpreter.cpp:447)
4   js-dbg-64-dm-clang-darwin-71fd23fa0803	0x0000000110314f84 Interpret(JSContext*, js::RunState&) + 36372 (Interpreter.cpp:2922)
/snip

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9ced693a9b87
user:        Tooru Fujisawa
date:        Wed Nov 09 03:27:49 2016 +0900
summary:     Bug 1314055 - Part 1: Port async/await implementation from self-hosted JS to C++. r=till

Arai-san, is bug 1314055 a likely regressor?
Blocks: 1314055
Flags: needinfo?(arai.unmht)
Yes, we should check the return value of generator, that can be altered by debugger.
Flags: needinfo?(arai.unmht)
I think it would make sense to fix this by extending the checks for unacceptable resumption values in Debugger.cpp:CheckResumptionValue to reject return values that the callee could never produce on its own.
Added check to CheckResumptionValue.
it checks the value when returning from generator or async.

CheckStarGeneratorResumptionValue checks if the value is an object with `done` and `value` data property, and `done` is a boolean.
at first I was about to allow `data` and `value` as getter, but it may change the shape while/after checking it, so disallowed it.

CheckAsyncResumptionValue calls CheckStarGeneratorResumptionValue, since currently it's using the same format as normal generator, but we might change the structure in future, as an optimization, so I added it separately.
Attachment #8811143 - Flags: review?(jimb)
Comment on attachment 8811143 [details] [diff] [review]
Check debugger resumption value of generator and async function.

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

This restriction on resumption values needs to be documented in js/src/doc/Debugger. Something like:

    If the frame is a generator or async function, then <i>value</i> must
    conform to the iterator protocol: it must be a non-proxy object of the form
    <code>{ done: <i>boolean</i>, value: <i>v</i> }</code>, where
    both `done` and `value` are ordinary properties.

in Conventions.md in the section for { return: blah } resumption values.

The tests are superb.

Rather than using the "|jit-test| error:TypeError" cookie, you could use the assertThrowsInstanceOf testing function (see js/src/jit-test/tests/debug/breakpoint-02.js for an example) and cover all the different combinations of correct and faulty resumption value, generator and async function, in less test code. Having so many test files that are almost identical is a bit ugly.

I notice that you don't cover the case where the return value is a proxy; you do want to exclude that as well, right?

::: js/src/vm/Debugger.cpp
@@ +1560,5 @@
>                       JSTrapStatus status, MutableHandleValue vp)
>  {
> +    if (status == JSTRAP_RETURN && frame && frame.isFunctionFrame()) {
> +        // Prevent altering generator's yield/return value or async function's
> +        // await value to unknown shape.

It took me a few tries to parse this comment. I would phrase it:

Don't let a { return: ... } resumption value make a generator or async function violate the iterator protocol. The return value from such a frame must have the form { done: <bool>, value: <anything> }.

::: js/src/vm/GeneratorObject.cpp
@@ +337,5 @@
>      return true;
>  }
> +
> +MOZ_MUST_USE bool
> +js::CheckStarGeneratorResumptionValue(JSContext* cx, HandleValue v, bool& result)

I'm not familiar with the *Pure functions, but it looks like they cannot raise an exception, and they return false only when carrying out the operation might have side effects. Is that correct?

It looks like this function always returns true, and `result` holds the real information. Would it make sense for the function to simply return what this code calls `result`, and be infallible?
Attachment #8811143 - Flags: review?(jimb) → review+
Depends on: 1317908
(In reply to Jim Blandy :jimb from comment #8)
> Rather than using the "|jit-test| error:TypeError" cookie, you could use the
> assertThrowsInstanceOf testing function (see
> js/src/jit-test/tests/debug/breakpoint-02.js for an example) and cover all
> the different combinations of correct and faulty resumption value, generator
> and async function, in less test code. Having so many test files that are
> almost identical is a bit ugly.

Since the exception is thrown by debugger itself, the exception is not catch-able by try-catch.
so I set uncaughtExceptionHook in the test to remember the exception, and continue executing with special return value.

now the test files are merged into 2 files for async and generator, and each single testcase checks uncaught exception and return value.

Because of above changes, I'd like to ask review again.


> I notice that you don't cover the case where the return value is a proxy;
> you do want to exclude that as well, right?

Added the case too.


> ::: js/src/vm/GeneratorObject.cpp
> @@ +337,5 @@
> >      return true;
> >  }
> > +
> > +MOZ_MUST_USE bool
> > +js::CheckStarGeneratorResumptionValue(JSContext* cx, HandleValue v, bool& result)
> 
> I'm not familiar with the *Pure functions, but it looks like they cannot
> raise an exception, and they return false only when carrying out the
> operation might have side effects. Is that correct?

Yes


> It looks like this function always returns true, and `result` holds the real
> information. Would it make sense for the function to simply return what this
> code calls `result`, and be infallible?

Removed `bool& result`.
Attachment #8811143 - Attachment is obsolete: true
Attachment #8811446 - Flags: review?(jimb)
Comment on attachment 8811446 [details] [diff] [review]
Check debugger resumption value of generator and async function.

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

Looks great. Thank you for the revisions!
Attachment #8811446 - Flags: review?(jimb) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda7c004b71e63bc09fbe362a6c32e291b3781c2
Bug 1317824 - Check debugger resumption value of generator and async function. r=jimb
https://hg.mozilla.org/mozilla-central/rev/cda7c004b71e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8811446 [details] [diff] [review]
Check debugger resumption value of generator and async function.

same patch is applicable to mozilla-aurora

Approval Request Comment
> [Feature/regressing bug #]
Bug 1314055

> [User impact if declined]
Possible crash in Devtools Debugger or Add-on

> [Describe test coverage new/current, TreeHerder]
Tested on m-c.

> [Risks and why]
Low.  Debugger is not exposed to web content, so the affected range is browser chrome only.
This change rejects unexpected value returned by debugger, that could happen only if it's done explicitly.

> [String/UUID change made/needed]
None
Attachment #8811446 - Flags: approval-mozilla-aurora?
Comment on attachment 8811446 [details] [diff] [review]
Check debugger resumption value of generator and async function.

fix recent assertion failure in aurora52
Attachment #8811446 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.