Closed Bug 1251921 Opened 4 years ago Closed 4 years ago

Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:498 or Assertion failure: wrapped, at js/src/jscompartment.h:102

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- verified
firefox-esr45 --- affected

People

(Reporter: gkw, Assigned: efaust)

References

(Blocks 2 open bugs)

Details

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

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 5e0140b6d118 (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
// Adapted from randomly chosen test: js/src/jit-test/tests/basic/testBug676486.js
evalcx("\
    g = newGlobal();\
    g.parent = this;\
    g.eval(\"(\" + function() {\
        Debugger(parent).onExceptionUnwind = function(frame) frame.eval(\"\")\
    } + \")()\");\
    var f = Proxy.createFunction({}, function() {});\
    new f();\
", newGlobal())

Backtrace:

0   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100797f5b js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 539 (Interpreter.cpp:498)
1   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100798c9b InternalConstruct(JSContext*, JS::CallArgs const&) + 347 (Interpreter.cpp:557)
2   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100798a1c js::Construct(JSContext*, JS::Handle<JS::Value>, js::ConstructArgs const&, JS::Handle<JS::Value>, JS::MutableHandle<JSObject*>) + 76 (Interpreter.cpp:604)
3   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010065d511 js::CallableScriptedIndirectProxyHandler::construct(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const + 513 (ScriptedIndirectProxyHandler.cpp:516)
4   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006505b4 js::Proxy::construct(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) + 244 (Proxy.cpp:410)
5   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100651a9e js::proxy_Construct(JSContext*, unsigned int, JS::Value*) + 142 (RootingAPI.h:666)
6   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001007c4942 js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 258 (jscntxtinlines.h:236)
7   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100798c5f InternalConstruct(JSContext*, JS::CallArgs const&) + 287 (Interpreter.cpp:567)
8   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010078d465 Interpret(JSContext*, js::RunState&) + 47285 (Interpreter.cpp:594)
9   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100781ae8 js::RunScript(JSContext*, js::RunState&) + 520 (Interpreter.cpp:428)
10  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001007994fa js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 602 (Interpreter.cpp:684)
11  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100799885 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 469 (RootingAPI.h:666)
12  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100532a11 Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) + 657 (jsapi.cpp:4460)
13  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100532e24 JS::Evaluate(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JS::Value>) + 212 (jsapi.cpp:4497)
14  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100010ee1 EvalInContext(JSContext*, unsigned int, JS::Value*) + 1617 (jsapi.h:3957)
15  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100798022 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 738 (jscntxtinlines.h:236)
16  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010078d4c1 Interpret(JSContext*, js::RunState&) + 47377 (Interpreter.cpp:2802)
17  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100781ae8 js::RunScript(JSContext*, js::RunState&) + 520 (Interpreter.cpp:428)
18  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001007994fa js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 602 (Interpreter.cpp:684)
19  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100799885 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 469 (RootingAPI.h:666)
20  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100531dc1 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 417 (jsapi.cpp:4366)
21  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100532032 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 82 (RootingAPI.h:666)
22  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010001ff35 Process(JSContext*, char const*, bool, FileKind) + 3461 (js.cpp:522)
23  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100005f0d main + 11773 (js.cpp:6559)
24  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100001a14 start + 52
This causes Assertion failure: wrapped, at /Users/skywalker/trees/mozilla-central/js/src/jscompartment.h:102 in opt builds
Summary: Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:498 → Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:498 or Assertion failure: wrapped, at /Users/skywalker/trees/mozilla-central/js/src/jscompartment.h:102
Attached file stack of opt crash
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cd25dbf77e57
user:        Eric Faust
date:        Thu Oct 08 17:01:49 2015 -0700
summary:     Bug 1141863 - Part 1: Make |this| object creation account for new.target. (r=jandem, r=jorendorff)

Eric, is bug 1141863 a likely regressor?
Blocks: 1141863
Flags: needinfo?(efaustbmo)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Attached patch FixSplinter Review
Not bad.

If InterpreterFrame::prologue fails, and we have an onExceptionUnwind hook that forces a return, we run all the normal code (including a Debugger::onLeaveFrame without ever having called onEnterFrame...). If that return value was |undefined|, then we return the |this|, which was uninitialized, because the prologue never succeeded.

This patch side-doors out in this case, skipping InterpreterFrame::epilogue and Debugger::onLeaveFrame calls before passing the exception back to the calling frame.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Flags: needinfo?(efaustbmo)
Attachment #8726987 - Flags: review?(jorendorff)
Comment on attachment 8726987 [details] [diff] [review]
Fix

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

r=me with the missing tests added :-P

In addition to the fuzzer's test case, please test that this weird case still does fire debugger.onExceptionUnwind, with all frames except the frame for the constructor itself still on the stack.

I'm a little curious to know if Debugger#onEnterFrame() is called for the constructor frame, since we then skip the matching Frame#onPop() callback (fired from Debugger::onLeaveFrame). That would be sad, but I imagine we could live with it. Better than a crash, anyway...

::: js/src/vm/Interpreter.cpp
@@ +1904,5 @@
>    return_continuation:
> +    frameHalfInitialized = false;
> +
> +  prologue_return_continuation:
> +

style nit: Please remove the blank line after this label.
Attachment #8726987 - Flags: review?(jorendorff) → review+
Please land this!
Flags: needinfo?(efaustbmo)
Sorry, lost a few days last week to other things. Didn't have time to get back to this. Landed.
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/mozilla-central/rev/9d99253b3e00
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Summary: Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:498 or Assertion failure: wrapped, at /Users/skywalker/trees/mozilla-central/js/src/jscompartment.h:102 → Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:498 or Assertion failure: wrapped, at js/src/jscompartment.h:102
Can we please get this at least on mozilla-aurora as well? (bonus if for mozilla-esr45 as well)
Flags: needinfo?(efaustbmo)
Comment on attachment 8726987 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: it's been here for a long time, but only as an OOM bug. Bug 1141863 made it accessible from script.
[User impact if declined]: possible crashes. At least broken invariants.
[Describe test coverage new/current, TreeHerder]: Tests landed in 48. No problems encountered.
[Risks and why]: Changes JS interpreter logic, but low.
[String/UUID change made/needed]: None.
Flags: needinfo?(efaustbmo)
Attachment #8726987 - Flags: approval-mozilla-aurora?
Hi Gary, I noticed the whiteboard tag and was wondering why this hasn't been marked verified even though the fix landed almost 5 weeks back. In the past, such bugs would be marked verified by now. Just curious! Also, it's much safer for me to accept uplifts that have been verified on Nightly. :)
Flags: needinfo?(gary)
I think verification auto-happens only for security bugs.

Nonetheless, I verify that the testcase in comment 0 occurs with m-c rev 5e0140b6d118 but no longer happens with m-c rev 9d99253b3e00 (the landing of the fix).
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Comment on attachment 8726987 [details] [diff] [review]
Fix

Crash fix that was verified by an automated test case, Aurora47+
Attachment #8726987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #13)
> I think verification auto-happens only for security bugs.
> 
> Nonetheless, I verify that the testcase in comment 0 occurs with m-c rev
> 5e0140b6d118 but no longer happens with m-c rev 9d99253b3e00 (the landing of
> the fix).

Ok. I understand. Thanks! So do bugs like this one ever transition to verified state? I thought that should be easy to do given that it was found by an automated test case. Just curious!
Depends on: 1268526
You need to log in before you can comment on or make changes to this bug.