Closed Bug 1249193 Opened 9 years ago Closed 9 years ago

Debugger.onEnterFrame and Debugger.Frame.onStep can inspect incorrect |this| value state for scripted constructors

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: efaust, Assigned: jandem)

References

Details

Attachments

(2 files)

when onEnterFrame fires, we have not yet run the script's prologue, which runs JSOP_FUNCTIONTHIS and initializes the |this| binding, but onEnterFrame passes a Debugger.Frame object to the script, which will seemingly always see |undefined|.
Shu and I did some digging here. It seems to me like we need to initalize that local or CallObject binding directly, during function setup, as we used to for the frame |this|. Jan, how feasible is that? ni? Jan as he wrote all the |this| binding code.
Flags: needinfo?(jdemooij)
(In reply to Eric Faust [:efaust] from comment #1) > Shu and I did some digging here. > > It seems to me like we need to initalize that local or CallObject binding > directly, during function setup, as we used to for the frame |this|. Jan, > how feasible is that? Agreed - the args to a function stepped into are populated, why not `this`? https://bugzilla.mozilla.org/show_bug.cgi?id=1250785#c4
I've a patch, will finish it (and write more tests) tomorrow.
Attached patch Patch (deleted) — Splinter Review
I had some time today so here's the approach we discussed on IRC.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8723208 - Flags: review?(shu)
Comment on attachment 8723208 [details] [diff] [review] Patch Review of attachment 8723208 [details] [diff] [review]: ----------------------------------------------------------------- Nice tests! ::: js/src/vm/ScopeObject.cpp @@ +3188,5 @@ > + // Figure out if we already executed JSOP_FUNCTIONTHIS. > + bool executedInitThisOp = false; > + if (script->functionHasThisBinding()) { > + jsbytecode* initThisPc = script->code(); > + while (*initThisPc != JSOP_FUNCTIONTHIS) Maybe also guard on initThisPc < main() to guard against possible bad bytecode generation. @@ +3200,5 @@ > + // in strict mode), and assign it to the this-argument slot so > + // JSOP_FUNCTIONTHIS will use it and not box a second time. > + if (!GetFunctionThis(cx, frame, res)) > + return false; > + frame.thisArgument() = res; Cool.
Attachment #8723208 - Flags: review?(shu) → review+
Attached patch Part 2 - Fix DebugScopeProxy (deleted) — Splinter Review
Hm I just realized DebugScopeProxy needs a similar fix (and tests).
Attachment #8724656 - Flags: review?(shu)
Keywords: leave-open
Comment on attachment 8724656 [details] [diff] [review] Part 2 - Fix DebugScopeProxy Review of attachment 8724656 [details] [diff] [review]: ----------------------------------------------------------------- Again, beautiful tests. Sorry for the delay.
Attachment #8724656 - Flags: review?(shu) → review+
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: