In the call method of the super class called through super ("super.call()") all properties of "this" is undefined. If the same method is called through "this" ("this.call()") the proper values are there. If the same method is renamed to log the problem disappears. This happened in Safari 11, but I can't choose that as a version above. Steps to reproduce: A fiddle showing the problem: https://jsfiddle.net/pohdferq/1/ Expected: The console log showing that the prop field is set to 'value' in every logged case Actual: The console log shows that the prop field is undefined in super.call but not in other cases. Build info: Version 11.0 (13604.1.38.1.6)
Is this a new problem in Safari 11, or was this happening with earlier versions too?
I checked, and saw this happen on 10.1.2 (12603.3.8) too.
Created attachment 323247 [details] Patch
Comment on attachment 323247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323247&action=review > Source/JavaScriptCore/parser/ASTBuilder.h:1353 > + if (!previousBaseWasSuper && (dot->identifier() == m_vm->propertyNames->builtinNames().callPublicName() || dot->identifier() == m_vm->propertyNames->builtinNames().callPrivateName())) > node = new (m_parserArena) CallFunctionCallDotNode(location, dot->base(), dot->identifier(), args, divot, divotStart, divotEnd, callOrApplyChildDepth); > - else if (dot->identifier() == m_vm->propertyNames->builtinNames().applyPublicName() || dot->identifier() == m_vm->propertyNames->builtinNames().applyPrivateName()) > + else if (!previousBaseWasSuper && (dot->identifier() == m_vm->propertyNames->builtinNames().applyPublicName() || dot->identifier() == m_vm->propertyNames->builtinNames().applyPrivateName())) Why not just pass a different |this| value when the base is super?
Comment on attachment 323247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323247&action=review >> Source/JavaScriptCore/parser/ASTBuilder.h:1353 >> + else if (!previousBaseWasSuper && (dot->identifier() == m_vm->propertyNames->builtinNames().applyPublicName() || dot->identifier() == m_vm->propertyNames->builtinNames().applyPrivateName())) > > Why not just pass a different |this| value when the base is super? If I understand correctly, both cases of base.apply and base.call generate different byte code of "FunctionCallDotNode". For example, here is the byte code of reported case with "super.call()": testSuper#ApV04K [ 0] enter [ 1] get_scope loc3 [ 3] mov loc4, loc3 [ 6] check_traps [ 7] get_by_id loc6, head3, PrivateSymbol.homeObject(@id0) predicting None [ 16] get_by_id loc7, loc6, __proto__(@id1) predicting None [ 25] mov loc6, loc7 [ 28] get_by_id_with_this loc8, loc6, this, call(@id2) predicting None [ 34] jneq_ptr loc8, 0 (0x10e7ec160), 22(->56) --> This check here changes the semantics of "super.call()" [ 39] mov loc9, loc6 [ 42] mov loc10, Undefined(const0) [ 45] call loc7, loc9, 1, 16 (this at loc10) status(Could Take Slow Path) Original; predicting None [ 54] jmp 14(->68) [ 56] mov loc10, loc6 [ 59] call loc7, loc8, 1, 16 (this at loc10) status(Could Take Slow Path) Original; predicting None [ 68] mov loc8, this [ 71] get_by_id loc9, head3, PrivateSymbol.homeObject(@id0) predicting None [ 80] get_by_id loc10, loc9, __proto__(@id1) predicting None [ 89] get_by_id_with_this loc6, loc10, loc8, log(@id3) predicting None [ 95] call loc6, loc6, 1, 14 (this at loc8) status(Could Take Slow Path) Original; predicting None [ 104] ret Undefined(const0) Here is the the byte code generated by proposed Patch: testSuper#ApV04K [ 0] enter [ 1] get_scope loc3 [ 3] mov loc4, loc3 [ 6] check_traps [ 7] mov loc8, this [ 10] get_by_id loc9, head3, PrivateSymbol.homeObject(@id0) predicting None [ 19] get_by_id loc10, loc9, __proto__(@id1) predicting None [ 28] get_by_id_with_this loc6, loc10, loc8, call(@id2) predicting None [ 34] call loc6, loc6, 1, 14 (this at loc8) status(Could Take Slow Path) Original; predicting None [ 43] mov loc8, this [ 46] get_by_id loc9, head3, PrivateSymbol.homeObject(@id0) predicting None [ 55] get_by_id loc10, loc9, __proto__(@id1) predicting None [ 64] get_by_id_with_this loc6, loc10, loc8, log(@id3) predicting None [ 70] call loc6, loc6, 1, 14 (this at loc8) status(Could Take Slow Path) Original; predicting None [ 79] ret Undefined(const0)
When they make a call, don't they provide a |this|? If so, I think you can pattern match what that |this| is inside bytecode generator. We don't want to give up our .call and .apply optimizations for super.call/super.apply
(In reply to Saam Barati from comment #6) > When they make a call, don't they provide a |this|? If so, I think you can > pattern match what that |this| is inside bytecode generator. We don't want > to give up our .call and .apply optimizations for super.call/super.apply I think I'm missing something here. Are these optimizations applied to super.call/super.apply? Checking the spec I couldn't find any case referring "call/apply" for super. https://tc39.github.io/ecma262/#sec-super-keyword-runtime-semantics-evaluation. What I have in mind is that they should be treat as any other "super.prop" access.
(In reply to Saam Barati from comment #6) > When they make a call, don't they provide a |this|? Yes, but now the |this| actually is homeObject.__proto__.
Comment on attachment 323247 [details] Patch r=me
Created attachment 323368 [details] Patch
Comment on attachment 323368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323368&action=review > Source/JavaScriptCore/ChangeLog:10 > + Bytecode generation for "super.call(...)" or "super.apply(...)" > + shouldn't be considered as CallFunctionCallDotNode or > + ApplyFunctionCallDotNode. Please explain why here
Created attachment 323372 [details] Patch For landing
Comment on attachment 323372 [details] Patch For landing Clearing flags on attachment: 323372 Committed r223175: <http://trac.webkit.org/changeset/223175>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34931958>