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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Fuzzing Team]

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

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Looks like a regression from bug 1232685?

Comment 3

[Hannes Verschore [:h4writer]]

Clearing needinfo from Eric, since he probably won't look into this anymore.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Consider .this bindings of derived class constructors to be always observable.

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?

Comment 5

[Jan de Mooij [:jandem]]

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().

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Consider .this bindings of derived class constructors to be always observable.

Comment 8

[Jan de Mooij [:jandem]]

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!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd60f3331c0
Consider .this bindings of derived class constructors to be always observable. (r=jandem)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dd60f3331c0

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Actually I just realized uplift was suggested in bug 1291310. 
Shu, can you request uplift to beta ? Thanks!

Comment 13

[Shu-yu Guo [: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

Comment 14

[Gerry Chang [:gchang]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/087894a6a8a6