1232685 - Crash [@ InternalConstruct] or Assertion failure: data.maxArgv[0].isObject(), at js/src/jit/BaselineJIT.cpp:149 or Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:495 with ES6 Classes and Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 749f9328dd76 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --baseline-eager):

newGlobal().Debugger(this).onExceptionUnwind = function() {
    return {
        return: 1
    }
}
function base() {}
class beforeThrow extends base {
    constructor() {}
}
new beforeThrow;



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0823f657 in EnterBaseline (cx=cx@entry=0xf7a73020, data=...) at js/src/jit/BaselineJIT.cpp:149
#0  0x0823f657 in EnterBaseline (cx=cx@entry=0xf7a73020, data=...) at js/src/jit/BaselineJIT.cpp:149
#1  0x08247865 in js::jit::EnterBaselineMethod (cx=cx@entry=0xf7a73020, state=...) at js/src/jit/BaselineJIT.cpp:172
#2  0x086f18e8 in Interpret (cx=cx@entry=0xf7a73020, state=...) at js/src/vm/Interpreter.cpp:2811
#3  0x086f5755 in js::RunScript (cx=cx@entry=0xf7a73020, state=...) at js/src/vm/Interpreter.cpp:391
#4  0x086fad99 in js::ExecuteKernel (cx=cx@entry=0xf7a73020, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:650
#5  0x086fb007 in js::Execute (cx=cx@entry=0xf7a73020, script=script@entry=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:685
#6  0x0852dfbc in ExecuteScript (cx=cx@entry=0xf7a73020, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4410
#7  0x0852e196 in JS_ExecuteScript (cx=cx@entry=0xf7a73020, scriptArg=scriptArg@entry=...) at js/src/jsapi.cpp:4443
#8  0x0806cabf in RunFile (compileOnly=false, file=<optimized out>, filename=0xffffd022 "min.js", cx=0xf7a73020) at js/src/shell/js.cpp:515
#9  Process (cx=cx@entry=0xf7a73020, filename=0xffffd022 "min.js", forceTTY=forceTTY@entry=false, kind=kind@entry=FileScript) at js/src/shell/js.cpp:728
#10 0x080db166 in ProcessArgs (op=0xffffcca0, cx=0xf7a73020) at js/src/shell/js.cpp:6204
#11 Shell (envp=<optimized out>, op=0xffffcca0, cx=0xf7a73020) at js/src/shell/js.cpp:6516
#12 main (argc=6, argv=0xffffcdf4, envp=0xffffce10) at js/src/shell/js.cpp:6877
eax	0x0	0
ebx	0x9801430	159388720
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0xffffc270	-15760
edi	0xf7a73020	-140038112
ebp	0xffffc208	4294951432
esp	0xffffc050	4294950992
eip	0x823f657 <EnterBaseline(JSContext*, js::jit::EnterJitData&)+1287>
=> 0x823f657 <EnterBaseline(JSContext*, js::jit::EnterJitData&)+1287>:	movl   $0x95,0x0
   0x823f661 <EnterBaseline(JSContext*, js::jit::EnterJitData&)+1297>:	call   0x80ffb70 <abort()>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a59b5b0139b4
user:        Eric Faust
date:        Thu Oct 08 17:01:48 2015 -0700
summary:     Bug 1169740 - Implement a TDZ-like behavior for |this| in derived class constructors. (r=jandem, r=jorendorff, inputs on nit resoulution from Waldo)

This iteration took 371.053 seconds to run.

Comment 2

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

Eric, is bug 1169740 a likely regressor?

Comment 3

[Eric Faust [:efaust]]

Yeeeeep. I had tried to ignore the Debugger interactions. No dice. I'm on it.

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix

Comment 6

[Shu-yu Guo [:shu]]

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

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

C++ stuff looks good to me modulo some renaming.

I'd like to see the new test.

Please also update the ## Resumption Values docs in Debugger/Conventions.md with a note about this behavior, since the Debugger API is like, the shining example of in-tree docs.

::: js/src/jit-test/tests/debug/bug1232685.js
@@ +7,5 @@
> +function base() {}
> +class beforeThrow extends base {
> +    constructor() {}
> +}
> +new beforeThrow;

Could you expand this test?

1) A better comment of the expected behavior here.
2) Rename to something like class-01.js or something
3) Test that all other hooks that get a frame are correctly handled. So, onEnterFrame, onLeaveFrame, breakpoints, etc.

::: js/src/vm/Debugger.cpp
@@ +661,5 @@
>                      status = dbg->handleUncaughtException(ac, false);
>                      break;
>                  }
>  
> +                // Calling the callback can cause frame invalidation, so save this off, first.

"invalidation" isn't the correct terminology here since we use it to refer to JIT invalidation. Say something like

"Calling into debugger hooks can result in the frame no longer being a debuggee, making it unusable. Check the return value now."

@@ +662,5 @@
>                      break;
>                  }
>  
> +                // Calling the callback can cause frame invalidation, so save this off, first.
> +                bool check = CheckReturnFromFrame(frameobj);

Nit: spell out checkPrimitiveReturn

@@ +1076,5 @@
>  
>  JSTrapStatus
>  Debugger::handleUncaughtException(Maybe<AutoCompartment>& ac, bool callHook)
>  {
> +    return handleUncaughtExceptionHelper(ac, nullptr, callHook, /* checkPrimitiveReturn = */false);

Nit: space after */

@@ +1636,5 @@
>          const Value& handler = frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER);
>          RootedValue rval(cx);
> +
> +        // Save this off. The hook might invalidate the frame.
> +        bool check = CheckReturnFromFrame(frame);

Ditto on use of "invalidate" and spelling on checkPrimitiveReturn

::: js/src/vm/Debugger.h
@@ +507,5 @@
>       *     anything else - Make a new TypeError the pending exception and
>       *         return handleUncaughtException(ac, vp, callHook).
>       */
>      JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool ok, const Value& rv,
> +                                      bool checkPrimitiveReturn, MutableHandleValue vp, bool callHook = true);

If you are so inclined a 2-value enum instead of a bool would be nice.

::: js/src/vm/Stack.h
@@ +219,5 @@
>      inline Value& thisArgument() const;
>  
>      inline Value newTarget() const;
>  
> +    inline bool debuggerCheckPrimitiveReturn() const;

Maybe name this debuggerNeedsCheckPrimitiveReturn?

Comment 7

[Jan de Mooij [:jandem]]

What's the status of this? Fuzzers are still hitting this more than once a day.

Comment 8

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

Can this patch please be updated and landed soon?

Comment 10

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

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug-1204863.js */
var dbg = newGlobal().Debugger(this);
dbg.onExceptionUnwind = function() {
    return {
        return: ""
    };
};
// Adapted from randomly chosen test: js/src/jit-test/tests/basic/bug602088.js
var p = Proxy.createFunction({}, function() {});
new p();

crashes js opt shell on m-c rev f53533d9eb77 with --fuzzing-safe --no-threads --no-baseline --ion-eager at InternalConstruct and asserts debug shell at:

Assertion failure: args.rval().isObject(), at js/src/vm/Interpreter.cpp:495

Debug configure command:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r f53533d9eb77

Comment 12

[Christian Holler (:decoder)]

Marking as fuzzblocker, please land asap. This bug causes many different signatures, some of them look like s-s bugs.

Comment 14

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

Can someone please fix up the patch according to the review and land this? Setting needinfo? from interpreter & debugger folks.

It's been 2+ months with lots of comments by least 3 people asking for fixup & landing that it isn't funny anymore. (not to mention the dupes we're hitting all the time)

Comment 15

[Jim Blandy :jimb]

Gary, I understand your frustration, but just needinfo'ing random people isn't the way to make progress here. Eric has the patch, which Shu has made some minor comments on. If Eric doesn't have time to carry this through to a landing, then perhaps he can suggest someone else.

But these are all issues that can be dealt with IRC, or a well-pointed needinfo, and if people aren't reachable that way, then you can talk to their manager to see what's wrong.

Comment 16

[Eric Faust [:efaust]]

Attachment: Fix.

I'm here. I got it. I'm done with the patch. Let's everyone calm down.

I did indeed get distracted, and then was busy after Jan needinfo'd me again. My apologies for the long delay.

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8720537 [details] [diff] [review]
Fix.

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

Per our convo on IRC, you don't need the GetAFPForFrame helper, as you should already have the AFP you need in all the hooks. Further, the pc argument to GetThisValueForDebuggerMaybeOptimizedOut shouldn't be necessary. This should all simplify the patch.

::: js/src/vm/Debugger.cpp
@@ +679,5 @@
>              Debugger* dbg = Debugger::fromChildJSObject(frameobj);
>  
>              if (dbg->enabled &&
> +                !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined())
> +            {

Thanks for style fix.

@@ +694,5 @@
>  
> +                /**
> +                 * Calling into debugger hooks can result in the frame no
> +                 * longer being a debuggee, making it unusable. Check the
> +                 * return value now.

To be pedantic, you're stashing the AFP, but still checking the return value later. Sorry if I gave you bad wording in the previous review.

@@ +1260,5 @@
> +    }
> +
> +    if (status == JSTRAP_RETURN && thisVForCheck.isSome() && v.isPrimitive()) {
> +        if (v.isUndefined()) {
> +            if (thisVForCheck.ref().isMagic(JS_UNINITIALIZED_LEXICAL)) {

OPTIMIZED_OUT can flow into here via GetThisValueForDebuggerMaybeOptimizedOut. Not sure what kind of error we should report for it, probably just the generic bad derived return.

Also, you can do (*thisVForCheck) instead of ref(), I think that's the preferred style.

@@ +1313,5 @@
> +        if (!success || !cx->compartment()->wrap(cx, &rootThis)) {
> +            ac.reset();
> +            return JSTRAP_ERROR;
> +        }
> +        MOZ_ASSERT_IF(rootThis.isMagic(), rootThis.isMagic(JS_UNINITIALIZED_LEXICAL));

It could also be JS_OPTIMIZED_OUT.

Comment 18

[Eric Faust [:efaust]]

Attachment: Reworked, per IRC discussion

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8720623 [details] [diff] [review]
Reworked, per IRC discussion

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

r=me with JS_OPTIMIZED_OUT handled.

::: js/src/vm/Debugger.cpp
@@ +1230,5 @@
> +    }
> +
> +    if (status == JSTRAP_RETURN && thisVForCheck.isSome() && v.isPrimitive()) {
> +        if (v.isUndefined()) {
> +            if (thisVForCheck.ref().isMagic(JS_UNINITIALIZED_LEXICAL)) {

Reminder to handle JS_OPTIMIZED_OUT.

@@ +1269,5 @@
> +        if (!success || !cx->compartment()->wrap(cx, &rootThis)) {
> +            ac.reset();
> +            return JSTRAP_ERROR;
> +        }
> +        MOZ_ASSERT_IF(rootThis.isMagic(), rootThis.isMagic(JS_UNINITIALIZED_LEXICAL));

Reminder to handle JS_OPTIMIZED_OUT.

::: js/src/vm/ScopeObject.cpp
@@ +3161,2 @@
>  {
> +    RootedScript firstScript(cx, frame.script());

What does 'first' here mean?

@@ +3161,3 @@
>  {
> +    RootedScript firstScript(cx, frame.script());
> +    for (ScopeIter si(cx, frame, firstScript->main()); !si.done(); ++si) {

Please add a brief comment on why it's correct to use main().

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40f18aee119c

Comment 21

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

I had to back this out for xpcshell crashes: https://treeherder.mozilla.org/logviewer.html#?job_id=21964587&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f44d3d568c2d

Comment 22

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

I'm also wanting to pin some devtools crashes on this patch, too: https://treeherder.mozilla.org/logviewer.html#?job_id=21964846&repo=mozilla-inbound

Comment 23

[Eric Faust [:efaust]]

Attachment: Fix, given test failures. (diff from previous patch)

Turns out, you can't just cheat the pc like we thought in GTVFDMOO. Oops. Do more plumbing.

Comment 24

[Shu-yu Guo [:shu]]

Comment on attachment 8721540 [details] [diff] [review]
Fix, given test failures. (diff from previous patch)

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

coo

Comment 25

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 889096db9c1b).

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7723ac2ee7ce

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7723ac2ee7ce

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late for assertion fixes in 46.