Closed
Bug 1232685
Opened 7 years ago
Closed 7 years ago
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
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: decoder, Assigned: efaust)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update])
Crash Data
Attachments
(2 files, 2 obsolete files)
26.38 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
18.56 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•7 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•7 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•7 years ago
|
||
Flags: needinfo?(efaustbmo)
Attachment #8698721 -
Flags: review?(shu)
Comment 6•7 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)
Comment 7•7 years ago
|
||
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?
// 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
![]() |
||
Updated•7 years ago
|
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
Reporter | ||
Comment 12•7 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]
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•7 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•7 years ago
|
||
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•7 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•7 years ago
|
||
Attachment #8720537 -
Attachment is obsolete: true
Attachment #8720623 -
Flags: review?(shu)
Comment 19•7 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+
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•7 years ago
|
||
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•7 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•7 years ago
|
Whiteboard: [jsbugmon:update][fuzzblocker] → [fuzzblocker] [jsbugmon:update,ignore]
Comment 25•7 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 889096db9c1b).
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7723ac2ee7ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
![]() |
||
Updated•7 years ago
|
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]
Too late for assertion fixes in 46.
You need to log in
before you can comment on or make changes to this bug.
Description
•