Bug 177230 – Object properties are undefined in super.call() but not in this.call()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mihaly Lengyel]

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)

[Alexey Proskuryakov]

Is this a new problem in Safari 11, or was this happening with earlier versions too?

[Mihaly Lengyel]

I checked, and saw this happen on 10.1.2 (12603.3.8) too.

[Caio Lima]

Created attachment 323247 [details]
Patch

[Saam Barati]

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?

[Caio Lima]

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)

[Saam Barati]

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

[Caio Lima]

(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.

[Caio Lima]

(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__.

[Saam Barati]

Comment on attachment 323247 [details]
Patch

r=me

[Caio Lima]

Created attachment 323368 [details]
Patch

[Saam Barati]

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

[Caio Lima]

Created attachment 323372 [details]
Patch For landing

[WebKit Commit Bot]

Comment on attachment 323372 [details]
Patch For landing

Clearing flags on attachment: 323372

Committed r223175: <http://trac.webkit.org/changeset/223175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34931958>