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)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: decoder, Assigned: shu)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Attachments
(1 file, 1 obsolete file)
6.50 KB,
patch
|
jandem
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years ago
|
||
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•8 years ago
|
||
Looks like a regression from bug 1232685?
Flags: needinfo?(shu)
Flags: needinfo?(efaustbmo)
Comment 3•8 years ago
|
||
Clearing needinfo from Eric, since he probably won't look into this anymore.
Flags: needinfo?(efaustbmo)
Updated•8 years ago
|
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(shu)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8806965 -
Flags: review?(jdemooij)
Assignee | ||
Updated•8 years ago
|
Attachment #8806200 -
Attachment is obsolete: true
Comment 8•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 11•8 years ago
|
||
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•8 years ago
|
||
Actually I just realized uplift was suggested in bug 1291310.
Shu, can you request uplift to beta ? Thanks!
Flags: needinfo?(shu)
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee: nobody → shu
Comment 15•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•