Getter and setter on super are called with wrong "this" object

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Cannot override getter / setter.

* TEST
<script>
"use strict";
var Base = class Base {
    constructor() { this._name = "Name"; }
    get name() { return this._name; } // If this instead returns a static: return "Foo" things somewhat work.
    set name(x) { this._name = x; }
};

var Subclass = class Subclass extends Base {
    get name() { return super.name; }
};

var instance = new Subclass;
console.log(instance.name); // undefined, but expected "Name"
</script>

[Radar WebKit Bug Importer]

<rdar://problem/21885916>

[Joseph Pecoraro]

After discussing with Ryosuke Niwa it sounds like we are invoking the `super.name` getter, but with the wrong "this" object.

Sounds like we were replacing `super` with `this.__proto__` which would be `Base.prototype`. So we are actually getting `Base.prototype._name` instead of `this._name`. You can verify this by setting `Base.prototype._name` to a value, and then instance.name returns that value.

[Ryosuke Niwa]

I talked with Geoff about this, and it seems like we'd need to add an extra argument to op_get_by_id, which specifies "this" for the getter/setter.

We should probably split the patch into two.  The first one that just does this refactoring and the second one that actually fixes the bug.

[Joseph Pecoraro]

Created attachment 277416 [details]
[TEST] Test Page

[Saam Barati]

Created attachment 277600 [details]
WIP

It begins

[Saam Barati]

*** Bug 157136 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 277684 [details]
WIP

[Saam Barati]

Created attachment 277730 [details]
WIP

I think the patch now works and is correct. I need to vet NodesCodegen again to make sure I didn't miss any places where we support super.
I also want to refactor get_by_id_with_this to not have a bunch of unused arguments.

[Saam Barati]

Created attachment 277758 [details]
WIP

Needs 32-bit support then I think I'm done.

[WebKit Commit Bot]

Attachment 277758 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:886:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:900:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2049:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2050:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2053:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 277802 [details]
WIP

maybe 32-bit is done? Lets see what EWS thinks.

[WebKit Commit Bot]

Attachment 277802 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:887:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:901:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2075:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2076:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2079:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 277835 [details]
WIP

[WebKit Commit Bot]

Attachment 277835 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:887:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:901:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2085:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2086:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/CCallHelpers.h:2089:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 277872 [details]
patch

[Saam Barati]

Comment on attachment 277872 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277872&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2887
> +        if (GPRInfo::numberOfRegisters < 8) {

I need to actually change this code I think. I did this for X86-32bit originally (which doesn't have argument registers).
This code is broken for an ABI that has argument registers but still has less than 8 registers like MIPS.
Writing this code for MIPS seems painful. I think you would have to first push the last 5 registers
onto the stack. Then call setupArgumentsWithExecState to get the four argument registers.

Anyway, this code should really be behind
#if CPU(X86)
...
#else

#endif

And then the MIPS folks can add any code they need to as an #elif

[Saam Barati]

Created attachment 277906 [details]
benchmark results

[Saam Barati]

Created attachment 278414 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 278414 [details]
patch for landing

Clearing flags on attachment: 278414

Committed r200586: <http://trac.webkit.org/changeset/200586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Nice! We should update the inspector now (bug 157488).

[Csaba Osztrogonác]

(In reply to comment #19)
> Comment on attachment 278414 [details]
> patch for landing
> 
> Clearing flags on attachment: 278414
> 
> Committed r200586: <http://trac.webkit.org/changeset/200586>

The new stress/super-property-access.js test fails (crash/timeout)
on all JSC tester bots, see build.webkit.org for details.

[Saam Barati]

(In reply to comment #22)
> (In reply to comment #19)
> > Comment on attachment 278414 [details]
> > patch for landing
> > 
> > Clearing flags on attachment: 278414
> > 
> > Committed r200586: <http://trac.webkit.org/changeset/200586>
> 
> The new stress/super-property-access.js test fails (crash/timeout)
> on all JSC tester bots, see build.webkit.org for details.

Thanks I'll update the test to be shorter.

[Saam Barati]

landed follow up patch to make test run faster:
http://trac.webkit.org/changeset/200632

[Filip Pizlo]

Comment on attachment 278414 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=278414&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4036
> +            Node* getByValWithThis = addToGraph(GetByValWithThis, base, thisValue, property);

It should be possible to get a prediction from a value profile here.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4137
> +                addToGraph(GetByIdWithThis, OpInfo(identifierNumber), base, thisValue));

It should be possible to get a prediction from a value profile here.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:666
> +            setPrediction(SpecBytecodeTop);

It's wrong to ever use any of the Top's in prediction propagation phase.  This tells the prediction propagator that this node will *definitely* return values of all types, which causes us to assume the worst for all downstream code.

This should just be a matter of giving these bytecode instructions a value profile, and pulling in the value profile's results in the bytecode parser.

Filed: https://bugs.webkit.org/show_bug.cgi?id=160922

[Saam Barati]

(In reply to comment #25)
> Comment on attachment 278414 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278414&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4036
> > +            Node* getByValWithThis = addToGraph(GetByValWithThis, base, thisValue, property);
> 
> It should be possible to get a prediction from a value profile here.
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4137
> > +                addToGraph(GetByIdWithThis, OpInfo(identifierNumber), base, thisValue));
> 
> It should be possible to get a prediction from a value profile here.
> 
> > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:666
> > +            setPrediction(SpecBytecodeTop);
> 
> It's wrong to ever use any of the Top's in prediction propagation phase. 
> This tells the prediction propagator that this node will *definitely* return
> values of all types, which causes us to assume the worst for all downstream
> code.
> 
> This should just be a matter of giving these bytecode instructions a value
> profile, and pulling in the value profile's results in the bytecode parser.
> 
> Filed: https://bugs.webkit.org/show_bug.cgi?id=160922

I think the right thing going forward is just teaching get_by_id
that it can have a different base and |this|. This way, we can remove
get_by_id_with_this completely.

[Filip Pizlo]

(In reply to comment #26)
> (In reply to comment #25)
> > Comment on attachment 278414 [details]
> > patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278414&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4036
> > > +            Node* getByValWithThis = addToGraph(GetByValWithThis, base, thisValue, property);
> > 
> > It should be possible to get a prediction from a value profile here.
> > 
> > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4137
> > > +                addToGraph(GetByIdWithThis, OpInfo(identifierNumber), base, thisValue));
> > 
> > It should be possible to get a prediction from a value profile here.
> > 
> > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:666
> > > +            setPrediction(SpecBytecodeTop);
> > 
> > It's wrong to ever use any of the Top's in prediction propagation phase. 
> > This tells the prediction propagator that this node will *definitely* return
> > values of all types, which causes us to assume the worst for all downstream
> > code.
> > 
> > This should just be a matter of giving these bytecode instructions a value
> > profile, and pulling in the value profile's results in the bytecode parser.
> > 
> > Filed: https://bugs.webkit.org/show_bug.cgi?id=160922
> 
> I think the right thing going forward is just teaching get_by_id
> that it can have a different base and |this|. This way, we can remove
> get_by_id_with_this completely.

Right now, if you have any GetByIdWithThis anywhere in a function, then *everything* in the function is likely to get polluted by the bad prediction propagation.

So, fixing this is *much* higher priority than implementing a proper get_by_id.

In the future, we should be very vigilant about this.  It's OK for some node in DFG IR to emit bad code.  It's never OK for a node in DFG IR to claim that it produces TOP as its output, since that pessimizes all downstream nodes.

[Saam Barati]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Comment on attachment 278414 [details]
> > > patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=278414&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4036
> > > > +            Node* getByValWithThis = addToGraph(GetByValWithThis, base, thisValue, property);
> > > 
> > > It should be possible to get a prediction from a value profile here.
> > > 
> > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4137
> > > > +                addToGraph(GetByIdWithThis, OpInfo(identifierNumber), base, thisValue));
> > > 
> > > It should be possible to get a prediction from a value profile here.
> > > 
> > > > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:666
> > > > +            setPrediction(SpecBytecodeTop);
> > > 
> > > It's wrong to ever use any of the Top's in prediction propagation phase. 
> > > This tells the prediction propagator that this node will *definitely* return
> > > values of all types, which causes us to assume the worst for all downstream
> > > code.
> > > 
> > > This should just be a matter of giving these bytecode instructions a value
> > > profile, and pulling in the value profile's results in the bytecode parser.
> > > 
> > > Filed: https://bugs.webkit.org/show_bug.cgi?id=160922
> > 
> > I think the right thing going forward is just teaching get_by_id
> > that it can have a different base and |this|. This way, we can remove
> > get_by_id_with_this completely.
> 
> Right now, if you have any GetByIdWithThis anywhere in a function, then
> *everything* in the function is likely to get polluted by the bad prediction
> propagation.
> 
> So, fixing this is *much* higher priority than implementing a proper
> get_by_id.
> 
> In the future, we should be very vigilant about this.  It's OK for some node
> in DFG IR to emit bad code.  It's never OK for a node in DFG IR to claim
> that it produces TOP as its output, since that pessimizes all downstream
> nodes.

Sounds good. We should probably do the same thing for tryGetById. 
Maybe we already have a bug open for that. If not, I can create one.