Clean up some this-related code

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
Attachment #8691323 - Flags: review?(shu)
(Assignee)

Comment 2

2 years ago
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.
Attachment #8691326 - Flags: review?(efaustbmo)
(Assignee)

Comment 3

2 years ago
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.
Attachment #8691327 - Flags: review?(efaustbmo)

Comment 4

2 years ago
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 5

2 years ago
Comment on attachment 8691327 [details] [diff] [review]
Part 3 - Remove this slot from non-function frames

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

Nice.
Attachment #8691327 - Flags: review?(efaustbmo) → review+

Updated

2 years ago
Attachment #8691323 - Flags: review?(shu) → review+

Comment 6

2 years ago
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
(Assignee)

Comment 7

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5ecca6e060c
https://hg.mozilla.org/mozilla-central/rev/cc18f42e0168
https://hg.mozilla.org/mozilla-central/rev/d10c07e6542a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.