Closed Bug 1308578 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

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

(Blocks 1 open bug)

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
Duplicate of this bug: 1291310
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)
https://hg.mozilla.org/mozilla-central/rev/5dd60f3331c0
Status: NEW → RESOLVED
Closed: 3 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.