Closed Bug 1227263 Opened 8 years ago Closed 8 years ago

Clean up some this-related code


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: jandem, Assigned: jandem)




(3 files)

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.
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.
Attachment #8691323 - Flags: review?(shu)
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.
Attachment #8691326 - Flags: review?(efaustbmo)
With this patch only non-eval function frames have a this-slot.
Attachment #8691327 - Flags: review?(efaustbmo)
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?
Attachment #8691326 - Flags: review?(efaustbmo) → review+
Comment on attachment 8691327 [details] [diff] [review]
Part 3 - Remove this slot from non-function frames

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

Attachment #8691327 - Flags: review?(efaustbmo) → review+
Attachment #8691323 - Flags: review?(shu) → review+
(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.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.