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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

Comment 1

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

This causes Assertion failure: wrapped, at /Users/skywalker/trees/mozilla-central/js/src/jscompartment.h:102 in opt builds

Comment 2

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

Attachment: stack of opt crash

Comment 3

[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/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?

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix

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.

Comment 5

[Jason Orendorff [: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.

Comment 6

[Jason Orendorff [:jorendorff]]

Please land this!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d99253b3e00

Comment 8

[Eric Faust [:efaust]]

Sorry, lost a few days last week to other things. Didn't have time to get back to this. Landed.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d99253b3e00

Comment 10

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

Can we please get this at least on mozilla-aurora as well? (bonus if for mozilla-esr45 as well)

Comment 11

[Eric Faust [:efaust]]

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.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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. :)

Comment 13

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

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).

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8726987 [details] [diff] [review]
Fix

Crash fix that was verified by an automated test case, Aurora47+

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b5ab4d96da8