* 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>
<rdar://problem/21885916>
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.
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.
Created attachment 277416 [details] [TEST] Test Page
Created attachment 277600 [details] WIP It begins
*** Bug 157136 has been marked as a duplicate of this bug. ***
Created attachment 277684 [details] WIP
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.
Created attachment 277758 [details] WIP Needs 32-bit support then I think I'm done.
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.
Created attachment 277802 [details] WIP maybe 32-bit is done? Lets see what EWS thinks.
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.
Created attachment 277835 [details] WIP
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.
Created attachment 277872 [details] patch
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
Created attachment 277906 [details] benchmark results
Created attachment 278414 [details] patch for landing
Comment on attachment 278414 [details] patch for landing Clearing flags on attachment: 278414 Committed r200586: <http://trac.webkit.org/changeset/200586>
All reviewed patches have been landed. Closing bug.
Nice! We should update the inspector now (bug 157488).
(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.
(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.
landed follow up patch to make test run faster: http://trac.webkit.org/changeset/200632
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
(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.
(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.
(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.