Last Comment Bug 1227263 - Clean up some this-related code
: Clean up some this-related code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla45
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1132183
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-23 12:11 PST by Jan de Mooij [:jandem]
Modified: 2015-11-26 08:01 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Rename thisValue to thisArgument (20.76 KB, patch)
2015-11-24 03:06 PST, Jan de Mooij [:jandem]
shu: review+
Details | Diff | Splinter Review
Part 2 - Remove this-slot from generators (12.93 KB, patch)
2015-11-24 03:14 PST, Jan de Mooij [:jandem]
efaustbmo: review+
Details | Diff | Splinter Review
Part 3 - Remove this slot from non-function frames (16.26 KB, patch)
2015-11-24 03:16 PST, Jan de Mooij [:jandem]
efaustbmo: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2015-11-23 12:11:18 PST
Bug 1132183 rewrote the way `this` works inside the engine. There's some more cleanup we can do now:

* frame->thisValue() is only valid for function frames and is the original this value, so it can be renamed frame->thisArgument().

* Only (non-eval) function frames need a this-slot. (Not sure if I'll change this; it might be simpler if every frame has a this-slot but only functions use it.)

* Generator objects no longer need a this-slot, as their 'this' is stored in the CallObject.
Comment 1 User image Jan de Mooij [:jandem] 2015-11-24 03:06:24 PST
Created attachment 8691323 [details] [diff] [review]
Part 1 - Rename thisValue to thisArgument

This patch renames the thisValue() methods on frames to thisArgument().

I also fixed some comments and added asserts to ensure we only call these methods on non-eval function frames.
Comment 2 User image Jan de Mooij [:jandem] 2015-11-24 03:14:05 PST
Created attachment 8691326 [details] [diff] [review]
Part 2 - Remove this-slot from generators

When we resume a generator, we push UndefinedValue() for all formals because the real formals are stored in the CallObject. With this patch we also push UndefinedValue() for |this|.

I found a small, pre-existing bug when calling a legacy generator with 'new' and then OSR'ing into Baseline - I added a test for this. ES6 generators cannot be called with 'new' so they don't have this issue.
Comment 3 User image Jan de Mooij [:jandem] 2015-11-24 03:16:54 PST
Created attachment 8691327 [details] [diff] [review]
Part 3 - Remove this slot from non-function frames

With this patch only non-eval function frames have a this-slot.
Comment 4 User image Eric Faust [:efaust] 2015-11-24 16:29:05 PST
Comment on attachment 8691326 [details] [diff] [review]
Part 2 - Remove this-slot from generators

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

::: js/src/jit/BaselineCompiler.cpp
@@ +4026,5 @@
>      }
>      masm.bind(&loopDone);
>  
> +    // Push |undefined| for |this|.
> +    masm.pushValue(UndefinedValue());

Hmm, can (should?) we use JS_POISON_THIS or something here (and throughout)? We should never consult this value, right?
Comment 5 User image Eric Faust [:efaust] 2015-11-24 16:30:43 PST
Comment on attachment 8691327 [details] [diff] [review]
Part 3 - Remove this slot from non-function frames

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

Nice.
Comment 7 User image Jan de Mooij [:jandem] 2015-11-26 03:05:02 PST
(In reply to Eric Faust [:efaust] from comment #4)
> Hmm, can (should?) we use JS_POISON_THIS or something here (and throughout)?
> We should never consult this value, right?

We also push undefined for the formal arguments, so I just followed that. Maybe we could use a magic value but not sure if it's worth it.

Note You need to log in before you can comment on or make changes to this bug.