1148388 - Assertion failure: !val.isMagic(), at js/src/jsobj.cpp:3607 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e046475a75cb (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

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;
      frame = frame.older;
    var completion = frame.eval(code);
  };
})(this);
function h() {
  evalInFrame(1, "a.push(0)");
}
new let (a = this) 
  (function g() { h(); })();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000afe483 in js::ToObjectSlow (cx=<optimized out>, val=..., reportScanStack=<optimized out>) at js/src/jsobj.cpp:3607
#0  0x0000000000afe483 in js::ToObjectSlow (cx=<optimized out>, val=..., reportScanStack=<optimized out>) at js/src/jsobj.cpp:3607
#1  0x00000000006561fd in GetPropertyOperation (vp=..., lval=..., pc=<optimized out>, script=..., fp=<optimized out>, cx=0x7ffff691b4e0) at js/src/vm/Interpreter.cpp:253
#2  Interpret (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/vm/Interpreter.cpp:2431
#3  0x0000000000662648 in js::RunScript (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/vm/Interpreter.cpp:452
#4  0x0000000000669656 in js::ExecuteKernel (cx=cx@entry=0x7ffff691b4e0, script=..., script@entry=..., scopeChainArg=..., thisv=..., type=js::EXECUTE_DEBUG, evalInFrame=..., result=result@entry=0x7fffffffafc0) at js/src/vm/Interpreter.cpp:660
#5  0x000000000066a19c in EvaluateInEnv (rval=..., lineno=<optimized out>, filename=<optimized out>, pc=<optimized out>, frame=..., thisv=..., env=..., cx=0x7ffff691b4e0, chars=...) at js/src/vm/Debugger.cpp:6166
#6  DebuggerGenericEval (cx=cx@entry=0x7ffff691b4e0, fullMethodName=fullMethodName@entry=0xd3bafc "Debugger.Frame.prototype.eval", code=..., evalWithBindings=evalWithBindings@entry=EvalWithDefaultBindings, bindings=..., options=..., vp=..., dbg=dbg@entry=0x7ffff695d000, scope=..., iter=iter@entry=0x7fffffffb328) at js/src/vm/Debugger.cpp:6318
#7  0x000000000066b502 in DebuggerFrame_eval (cx=0x7ffff691b4e0, argc=<optimized out>, vp=<optimized out>) at js/src/vm/Debugger.cpp:6332
#8  0x0000000000673822 in js::CallJSNative (cx=0x7ffff691b4e0, native=0x66b280 <DebuggerFrame_eval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#9  0x00000000006628e3 in js::Invoke (cx=cx@entry=0x7ffff691b4e0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:502
#10 0x00000000006643a3 in js::Invoke (cx=cx@entry=0x7ffff691b4e0, thisv=..., fval=..., argc=<optimized out>, argv=0x7ffff51e3268, rval=...) at js/src/vm/Interpreter.cpp:558
#11 0x0000000000b3f03b in js::DirectProxyHandler::call (this=this@entry=0x19ed780 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff691b4e0, proxy=..., proxy@entry=..., args=...) at js/src/proxy/DirectProxyHandler.cpp:77
#12 0x0000000000b45772 in js::CrossCompartmentWrapper::call (this=0x19ed780 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff691b4e0, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:288
#13 0x0000000000b50f32 in js::Proxy::call (cx=cx@entry=0x7ffff691b4e0, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:391
#14 0x0000000000b50fb2 in js::proxy_Call (cx=0x7ffff691b4e0, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:697
#15 0x0000000000673822 in js::CallJSNative (cx=0x7ffff691b4e0, native=0xb50f50 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#16 0x0000000000662b91 in js::Invoke (cx=cx@entry=0x7ffff691b4e0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:495
#17 0x000000000065cf8d in Interpret (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/vm/Interpreter.cpp:2617
#18 0x0000000000662648 in js::RunScript (cx=cx@entry=0x7ffff691b4e0, state=...) at js/src/vm/Interpreter.cpp:452
#19 0x0000000000669656 in js::ExecuteKernel (cx=cx@entry=0x7ffff691b4e0, script=..., script@entry=..., scopeChainArg=..., thisv=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:660
#20 0x000000000066b720 in js::Execute (cx=cx@entry=0x7ffff691b4e0, script=script@entry=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:703
#21 0x0000000000a445ae in ExecuteScript (cx=cx@entry=0x7ffff691b4e0, obj=..., scriptArg=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4095
#22 0x0000000000a4472b in JS_ExecuteScript (cx=cx@entry=0x7ffff691b4e0, scriptArg=..., scriptArg@entry=...) at js/src/jsapi.cpp:4117
#23 0x0000000000425313 in RunFile (compileOnly=false, file=0x7ffff699b400, filename=0x7fffffffdf74 "min.js", cx=0x7ffff691b4e0) at js/src/shell/js.cpp:466
#24 Process (cx=cx@entry=0x7ffff691b4e0, filename=0x7fffffffdf74 "min.js", forceTTY=forceTTY@entry=false) at js/src/shell/js.cpp:597
#25 0x0000000000471d93 in ProcessArgs (op=0x7fffffffda00, cx=0x7ffff691b4e0) at js/src/shell/js.cpp:5758
#26 Shell (envp=<optimized out>, op=0x7fffffffda00, cx=0x7ffff691b4e0) at js/src/shell/js.cpp:6024
#27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6366
rax	0x0	0
rbx	0x7ffff691b4e0	140737330132192
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffa720	140737488332576
rsp	0x7fffffffa720	140737488332576
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffa4e0	140737488332000
r11	0x7ffff6c27960	140737333328224
r12	0x7ffff51e3310	140737305785104
r13	0x7ffff51e3308	140737305785096
r14	0x7ffff691b4f8	140737330132216
r15	0x198e120	26796320
rip	0xafe483 <js::ToObjectSlow(JSContext*, JS::Handle<JS::Value>, bool)+115>
=> 0xafe483 <js::ToObjectSlow(JSContext*, JS::Handle<JS::Value>, bool)+115>:	movl   $0xe17,0x0
   0xafe48e <js::ToObjectSlow(JSContext*, JS::Handle<JS::Value>, bool)+126>:	callq  0x4230a0 <abort@plt>

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/fb00dedf441c
user:        Shu-yu Guo
date:        Wed Jan 14 22:57:35 2015 -0800
summary:     Bug 963879 - Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through evals. (r=luke)

This iteration took 0.845 seconds to run.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Handle lost accesses in missing Debugger scopes for block objects.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8589406 [details] [diff] [review]
Handle lost accesses in missing Debugger scopes for block objects.

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

Should we test assignment to 'a' as well?

What other sorts of unexpected scope objects can the debugger create? Does an analogous situation ever arise with Call objects? I don't know the details, but I imagine a Call object for a lightweight function might be analogous to a ClonedBlock for a non-needsClone static block.

::: js/src/jit-test/tests/debug/Frame-eval-29.js
@@ +7,5 @@
> +
> +function test() {
> +  {
> +    let a = 42;
> +    (function () { f(); })();

Why is this contortion necessary? Why not just |f();|?

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8589406 [details] [diff] [review]
Handle lost accesses in missing Debugger scopes for block objects.

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

::: js/src/vm/ScopeObject.cpp
@@ +1355,5 @@
> +                    // A ClonedBlockObject whose static block does not need
> +                    // cloning is a "hollow" block object reflected for
> +                    // missing block scopes. Their slot values are lost.
> +                    if (!block->staticBlock().needsClone()) {
> +                        *accessResult = ACCESS_LOST;

I'm not sure I understand exactly when hollow block objects are created. Can we store values in hollow block objects? If so, then shouldn't we be able to sometimes retrieve them via this path?

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #3)
> Comment on attachment 8589406 [details] [diff] [review]
> Handle lost accesses in missing Debugger scopes for block objects.
> 
> Review of attachment 8589406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should we test assignment to 'a' as well?
> 
> What other sorts of unexpected scope objects can the debugger create? Does
> an analogous situation ever arise with Call objects? I don't know the
> details, but I imagine a Call object for a lightweight function might be
> analogous to a ClonedBlock for a non-needsClone static block.
> 
> ::: js/src/jit-test/tests/debug/Frame-eval-29.js
> @@ +7,5 @@
> > +
> > +function test() {
> > +  {
> > +    let a = 42;
> > +    (function () { f(); })();
> 
> Why is this contortion necessary? Why not just |f();|?

It's necessary because going through a non-heavyweight inner function is what causes creation of the "hollow" scopes for missing scopes. Otherwise the Debugger can actually read test()'s frame and get the value of a, since it knows exactly that that frame is live on stack. By contorting through an inner function, even though we know the lambda's enclosing function is test(), we have no way of actually knowing if the frame of test() that is on-stack *would have been* the stack that allocated the lambda. In this test, the ground truth is "yes", but the Debugger has no way to tell, and so it creates a fake "hollow" call object to stand in.

Comment 6

[Shu-yu Guo [:shu]]

s/the stack that allocated/the frame that allocated

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #4)
> Comment on attachment 8589406 [details] [diff] [review]
> Handle lost accesses in missing Debugger scopes for block objects.
> 
> Review of attachment 8589406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ScopeObject.cpp
> @@ +1355,5 @@
> > +                    // A ClonedBlockObject whose static block does not need
> > +                    // cloning is a "hollow" block object reflected for
> > +                    // missing block scopes. Their slot values are lost.
> > +                    if (!block->staticBlock().needsClone()) {
> > +                        *accessResult = ACCESS_LOST;
> 
> I'm not sure I understand exactly when hollow block objects are created. Can
> we store values in hollow block objects? If so, then shouldn't we be able to
> sometimes retrieve them via this path?

Despite what we talked about on IRC, we cannot store values in hollow scope objects, block or call or otherwise. The check for that is higher up in the call chain, in DebugScopeProxy::set itself. I'll change get to match.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #7)

> 
> Despite what we talked about on IRC, we cannot store values in hollow scope
> objects, block or call or otherwise. The check for that is higher up in the
> call chain, in DebugScopeProxy::set itself. I'll change get to match.

Gah, nevermind on changing get to match too. The reason is that sometimes we want to actually return the sentinel OPTIMIZED_OUT value, and sometimes we want to throw. Setting always throws.

Comment 9

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #5)
> > Why is this contortion necessary? Why not just |f();|?
> 
> It's necessary because going through a non-heavyweight inner function is
> what causes creation of the "hollow" scopes for missing scopes. Otherwise
> the Debugger can actually read test()'s frame and get the value of a, since
> it knows exactly that that frame is live on stack. By contorting through an
> inner function, even though we know the lambda's enclosing function is
> test(), we have no way of actually knowing if the frame of test() that is
> on-stack *would have been* the stack that allocated the lambda. In this
> test, the ground truth is "yes", but the Debugger has no way to tell, and so
> it creates a fake "hollow" call object to stand in.

This is a great explanation. Could you lightly edit this into a comment above the contortion?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89c9b9067ed8