Summary: | Object properties are undefined in super.call() but not in this.call() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihaly Lengyel <mihaly.lengyel> | ||||||||
Component: | JavaScriptCore | Assignee: | 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
Mihaly Lengyel
2017-09-20 07:28:06 PDT
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. |