Bug 177230

Summary: Object properties are undefined in super.call() but not in this.call()
Product: WebKit Reporter: Mihaly Lengyel <mihaly.lengyel>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: adam, ap, buildbot, commit-queue, fpizlo, keith_miller, mark.lam, msaboff, saam, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: macOS 10.13   
Attachments:
Description Flags
Patch
none
Patch
saam: review+
Patch For landing none

Description Mihaly Lengyel 2017-09-20 07:28:06 PDT
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)
Comment 1 Alexey Proskuryakov 2017-09-20 10:31:31 PDT
Is this a new problem in Safari 11, or was this happening with earlier versions too?
Comment 2 Mihaly Lengyel 2017-09-21 03:26:42 PDT
I checked, and saw this happen on 10.1.2 (12603.3.8) too.
Comment 3 Caio Lima 2017-10-09 17:08:39 PDT
Created attachment 323247 [details]
Patch
Comment 4 Saam Barati 2017-10-09 17:13:57 PDT
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 5 Caio Lima 2017-10-09 18:36:02 PDT
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)
Comment 6 Saam Barati 2017-10-09 18:44:09 PDT
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
Comment 7 Caio Lima 2017-10-09 19:10:54 PDT
(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.
Comment 8 Caio Lima 2017-10-09 19:34:39 PDT
(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 9 Saam Barati 2017-10-10 16:28:48 PDT
Comment on attachment 323247 [details]
Patch

r=me
Comment 10 Caio Lima 2017-10-10 18:57:44 PDT
Created attachment 323368 [details]
Patch
Comment 11 Saam Barati 2017-10-10 19:04:42 PDT
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
Comment 12 Caio Lima 2017-10-10 19:35:02 PDT
Created attachment 323372 [details]
Patch For landing
Comment 13 WebKit Commit Bot 2017-10-11 05:59:39 PDT
Comment on attachment 323372 [details]
Patch For landing

Clearing flags on attachment: 323372

Committed r223175: <http://trac.webkit.org/changeset/223175>
Comment 14 WebKit Commit Bot 2017-10-11 05:59:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-10-11 06:01:28 PDT
<rdar://problem/34931958>