1227263 - Clean up some this-related code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

[Jan de Mooij [:jandem]]

Attachment: 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

[Jan de Mooij [:jandem]]

Attachment: 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

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove this slot from non-function frames

With this patch only non-eval function frames have a this-slot.

Comment 4

[Eric Faust [:efaust]]

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

[Eric Faust [:efaust]]

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 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ecca6e060c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc18f42e0168
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10c07e6542a

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5ecca6e060c
https://hg.mozilla.org/mozilla-central/rev/cc18f42e0168
https://hg.mozilla.org/mozilla-central/rev/d10c07e6542a