Assertion failure: v.isUndefined(), at js/src/jsnum.cpp:1531 with Debugger

RESOLVED FIXED in Firefox 51

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla51
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment)

The following testcase crashes on mozilla-central revision c9a70b64f2fa (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

var evalInFrame = (function (global) {
  var dbgGlobal = newGlobal();
  var dbg = new dbgGlobal.Debugger();
  return function evalInFrame(upCount, code) {
    dbg.addDebuggee(global);
    var frame = dbg.getNewestFrame().older;
    for (var i = 0; i < upCount; i++) {}
    var completion = frame.eval(code);
  };
})(this);
let y = evalInFrame(0, "x |= 1"), x=x0;



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000096895b in js::ToNumberSlow (cx=0x7ffff6965000, v=..., out=out@entry=0x7fffffffb540) at js/src/jsnum.cpp:1531
#0  0x000000000096895b in js::ToNumberSlow (cx=0x7ffff6965000, v=..., out=out@entry=0x7fffffffb540) at js/src/jsnum.cpp:1531
#1  0x000000000096897a in js::ToNumberSlow (cx=<optimized out>, v=..., out=out@entry=0x7fffffffb540) at js/src/jsnum.cpp:1554
#2  0x000000000096904d in js::ToInt32Slow (cx=<optimized out>, v=..., out=0x7fffffffb920) at js/src/jsnum.cpp:1660
#3  0x0000000000ae1f61 in Interpret (cx=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:2208
#4  0x0000000000aeac79 in js::RunScript (cx=cx@entry=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:399
#5  0x0000000000aed63b in js::ExecuteKernel (cx=cx@entry=0x7ffff6965000, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., result=result@entry=0x7fffffffbdc0) at js/src/vm/Interpreter.cpp:679
#6  0x0000000000a8ccae in EvaluateInEnv (rval=..., lineno=<optimized out>, filename=0x1002dbd "debugger eval code", pc=<optimized out>, frame=..., env=..., cx=0x7ffff6965000, chars=...) at js/src/vm/Debugger.cpp:7623
#7  DebuggerGenericEval (cx=0x7ffff6965000, bindings=..., bindings@entry=..., options=..., vp=..., dbg=0x7ffff6956000, scope=..., iter=0x7fffffffc278, chars=...) at js/src/vm/Debugger.cpp:7708
#8  0x0000000000a8d4a4 in DebuggerFrame_eval (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=<optimized out>) at js/src/vm/Debugger.cpp:7730
#9  0x0000000000af01d4 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xa8d0d0 <DebuggerFrame_eval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232
#10 0x0000000000aeae33 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6965000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453
#11 0x0000000000aeb176 in InternalCall (cx=cx@entry=0x7ffff6965000, args=...) at js/src/vm/Interpreter.cpp:498
#12 0x0000000000aeb2ce in js::Call (cx=cx@entry=0x7ffff6965000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:517
#13 0x0000000000a12556 in js::Wrapper::call (this=this@entry=0x1cc6280 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6965000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:165
#14 0x00000000009fcc33 in js::CrossCompartmentWrapper::call (this=0x1cc6280 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6965000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:329
#15 0x0000000000a06cd3 in js::Proxy::call (cx=cx@entry=0x7ffff6965000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:401
#16 0x0000000000a06dd8 in js::proxy_Call (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:689
#17 0x0000000000af01d4 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xa06d40 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232
[...]
#30 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7437
rax	0x0	0
rbx	0x7fffffffb4e0	140737488336096
rcx	0x7ffff6c28a10	140737333332496
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb530	140737488336176
rsp	0x7fffffffb4c0	140737488336064
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fdc740	140737353992000
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0xfffdffffffffffff	-562949953421313
r13	0xfff8ffffffffffff	-1970324836974593
r14	0x7fffffffb4f0	140737488336112
r15	0x7ffff6965000	140737330434048
rip	0x96895b <js::ToNumberSlow(js::ExclusiveContext*, JS::Value, double*)+715>
=> 0x96895b <js::ToNumberSlow(js::ExclusiveContext*, JS::Value, double*)+715>:	movl   $0x0,0x0
   0x968966 <js::ToNumberSlow(js::ExclusiveContext*, JS::Value, double*)+726>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151006132131" and the hash "d6059530b0317e6f6b141582b611469505256be4".
The "bad" changeset has the timestamp "20151006135536" and the hash "cfc1820361f599c55128b29de4332f8d06511e07".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d6059530b0317e6f6b141582b611469505256be4&tochange=cfc1820361f599c55128b29de4332f8d06511e07
Needinfo shu, based on comment 1.
Flags: needinfo?(shu)
This regression is now in Aurora.
Flags: needinfo?(nihsanullah)
Meant to say "can we please get an update?" :)
Assignee: nobody → shu
Flags: needinfo?(nihsanullah)
Some background first. For names that must be looked up dynamically, TDZ
checks are built in to the various NAME ops. For an assignment |x = 42|,
we emit:

BINDNAME "x"
INT8 42
SETNAME "x"

where the order of operations is:

1) BINDNAME first looks up the env where "x" is bound,
2) The RHS is evaluated,
3) The resulting value is assigned to "x" in the env from step 1)

That is, spec requires it that 3) is what throws any TDZ violations,
meaning we must evaluate RHS first. However, the implementation of
SETNAME is ultimately a call into SetProperty. Slowing down SetProperty
calls with TDZ checks was a non-starter. What we do instead is that if
BINDNAME sees a TDZ poison value when looking up the environment,
instead of pushing the actual environment, it pushes a magical
LexicalRuntimeErrorObject, which throws TDZ errors if it's touched in
any way (sets, gets, has-own-property, etc).

The code that created the LexicalRuntimeErrorObject did not understand
how to unwrap DebugScopeObjects, thus it considered that
DebugScopeObjects could never generate TDZ errors, leading to this bug.
Attachment #8783752 - Flags: review?(jimb)
Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

Review of attachment 8783752 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for the background. That made the review much easier.

::: js/src/jsobj.cpp
@@ +2234,5 @@
> +        if (name != cx->names().dotThis) {
> +            // Treat Debugger environments specially for TDZ checks, as they
> +            // look like non-native environments but in fact wrap native
> +            // environments.
> +            if (scope->is<DebugScopeObject>() && shape) {

If I'm reading correctly, it's never possible for isTDZ to be true if `shape` is null. Having the test for `shape` in the DebugScopeObject branch, but not in the other branch, made me wonder naively if it's ever possible to set `isTDZ` when `shape` is null. Could we just put `shape &&` next to the other `dotThis` check?

This would all make a bit more sense if `IsUnitializedLexicalSlot` always required a non-null `shape`, and the callers checked for it.
Attachment #8783752 - Flags: review?(jimb) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0238732bceb1
Fix runtime lexical errors with Debugger environments on the environment chain. (r=jimb)
Flags: needinfo?(shu)
https://hg.mozilla.org/mozilla-central/rev/0238732bceb1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Shu-yu, is it something that we would like to uplift to aurora? If this is the case, could you fill the uplift request? Thanks
Flags: needinfo?(shu)
Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

Approval Request Comment
[Feature/regressing bug #]: 1263355
[User impact if declined]: non-spec compliant behavior in JS debugger
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: low, bug fix
[String/UUID change made/needed]: none
Flags: needinfo?(shu)
Attachment #8783752 - Flags: approval-mozilla-aurora?
Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

Hi :shu,
Per comment #8, the patch has been in 51 aurora. Do you want to uplift to 50 beta?
Flags: needinfo?(shu)
Attachment #8783752 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

See comment 10.
Flags: needinfo?(shu)
Attachment #8783752 - Flags: approval-mozilla-beta?
Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

Stabilized on 51 for a while, low risk, Beta50+
Attachment #8783752 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I had to back this out from Beta for bustage. Needs rebasing from someone who knows their way around this code a bit better :)

https://hg.mozilla.org/releases/mozilla-beta/rev/49776d31766dd130969f9ec4ea3354a43e8e6d9d
Flags: needinfo?(shu)
Oh, actually, this fix depends on the big rewrite from bug 1263355, which we aren't going to uplift, so I'm going to retract the uplift request.
Flags: needinfo?(shu)
Attachment #8783752 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.