Closed Bug 1538301 Opened 7 years ago Closed 5 years ago

js::GetThisValueForDebuggerMaybeOptimizedOut compares bytecode pointers for direction between two unrelated scripts

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mgaudet, Unassigned)

References

Details

A contributor working on a patch for Bug 1531479 seems to have stumbled on something curious in js::GetThisValueForDebuggerMaybeOptimizedOut -- Essentially, as I read it, this function iterates through environment scopes to find the |this| value. The curious bit is here: https://searchfox.org/mozilla-central/source/js/src/vm/EnvironmentObject.cpp#3325-3336

It iterates over the bytecode for a particular function scope, and if it sees a FUNCTIONTHIS, attempts to check if we've executed it yet.The thing is, it does this using executedInitThisOp = pc > GetNextPc(it); The problem being -- there's no code guaranteeing that PC is from the same script as we're iterating over, so we're just comparing PC pointers for direction, where they may not exist in the same script bytecode.

This seems... wrong. On IRC Jason said this

The intent is that we walk environments only so far as the immediately enclosing function call activation, so unless we end up in the wrong place, or we're missing a case, that should actually be the currently executing script

Switching this to use BytecodeLocation, as was the plan for Bug 1531479, highlights this because we can't construct a BytecodeLocation out of PC using the script there because the pc doesn't belong to that script, and so asserts.

A minimal reproducing diff of that work is here: https://phabricator.services.mozilla.com/differential/diff/80028/

You can also see the issue just by inserting the following assert just after the RootedScript definition:

MOZ_ASSERT(script->code() <= pc && pc <= script->codeEnd());

and running ../jit-test/jit_test.py --jitflags=none --debugger=lldb ./dist/bin/js debug/Frame-this-02.js

Here's the backtrace there:

Assertion failure: script->code() <= pc && pc <= script->codeEnd(), at /Users/mgaudet/unified/js/src/vm/EnvironmentObject.cpp:3324
Process 3302 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100338188 js`js::GetThisValueForDebuggerMaybeOptimizedOut(cx=0x000000010632d000, frame=(ptr_ = 4399305249), pc="s\x99\x99\t", res=JS::MutableHandleValue @ 0x00007ffeefbeee78) at EnvironmentObject.cpp:3324
   3321     }
   3322
   3323     RootedScript script(cx, ei.scope().as<FunctionScope>().script());
-> 3324     MOZ_ASSERT(script->code() <= pc && pc <= script->codeEnd());
   3325     // Figure out if we executed JSOP_FUNCTIONTHIS and set it.
   3326     bool executedInitThisOp = false;
   3327     if (script->functionHasThisBinding()) {
Target 0: (js) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100338188 js`js::GetThisValueForDebuggerMaybeOptimizedOut(cx=0x000000010632d000, frame=(ptr_ = 4399305249), pc="s\x99\x99\t", res=JS::MutableHandleValue @ 0x00007ffeefbeee78) at EnvironmentObject.cpp:3324
    frame #1: 0x0000000100260887 js`js::DebuggerFrame::getThis(cx=0x000000010632d000, frame=js::HandleDebuggerFrame @ 0x00007ffeefbeef28, result=JS::MutableHandleValue @ 0x00007ffeefbeef20) at Debugger.cpp:9011
    frame #2: 0x000000010026399f js`js::DebuggerFrame::thisGetter(cx=0x000000010632d000, argc=0, vp=0x00007ffeefbef730) at Debugger.cpp:9621
    frame #3: 0x00000001000cc6d6 js`CallJSNative(cx=0x000000010632d000, native=(js`js::DebuggerFrame::thisGetter(JSContext*, unsigned int, JS::Value*) at Debugger.cpp:9618), args=0x00007ffeefbef700)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at Interpreter.cpp:442
    frame #4: 0x00000001000cc31f js`js::InternalCallOrConstruct(cx=0x000000010632d000, args=0x00007ffeefbef700, construct=NO_CONSTRUCT) at Interpreter.cpp:534
    frame #5: 0x00000001000ccbd3 js`InternalCall(cx=0x000000010632d000, args=0x00007ffeefbef700) at Interpreter.cpp:589
    frame #6: 0x00000001000ccc46 js`js::Call(cx=0x000000010632d000, fval=JS::HandleValue @ 0x00007ffeefbef6c0, thisv=JS::HandleValue @ 0x00007ffeefbef6b8, args=0x00007ffeefbef700, rval=JS::MutableHandleValue @ 0x00007ffeefbef6b0) at Interpreter.cpp:605
    frame #7: 0x00000001000cd6ea js`js::CallGetter(cx=0x000000010632d000, thisv=JS::HandleValue @ 0x00007ffeefbef760, getter=JS::HandleValue @ 0x00007ffeefbef758, rval=JS::MutableHandleValue @ 0x00007ffeefbef750) at Interpreter.cpp:729
    frame #8: 0x00000001004db168 js`CallGetter(cx=0x000000010632d000, obj=JS::HandleObject @ 0x00007ffeefbef820, receiver=JS::HandleValue @ 0x00007ffeefbef818, shape=js::HandleShape @ 0x00007ffeefbef810, vp=JS::MutableHandleValue @ 0x00007ffeefbef808) at NativeObject.cpp:2243
    frame #9: 0x0000000100485318 js`bool GetExistingProperty<(js::AllowGC)1>(cx=0x000000010632d000, receiver=js::MaybeRooted<JS::Value, js::CanGC>::HandleType @ 0x00007ffeefbef900, obj=js::MaybeRooted<js::NativeObject *, js::CanGC>::HandleType @ 0x00007ffeefbef8f8, shape=js::MaybeRooted<js::Shape *, js::CanGC>::HandleType @ 0x00007ffeefbef8f0, vp=js::MaybeRooted<JS::Value, js::CanGC>::MutableHandleType @ 0x00007ffeefbef8e8)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) at NativeObject.cpp:2295
    frame #10: 0x00000001004858d9 js`bool NativeGetPropertyInline<(js::AllowGC)1>(cx=0x000000010632d000, obj=js::MaybeRooted<js::NativeObject *, js::CanGC>::HandleType @ 0x00007ffeefbefa80, receiver=js::MaybeRooted<JS::Value, js::CanGC>::HandleType @ 0x00007ffeefbefa78, id=js::MaybeRooted<JS::PropertyKey, js::CanGC>::HandleType @ 0x00007ffeefbefa70, nameLookup=NotNameLookup, vp=js::MaybeRooted<JS::Value, js::CanGC>::MutableHandleType @ 0x00007ffeefbefa68)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyKey, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) at NativeObject.cpp:2544
    frame #11: 0x00000001004856ca js`js::NativeGetProperty(cx=0x000000010632d000, obj=js::HandleNativeObject @ 0x00007ffeefbefae8, receiver=JS::HandleValue @ 0x00007ffeefbefae0, id=JS::HandleId @ 0x00007ffeefbefad8, vp=JS::MutableHandleValue @ 0x00007ffeefbefad0) at NativeObject.cpp:2581
    frame #12: 0x00000001000830ef js`js::GetProperty(cx=0x000000010632d000, obj=Handle<JSObject *> @ 0x00007ffeefbefb70, receiver=Handle<JS::Value> @ 0x00007ffeefbefb68, id=Handle<JS::PropertyKey> @ 0x00007ffeefbefb60, vp=MutableHandle<JS::Value> @ 0x00007ffeefbefb58) at ObjectOperations-inl.h:117
    frame #13: 0x0000000100082fc5 js`js::GetProperty(cx=0x000000010632d000, obj=Handle<JSObject *> @ 0x00007ffeefbefc08, receiver=Handle<JS::Value> @ 0x00007ffeefbefc00, name=0x00002c2583527200, vp=MutableHandle<JS::Value> @ 0x00007ffeefbefbf8) at ObjectOperations-inl.h:124
    frame #14: 0x00000001000cff3c js`js::GetProperty(cx=0x000000010632d000, v=JS::HandleValue @ 0x00007ffeefbefd30, name=js::HandlePropertyName @ 0x00007ffeefbefd28, vp=JS::MutableHandleValue @ 0x00007ffeefbefd20) at Interpreter.cpp:4486
    frame #15: 0x00000001000dc2e6 js`GetPropertyOperation(cx=0x000000010632d000, fp=0x00000001063812a8, script=JS::HandleScript @ 0x00007ffeefbefe30, pc="5\x02", lval=JS::MutableHandleValue @ 0x00007ffeefbefe28, vp=JS::MutableHandleValue @ 0x00007ffeefbefe20) at Interpreter.cpp:215
    frame #16: 0x00000001000be1d7 js`Interpret(cx=0x000000010632d000, state=0x00007ffeefbf3038) at Interpreter.cpp:2762
    frame #17: 0x00000001000b5852 js`js::RunScript(cx=0x000000010632d000, state=0x00007ffeefbf3038) at Interpreter.cpp:422
    frame #18: 0x00000001000cc4c7 js`js::InternalCallOrConstruct(cx=0x000000010632d000, args=0x00007ffeefbf3218, construct=NO_CONSTRUCT) at Interpreter.cpp:562
    frame #19: 0x00000001000ccbd3 js`InternalCall(cx=0x000000010632d000, args=0x00007ffeefbf3218) at Interpreter.cpp:589
    frame #20: 0x00000001000ccc46 js`js::Call(cx=0x000000010632d000, fval=JS::HandleValue @ 0x00007ffeefbf31c0, thisv=JS::HandleValue @ 0x00007ffeefbf31b8, args=0x00007ffeefbf3218, rval=JS::MutableHandleValue @ 0x00007ffeefbf31b0) at Interpreter.cpp:605
    frame #21: 0x0000000100242c8b js`js::Call(cx=0x000000010632d000, fval=JS::HandleValue @ 0x00007ffeefbf32a8, thisObj=0x00002c258358d240, arg0=JS::HandleValue @ 0x00007ffeefbf32a0, rval=JS::MutableHandleValue @ 0x00007ffeefbf3298) at Interpreter.h:106
    frame #22: 0x0000000100244480 js`js::Debugger::fireDebuggerStatement(this=0x000000010634b000, cx=0x000000010632d000, vp=JS::MutableHandleValue @ 0x00007ffeefbf33d8) at Debugger.cpp:1959
    frame #23: 0x00000001002fd147 js`js::Debugger::slowPathOnDebuggerStatement(this=0x00007ffeefbf3968, dbg=0x000000010634b000)::$_12::operator()(js::Debugger*) const at Debugger.cpp:1142
    frame #24: 0x000000010023fd7b js`js::ResumeMode js::Debugger::dispatchHook<js::Debugger::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr)::$_11, js::Debugger::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr)::$_12>(cx=0x000000010632d000, hookIsEnabled=(anonymous class) @ 0x00007ffeefbf3978, fireHook=(anonymous class) @ 0x00007ffeefbf3968)::$_11, js::Debugger::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr)::$_12) at Debugger.cpp:2117
    frame #25: 0x000000010023f9a3 js`js::Debugger::slowPathOnDebuggerStatement(cx=0x000000010632d000, frame=(ptr_ = 4399305249)) at Debugger.cpp:1138
    frame #26: 0x00000001000df0ea js`js::Debugger::onDebuggerStatement(cx=0x000000010632d000, frame=(ptr_ = 4399305249)) at Debugger-inl.h:84
    frame #27: 0x00000001000c7b9b js`Interpret(cx=0x000000010632d000, state=0x00007ffeefbf6c88) at Interpreter.cpp:3927
    frame #28: 0x00000001000b5852 js`js::RunScript(cx=0x000000010632d000, state=0x00007ffeefbf6c88) at Interpreter.cpp:422
    frame #29: 0x00000001000cdb14 js`js::ExecuteKernel(cx=0x000000010632d000, script=JS::HandleScript @ 0x00007ffeefbf6d40, envChainArg=0x0000160683600c40, newTargetValue=0x00007ffeefbf6e68, evalInFrame=(ptr_ = 0), result=0x00000001063811f0) at Interpreter.cpp:781
    frame #30: 0x0000000100124ca9 js`EvalKernel(cx=0x000000010632d000, v=JS::HandleValue @ 0x00007ffeefbf7108, evalType=DIRECT_EVAL, caller=(ptr_ = 4399305081), env=JS::HandleObject @ 0x00007ffeefbf70f8, pc="|\x01", vp=JS::MutableHandleValue @ 0x00007ffeefbf70f0) at Eval.cpp:326
    frame #31: 0x0000000100124fcf js`js::DirectEval(cx=0x000000010632d000, v=JS::HandleValue @ 0x00007ffeefbf73d8, vp=JS::MutableHandleValue @ 0x00007ffeefbf73d0) at Eval.cpp:440
    frame #32: 0x00000001000bfcf6 js`Interpret(cx=0x000000010632d000, state=0x00007ffeefbfa9e8) at Interpreter.cpp:2963
    frame #33: 0x00000001000b5852 js`js::RunScript(cx=0x000000010632d000, state=0x00007ffeefbfa9e8) at Interpreter.cpp:422
    frame #34: 0x00000001000cdb14 js`js::ExecuteKernel(cx=0x000000010632d000, script=JS::HandleScript @ 0x00007ffeefbfaaa0, envChainArg=0x00002c25835bf040, newTargetValue=0x00007ffeefbfabc8, evalInFrame=(ptr_ = 0), result=0x00007ffeefbfb4e0) at Interpreter.cpp:781
    frame #35: 0x0000000100124ca9 js`EvalKernel(cx=0x000000010632d000, v=JS::HandleValue @ 0x00007ffeefbfae68, evalType=INDIRECT_EVAL, caller=(ptr_ = 0), env=JS::HandleObject @ 0x00007ffeefbfae58, pc=0x0000000000000000, vp=JS::MutableHandleValue @ 0x00007ffeefbfae50) at Eval.cpp:326
    frame #36: 0x000000010012405e js`js::IndirectEval(cx=0x000000010632d000, argc=1, vp=0x00007ffeefbfb4e0) at Eval.cpp:424
    frame #37: 0x00000001000cc6d6 js`CallJSNative(cx=0x000000010632d000, native=(js`js::IndirectEval(JSContext*, unsigned int, JS::Value*) at Eval.cpp:417), args=0x00007ffeefbfb490)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at Interpreter.cpp:442
    frame #38: 0x00000001000cc31f js`js::InternalCallOrConstruct(cx=0x000000010632d000, args=0x00007ffeefbfb490, construct=NO_CONSTRUCT) at Interpreter.cpp:534
    frame #39: 0x00000001000ccbd3 js`InternalCall(cx=0x000000010632d000, args=0x00007ffeefbfb490) at Interpreter.cpp:589
    frame #40: 0x00000001000ccc46 js`js::Call(cx=0x000000010632d000, fval=JS::HandleValue @ 0x00007ffeefbfb3e0, thisv=JS::HandleValue @ 0x00007ffeefbfb3d8, args=0x00007ffeefbfb490, rval=JS::MutableHandleValue @ 0x00007ffeefbfb3d0) at Interpreter.cpp:605
    frame #41: 0x00000001008ecabd js`js::ForwardingProxyHandler::call(this=0x0000000101f6f5d8, cx=0x000000010632d000, proxy=JS::HandleObject @ 0x00007ffeefbfb480, args=0x00007ffeefbfdad0) const at Wrapper.cpp:162
    frame #42: 0x00000001008bf9e9 js`js::CrossCompartmentWrapper::call(this=0x0000000101f6f5d8, cx=0x000000010632d000, wrapper=JS::HandleObject @ 0x00007ffeefbfb610, args=0x00007ffeefbfdad0) const at CrossCompartmentWrapper.cpp:238
    frame #43: 0x00000001008e1768 js`js::Proxy::call(cx=0x000000010632d000, proxy=JS::HandleObject @ 0x00007ffeefbfb6b0, args=0x00007ffeefbfdad0) at Proxy.cpp:503
    frame #44: 0x00000001000cc0a3 js`js::InternalCallOrConstruct(cx=0x000000010632d000, args=0x00007ffeefbfdad0, construct=NO_CONSTRUCT) at Interpreter.cpp:508
    frame #45: 0x00000001000ccbd3 js`InternalCall(cx=0x000000010632d000, args=0x00007ffeefbfdad0) at Interpreter.cpp:589
    frame #46: 0x00000001000cc9cd js`js::CallFromStack(cx=0x000000010632d000, args=0x00007ffeefbfdad0) at Interpreter.cpp:593
    frame #47: 0x00000001000c07be js`Interpret(cx=0x000000010632d000, state=0x00007ffeefbfeaa8) at Interpreter.cpp:3075
    frame #48: 0x00000001000b5852 js`js::RunScript(cx=0x000000010632d000, state=0x00007ffeefbfeaa8) at Interpreter.cpp:422
    frame #49: 0x00000001000cdb14 js`js::ExecuteKernel(cx=0x000000010632d000, script=JS::HandleScript @ 0x00007ffeefbfeb60, envChainArg=0x00002c258358e040, newTargetValue=0x00007ffeefbfebb0, evalInFrame=(ptr_ = 0), result=0x0000000000000000) at Interpreter.cpp:781
    frame #50: 0x00000001000cdfd8 js`js::Execute(cx=0x000000010632d000, script=JS::HandleScript @ 0x00007ffeefbfec08, envChainArg=0x00002c258358e040, rval=0x0000000000000000) at Interpreter.cpp:814
    frame #51: 0x0000000100232535 js`ExecuteScript(cx=0x000000010632d000, scope=JS::HandleObject @ 0x00007ffeefbfec48, script=JS::HandleScript @ 0x00007ffeefbfec40, rval=0x0000000000000000) at CompilationAndEvaluation.cpp:438
    frame #52: 0x00000001002325b7 js`JS_ExecuteScript(cx=0x000000010632d000, scriptArg=JS::HandleScript @ 0x00007ffeefbfecb8) at CompilationAndEvaluation.cpp:471
    frame #53: 0x0000000100085776 js`RunFile(cx=0x000000010632d000, filename="/Users/mgaudet/unified/js/src/jit-test/tests/debug/Frame-this-02.js", file=0x00007fffa0a5d030, compileMethod=InflateToUtf16, compileOnly=false) at js.cpp:883
    frame #54: 0x0000000100084e82 js`Process(cx=0x000000010632d000, filename="/Users/mgaudet/unified/js/src/jit-test/tests/debug/Frame-this-02.js", forceTTY=false, kind=FileScript) at js.cpp:1423
    frame #55: 0x000000010003094e js`ProcessArgs(cx=0x000000010632d000, op=0x00007ffeefbff468) at js.cpp:10068
    frame #56: 0x000000010000c368 js`Shell(cx=0x000000010632d000, op=0x00007ffeefbff468, envp=0x00007ffeefbff688) at js.cpp:10685
    frame #57: 0x00000001000075de js`main(argc=13, argv=0x00007ffeefbff618, envp=0x00007ffeefbff688) at js.cpp:11289
Priority: -- → P2

Is there any method to check if pc is from the same script?
and if not, could we say false?

Asking if it's from the same script is easy enough; all jsbytecode* pc that belong to a script obey the following inequality script->code() <= pc <= script->codeEnd(), so you could use the condition in the assert above:

bool inScript = script->code() <= pc && pc <= script->codeEnd();

Trying to reverse engineer enough of this algorithm to say how we handle this isn't entirely clear to me. The documentation for JSOp::FunctionThis says that the opcode only exists in function prologues; so we can assume for a given function either it has run or it hasn't; meaning that within a script executedInitThisOp = pc > GetNextPc(it); is probably a legitimate check;

My personal suspicion here is that some assumption this function made no longer holds (ie, that if you call with pc, your environment iter's initial frame will point to the same script pc comes from). This really deserves some more investigation from someone whose got a better handle on this. As it is, it's -wrong-, but because I can't seem to sort through the algorithm, I can't suss out what is wrong. This makes me uncomfortable with trying to some sort of local patch, without deeply understanding the problem.

If you wanted a large challenge, you could go on an adventure like I did for generators and really reverse how -all of this- works. It would be a lot of work, and you'd have to ask tons of question, but you'd learn reams. The outcome we'd be looking for at the other end is being able to clearly articulate how this works, what the fix is, and why it's legitimate. Probably would land a few comment patches and maybe some refactorings.

Just as a reminder (partially to myself, partially to others) we have the following description of what this 'ought' to be doing:

The intent is that we walk environments only so far as the immediately enclosing function call activation, so unless we end up in the wrong place, or we're missing a case, that should actually be the currently executing script

from Jason.

I want to go on the adventure :)

So I need to learn about what environments are -> how we walk environments -> how the wrong script pc got in here?

Flags: needinfo?(mgaudet)

Heh: Glad to hear you're game!

Yeah, so as far as things that seem to be needed to known here:

  1. Environments (I have a post about this that will help explain what they are, and how they're linked)
  2. How the environment iterator works, and what it means to have withinInitialFrame return true. I've been asking that question in #spidermonkey; still don't have a firm answer yet, but the conversation is progressing.
  3. How does the pc passed to GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut get determined, and why should we expect that the script it comes from be the same as the one determined from the environment iterator.

It's worth noting, some of this stuff changed recently, which may give some hints as to how this was supposed to work. I'd also pay attention to the phabricator comments, as they're often valuable hints.

Flags: needinfo?(mgaudet)

Hmm.

I went through the reproduction steps on this bug, with a minor update (and using Yozaam's patch from Bug 1531479) -- I wasn't able to reproduce!

I think the refactoring that happened around GetThisValueForDebuggerMaybeOptimizedOut getting renamed to GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut may have inadvertently fixed this.

I'm going to take a closer look, but it seems like we may be able to resolve this as FIXED.

I think Bug 1600204 appears to have resolved this.

This try build, with an assert added, is green.

I am going to resolve this as Fixed.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.