Closed Bug 1308578 Opened 8 years ago Closed 8 years ago

Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h:1211 with Debugger

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: decoder, Assigned: shu)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision 4b9944879c9a (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off): g = newGlobal(); g.parent = this; g.eval("new Debugger(parent).onExceptionUnwind = function () {}"); a = new class extends Array { constructor() { for (;; ([] = p)) {} } } Backtrace: received signal SIGSEGV, Segmentation fault. 0x0000000000a5e76c in JS::Value::isMagic (this=this@entry=0x7fffffffb900, why=why@entry=JS_UNINITIALIZED_LEXICAL) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1211 #0 0x0000000000a5e76c in JS::Value::isMagic (this=this@entry=0x7fffffffb900, why=why@entry=JS_UNINITIALIZED_LEXICAL) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1211 #1 0x0000000000a77b97 in JS::Value::isMagic (why=JS_UNINITIALIZED_LEXICAL, this=<optimized out>) at js/src/vm/Debugger.cpp:1592 #2 js::ValueOperations<JS::MutableHandle<JS::Value> >::isMagic (why=JS_UNINITIALIZED_LEXICAL, this=<synthetic pointer>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1776 #3 GetThisValueForCheck (cx=cx@entry=0x7ffff695f000, frame=..., pc=pc@entry=0x7ffff03027b3 "\232", thisv=thisv@entry=..., maybeThisv=...) at js/src/vm/Debugger.cpp:1592 #4 0x0000000000a77ca9 in js::Debugger::processHandlerResult (this=this@entry=0x7ffff693d800, ac=..., success=success@entry=true, rv=..., frame=..., pc=pc@entry=0x7ffff03027b3 "\232", vp=...) at js/src/vm/Debugger.cpp:1672 #5 0x0000000000a8ffdf in js::Debugger::fireExceptionUnwind (this=this@entry=0x7ffff693d800, cx=cx@entry=0x7ffff695f000, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:1761 #6 0x0000000000a9045c in js::Debugger::<lambda(js::Debugger*)>::operator() (dbg=0x7ffff693d800, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:1023 #7 js::Debugger::dispatchHook<js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)>, js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)> > (fireHook=..., cx=0x7ffff695f000, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1873 #8 js::Debugger::slowPathOnExceptionUnwind (cx=cx@entry=0x7ffff695f000, frame=...) at js/src/vm/Debugger.cpp:1024 #9 0x00000000006d769a in js::Debugger::onExceptionUnwind (frame=..., cx=0x7ffff695f000) at js/src/vm/Debugger-inl.h:66 #10 js::jit::HandleExceptionBaseline (pc=0x7ffff03027b3 "\232", rfe=0x7fffffffc830, frame=..., cx=0x7ffff695f000) at js/src/jit/JitFrames.cpp:656 #11 js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:846 #12 0x00007ffff7e3b646 in ?? () #13 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x1 1 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffb820 140737488336928 rsp 0x7fffffffb820 140737488336928 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff695f000 140737330409472 r13 0x7fffffffb900 140737488337152 r14 0x7fffffffb830 140737488336944 r15 0x7fffffffb930 140737488337200 rip 0xa5e76c <JS::Value::isMagic(JSWhyMagic) const+44> => 0xa5e76c <JS::Value::isMagic(JSWhyMagic) const+44>: movl $0x0,0x0 0xa5e777 <JS::Value::isMagic(JSWhyMagic) const+55>: ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20160222110021" and the hash "32de5c14d7f8dd8563c3f6fd807ede191c1f254e". The "bad" changeset has the timestamp "20160222124458" and the hash "7723ac2ee7ce143112c8506826a858c7b0df455a". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=32de5c14d7f8dd8563c3f6fd807ede191c1f254e&tochange=7723ac2ee7ce143112c8506826a858c7b0df455a
Looks like a regression from bug 1232685?
Flags: needinfo?(shu)
Flags: needinfo?(efaustbmo)
Clearing needinfo from Eric, since he probably won't look into this anymore.
Flags: needinfo?(efaustbmo)
The bug is that the Debugger needs to get the 'this' value in handlers when unwinding through derived class ctor frames to do a TDZ check, and Ion sometimes optimizes out the binding. I'm not 100% sure this is the correct fix. Jan, do you see a way to allow Ion to optimize out the .this of derived class ctors?
Attachment #8806200 - Flags: review?(jdemooij)
Flags: needinfo?(shu)
Comment on attachment 8806200 [details] [diff] [review] Consider .this bindings of derived class constructors to be always observable. Review of attachment 8806200 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this (pun intended). ::: js/src/jit/CompileInfo.h @@ +440,5 @@ > + // The |this| frame slot in derived class constructors should never be > + // optimized out, as a Debugger might need to perform TDZ checks on it > + // via, e.g., an exceptionUnwind handler. The TDZ check is required > + // for correctness if the handler decides to continue execution. > + if (script_->isDerivedClassConstructor()) { I think this can be called on background threads (via MResumePoint::isRecoverableOperand) and I'm a bit worried about perf issues with the loop below. I'd prefer adding a field to CompileInfo to store the slot, we can set that slot in the CompileInfo constructor if script->isDerivedClassConstructor().
Attachment #8806200 - Flags: review?(jdemooij)
Attachment #8806200 - Attachment is obsolete: true
Comment on attachment 8806965 [details] [diff] [review] Consider .this bindings of derived class constructors to be always observable. Review of attachment 8806965 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8806965 - Flags: review?(jdemooij) → review+
Pushed by shu@rfrn.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd60f3331c0 Consider .this bindings of derived class constructors to be always observable. (r=jandem)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Probably best to let this ride the trains to release from 52. But you can still request uplift to 51 if you think it is worth doing.
Actually I just realized uplift was suggested in bug 1291310. Shu, can you request uplift to beta ? Thanks!
Flags: needinfo?(shu)
Comment on attachment 8806965 [details] [diff] [review] Consider .this bindings of derived class constructors to be always observable. Approval Request Comment [Feature/regressing bug #]: Don't know [User impact if declined]: Incorrect behavior in JS debugger for debugging derived class constructors [Describe test coverage new/current, TreeHerder]: On m-c [Risks and why]: Low, bug fix only [String/UUID change made/needed]: None
Flags: needinfo?(shu)
Attachment #8806965 - Flags: approval-mozilla-beta?
Comment on attachment 8806965 [details] [diff] [review] Consider .this bindings of derived class constructors to be always observable. Fix an assertion error. Beta51+. Should be in 51 beta 2.
Attachment #8806965 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: