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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Fuzzing Team]

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

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Needinfo shu, based on comment 1.

Comment 3

[Andrew Overholt [:overholt]]

This regression is now in Aurora.

Comment 4

[Andrew Overholt [:overholt]]

Meant to say "can we please get an update?" :)

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Fix runtime lexical errors with Debugger environments on the environment chain.

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.

Comment 6

[Jim Blandy :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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0238732bceb1
Fix runtime lexical errors with Debugger environments on the environment chain. (r=jimb)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0238732bceb1

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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

Comment 10

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

Comment 11

[Gerry Chang [:gchang]]

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?

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8783752 [details] [diff] [review]
Fix runtime lexical errors with Debugger environments on the environment chain.

See comment 10.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33573c52d0e8

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

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