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

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: decoder, Assigned: efaust)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla47
x86
Linux
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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()>

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
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.
Eric, is bug 1169740 a likely regressor?
Blocks: 1169740
Flags: needinfo?(efaustbmo)
(Assignee)

Comment 3

2 years ago
Yeeeeep. I had tried to ignore the Debugger interactions. No dice. I'm on it.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8698721 [details] [diff] [review]
Fix
Flags: needinfo?(efaustbmo)
Attachment #8698721 - Flags: review?(shu)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1225471

Comment 6

2 years ago
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?
Attachment #8698721 - Flags: review?(shu)
What's the status of this? Fuzzers are still hitting this more than once a day.
Flags: needinfo?(efaustbmo)
Can this patch please be updated and landed soon?
Duplicate of this bug: 1244081
// 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
Crash Signature: [@ InternalConstruct]
Summary: Assertion failure: data.maxArgv[0].isObject(), at js/src/jit/BaselineJIT.cpp:149 with ES6 Classes and Debugger → 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
Duplicate of this bug: 1235194
(Reporter)

Comment 12

2 years ago
Marking as fuzzblocker, please land asap. This bug causes many different signatures, some of them look like s-s bugs.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][fuzzblocker]
Duplicate of this bug: 1248343
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)
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Flags: needinfo?(jimb)

Comment 15

2 years ago
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.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Flags: needinfo?(jimb)
(Assignee)

Comment 16

2 years ago
Created attachment 8720537 [details] [diff] [review]
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.
Attachment #8698721 - Attachment is obsolete: true
Flags: needinfo?(efaustbmo)
Attachment #8720537 - Flags: review?(shu)

Comment 17

2 years ago
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.
Attachment #8720537 - Flags: review?(shu)
(Assignee)

Comment 18

2 years ago
Created attachment 8720623 [details] [diff] [review]
Reworked, per IRC discussion
Attachment #8720537 - Attachment is obsolete: true
Attachment #8720623 - Flags: review?(shu)

Comment 19

2 years ago
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().
Attachment #8720623 - Flags: review?(shu) → review+

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f18aee119c
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
Flags: needinfo?(efaustbmo)
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
(Assignee)

Comment 23

2 years ago
Created attachment 8721540 [details] [diff] [review]
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.
Flags: needinfo?(efaustbmo)
Attachment #8721540 - Flags: review?(shu)

Comment 24

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

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

coo
Attachment #8721540 - Flags: review?(shu) → review+

Updated

2 years ago
Whiteboard: [jsbugmon:update][fuzzblocker] → [fuzzblocker] [jsbugmon:update,ignore]

Comment 25

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

Comment 26

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

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7723ac2ee7ce
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]
Too late for assertion fixes in 46.
status-firefox46: affected → wontfix
Depends on: 1291310
You need to log in before you can comment on or make changes to this bug.