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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: efaust, Assigned: jandem)
References
Details
Attachments
(2 files)
(deleted),
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
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|.
Reporter | ||
Comment 1•9 years ago
|
||
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)
Updated•9 years ago
|
(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
Assignee | ||
Comment 4•9 years ago
|
||
I've a patch, will finish it (and write more tests) tomorrow.
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
Hm I just realized DebugScopeProxy needs a similar fix (and tests).
Attachment #8724656 -
Flags: review?(shu)
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 9•9 years ago
|
||
bugherder |
Comment 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 12•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•