Assertion failure for super() call in arrow function default parameters

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[André Bargull]

SVN: rev200124
Build with: perl Tools/Scripts/build-jsc --gtk --debug


The following test case triggers this assertion error:
---
ASSERTION FAILED: !newTarget || newTarget.isConstructor()
---


Test case:
---
new class extends Array { constructor() { ((a = super())=>{})() } }
---


Stack trace:
---
#0  0x00007ffff6e289ac in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:322
#1  0x00007ffff6bbf077 in JSC::InternalFunction::createSubclassStructure (exec=0x7fffffffc9f0, newTarget=..., baseClass=0x7fffaedf4380) at ../../Source/JavaScriptCore/runtime/InternalFunction.cpp:100
#2  0x0000000000447887 in JSC::JSGlobalObject::arrayStructureForIndexingTypeDuringAllocation (this=0x7fffaede7900, exec=0x7fffffffc9f0, indexingType=3 '\003', newTarget=...)
    at ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:506
#3  0x00000000004478c5 in JSC::JSGlobalObject::arrayStructureForProfileDuringAllocation (this=0x7fffaede7900, exec=0x7fffffffc9f0, profile=0x0, newTarget=...)
    at ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:510
#4  0x00007ffff600afb9 in JSC::constructArray (exec=0x7fffffffc9f0, profile=0x0, globalObject=0x7fffaede7900, values=..., newTarget=...) at ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:766
#5  0x00007ffff6b2efd6 in JSC::constructArrayWithSizeQuirk (exec=0x7fffffffc9f0, args=..., newTarget=...) at ../../Source/JavaScriptCore/runtime/ArrayConstructor.cpp:101
#6  0x00007ffff6b2f03c in JSC::constructWithArrayConstructor (exec=0x7fffffffc9f0) at ../../Source/JavaScriptCore/runtime/ArrayConstructor.cpp:107
#7  0x00007ffff6a296aa in JSC::(anonymous namespace)::handleHostCall (execCallee=0x7fffffffc9f0, pc=0x7ffff0dd7530, callee=..., kind=JSC::CodeForConstruct)
    at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
#8  0x00007ffff6a2c930 in JSC::(anonymous namespace)::setUpCall (execCallee=0x7fffffffc9f0, pc=0x7ffff0dd7530, kind=JSC::CodeForConstruct, calleeAsValue=..., callLinkInfo=0x7ffff0dbdcc8)
    at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1178
#9  0x00007ffff6a2cee3 in JSC::(anonymous namespace)::genericCall (exec=0x7fffffffca70, pc=0x7ffff0dd7530, kind=JSC::CodeForConstruct) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1262
#10 0x00007ffff6a298d9 in JSC::(anonymous namespace)::llint_slow_path_construct (exec=0x7fffffffca70, pc=0x7ffff0dd7530) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1274
#11 0x00007ffff6a33b3d in llint_entry () at ../../Source/JavaScriptCore/runtime/Butterfly.h:58
#12 0x00007ffff6a33818 in llint_entry () at ../../Source/JavaScriptCore/runtime/Butterfly.h:58
#13 0x00007ffff6a33b4b in llint_entry () at ../../Source/JavaScriptCore/runtime/Butterfly.h:58
#14 0x00007ffff6a2d895 in vmEntryToJavaScript () at ../../Source/JavaScriptCore/runtime/Butterfly.h:58
#15 0x00007ffff69d46d2 in JSC::JITCode::execute (this=0x7ffff0d9c668, vm=0x7fffb09f1000, protoCallFrame=0x7fffffffcd30) at ../../Source/JavaScriptCore/jit/JITCode.cpp:80
#16 0x00007ffff6997f4f in JSC::Interpreter::execute (this=0x7ffff0def058, program=0x7fffaedfbf70, callFrame=0x7fffaede7940, thisObj=0x7fffaedba360)
    at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:960
#17 0x00007ffff6b849ad in JSC::evaluate (exec=0x7fffaede7940, source=..., thisValue=..., returnedException=...) at ../../Source/JavaScriptCore/runtime/Completion.cpp:106
#18 0x000000000043a120 in runInteractive (globalObject=0x7fffaede7900) at ../../Source/JavaScriptCore/jsc.cpp:2083
#19 0x000000000043abcb in runJSC (vm=0x7fffb09f1000, options=...) at ../../Source/JavaScriptCore/jsc.cpp:2244
#20 0x000000000043b0a1 in jscmain (argc=1, argv=0x7fffffffdbb8) at ../../Source/JavaScriptCore/jsc.cpp:2293
#21 0x000000000043967f in main (argc=1, argv=0x7fffffffdbb8) at ../../Source/JavaScriptCore/jsc.cpp:1947
---

[Radar WebKit Bug Importer]

<rdar://problem/26004822>

[GSkachkov]

Good catch!
It seems that we mismatch the scope when we resolve scope for 'super()'. There is byte code for arrow function in little bit modified test:
----
new class extends Array { constructor() { var abc = 0;  ((a = super())=>{ debug(abc);})() } }
----

----
[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] mov               loc6, <JSValue()>(const1)
[   9] mov               loc7, arg1
[  12] is_undefined      loc8, arg1
[  15] jfalse            loc8, 80(->95)
[  18] resolve_scope     loc9, loc3, PrivateSymbol.derivedConstructor(@id0), <ClosureVar>, 0, 0x1077dbd60
[  25] get_from_scope    loc9, loc9, PrivateSymbol.derivedConstructor(@id0), 2051<ThrowIfNotFound|ClosureVar|NotInitialization>, 3    predicting None
[  33] get_by_id         loc9, loc9, __proto__(@id1)    predicting None
[  42] mov               loc10, loc5
[  45] construct         loc7, loc9, 1, 16 status(Could Take Slow Path)    predicting None
[  54] resolve_scope     loc11, loc3, this(@id2), <ClosureVar>, 0, 0x1077dbd60
[  61] get_from_scope    this, loc11, this(@id2), 1050627<DoNotThrowIfNotFound|ClosureVar|NotInitialization>, 1    predicting None
[  69] is_empty          loc11, this
[  72] jtrue             loc11, 6(->78)
[  75] throw_static_error String (atomic) (identifier): 'super()' can't be called more than once in a constructor., ID: 4(const2), true
[  78] mov               this, loc7
[  81] resolve_scope     loc11, loc3, this(@id2), <ClosureVar>, 0, 0x1077dbd60
[  88] put_to_scope      loc11, this(@id2), this, 2051<ThrowIfNotFound|ClosureVar|NotInitialization>, <structure>, 1
[  95] mov               loc6, loc7
[  98] resolve_scope     loc7, loc3, PrivateSymbol.newTargetLocal(@id3), <ClosureVar>, 0, 0x1077dbd60
[ 105] get_from_scope    loc5, loc7, PrivateSymbol.newTargetLocal(@id3), 2051<ThrowIfNotFound|ClosureVar|NotInitialization>, 2    predicting None
[ 113] resolve_scope     loc10, loc3, debug(@id4), <GlobalProperty>, 2, 0x1077df900
[ 120] get_from_scope    loc7, loc10, debug(@id4), 2048<ThrowIfNotFound|GlobalProperty|NotInitialization>, 120    predicting None
[ 128] resolve_scope     loc9, loc3, abc(@id5), <ClosureVar>, 0, 0x1077dbd60
[ 135] get_from_scope    loc9, loc9, abc(@id5), 2051<ThrowIfNotFound|ClosureVar|NotInitialization>, 0    predicting None
[ 143] call              loc7, loc7, 2, 16 status(Could Take Slow Path)    Original; predicting None
[ 152] ret               Undefined(const3)
----

line [18]-resolve scope for 'super()' and [128] resolve scope for 'abc', do resolve with the same scope 0x1077dbd60, but should with different scopes

[GSkachkov]

I'll try to make patch with fix soon.

[GSkachkov]

As for now I see that fix will be more complicated, because before idea was that we load all arrow function bound variables just after all function's arguments are prepared -> after initializeDefaultParameterValuesAndSetupFunctionScopeStack, but to cover current issue we need move initialization of arrow function variable before we trying initialize default parameters.

[Saam Barati]

(In reply to comment #4)
> As for now I see that fix will be more complicated, because before idea was
> that we load all arrow function bound variables just after all function's
> arguments are prepared -> after
> initializeDefaultParameterValuesAndSetupFunctionScopeStack, but to cover
> current issue we need move initialization of arrow function variable before
> we trying initialize default parameters.

This seems like the correct solution to me. We should probably write tests
also for 'this' and 'arguments'. Is there a good reason why
this needs to happen after we initialize default parameter values?

[GSkachkov]

Created attachment 278425 [details]
Patch

[Saam Barati]

I see we have the following comment:

    // Loading |this| inside an arrow function must be done after initializeDefaultParameterValuesAndSetupFunctionScopeStack()
    // because that function sets up the SymbolTable stack and emitLoadThisFromArrowFunctionLexicalEnvironment()
    // consults the SymbolTable stack


Is that comment correct though? I don't see why we need to consult the local symbol table stack here because we should always be doing non-local resolution.

[Saam Barati]

Comment on attachment 278425 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278425&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:574
> +    m_forceLoadThisFromArrowFunctionLexicalEnvironment = false;

Why do we need this?

[GSkachkov]

(In reply to comment #7)
> I see we have the following comment:
> 
>     // Loading |this| inside an arrow function must be done after
> initializeDefaultParameterValuesAndSetupFunctionScopeStack()
>     // because that function sets up the SymbolTable stack and
> emitLoadThisFromArrowFunctionLexicalEnvironment()
>     // consults the SymbolTable stack
> 
> 
> Is that comment correct though? I don't see why we need to consult the local
> symbol table stack here because we should always be doing non-local
> resolution.

Oh, I've copy pasted comments with if condition. 
I can be wrong, but if we do emitLoadThisFromArrowFunctionLexicalEnvironment before initializeDefaultParameterValuesAndSetupFunctionScopeStack, we resolved to wrong scope

[GSkachkov]

(In reply to comment #8)
> Comment on attachment 278425 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278425&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:574
> > +    m_forceLoadThisFromArrowFunctionLexicalEnvironment = false;
> 
> Why do we need this?

There is a case: 
var f = function () { return (a=this)=>{ return a; };};
var result = f.call({a:'data'})();

So just moving up loading 'this' for arrow function, broke several tests and to cover snipped, we need load 'this' twice before initializeDefaultParameterValuesAndSetupFunctionScopeStack and after. To prevent this I added parameter that allow load 'this' from arrow function scope instead of thisRegister when we access to 'this'. Currently I'm playing with loading 'this' to avoid using this m_forceLoadThisFromArrowFunctionLexicalEnvironment flag.

[GSkachkov]

Created attachment 278514 [details]
Patch

New version of the fix

[Saam Barati]

Comment on attachment 278514 [details]
Patch

LGTM.
Can you also add tests for TDZ with super(), 'this', super.getter, super.setter, etc, inside default parameter expressions

[GSkachkov]

New tests added

Patch landed r200824 <https://trac.webkit.org/changeset/200824>