Bug 171274 – ASSERTION FAILED: generator.isConstructor() || generator.derivedContextType() == DerivedContextType::DerivedConstructorContext

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[André Bargull]

svn revision: 215724

Test case:
---
new class extends Object {
  constructor() {
    var f = async(a=super())=>{ super() }
    f()
  }
}
---


Asserts with:
---
ASSERTION FAILED: generator.isConstructor() || generator.derivedContextType() == DerivedContextType::DerivedConstructorContext
---


Stacktrace:
---
#0  0x00007ffff6c4b5af in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:292
#1  0x00007ffff6045f40 in JSC::FunctionCallValueNode::emitBytecode (this=0x7fffad7f4188, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:822
#2  0x00007ffff6018bdd in JSC::BytecodeGenerator::emitNodeInTailPosition (this=0x7fffee78e500, dst=0x7fffee78e580, n=0x7fffad7f4188) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:494
#3  0x00007ffff6018ab3 in JSC::BytecodeGenerator::emitNode (this=0x7fffee78e500, dst=0x7fffee78e580, n=0x7fffad7f4188) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:483
#4  0x00007ffff6053b9a in JSC::ExprStatementNode::emitBytecode (this=0x7fffad7f41d8, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2507
#5  0x00007ffff6060e79 in JSC::BytecodeGenerator::emitNodeInTailPosition (this=0x7fffee78e500, dst=0x7fffee78e580, n=0x7fffad7f41d8) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:467
#6  0x00007ffff6061427 in JSC::SourceElements::emitBytecode (this=0x7fffad7f4150, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2464
#7  0x00007ffff6053a98 in JSC::BlockNode::emitBytecode (this=0x7fffad7f4210, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2484
#8  0x00007ffff6060e79 in JSC::BytecodeGenerator::emitNodeInTailPosition (this=0x7fffee78e500, dst=0x7fffee78e580, n=0x7fffad7f4210) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:467
#9  0x00007ffff6061427 in JSC::SourceElements::emitBytecode (this=0x7fffad7f4140, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2464
#10 0x00007ffff6061506 in JSC::ScopeNode::emitStatementsBytecode (this=0x7fffee7bd4b0, generator=..., dst=0x7fffee78e580) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3452
#11 0x00007ffff605a580 in JSC::FunctionNode::emitBytecode (this=0x7fffee7bd4b0, generator=...) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3609
#12 0x00007ffff5ff66ee in JSC::BytecodeGenerator::generate (this=0x7fffee78e500) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:125
#13 0x00007ffff5fef0bb in JSC::BytecodeGenerator::generate<JSC::FunctionNode*, JSC::UnlinkedFunctionCodeBlock*&, JSC::DebuggerMode&, JSC::VariableEnvironment const*> (vm=...)
    at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:369
#14 0x00007ffff5fec9b8 in JSC::generateUnlinkedFunctionCodeBlock (vm=..., executable=0x7fffadf74260, source=..., kind=JSC::CodeForCall, debuggerMode=JSC::DebuggerOff, 
    functionKind=JSC::UnlinkedNormalFunction, error=..., parseMode=JSC::SourceParseMode::AsyncArrowFunctionBodyMode) at ../../Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:71
#15 0x00007ffff5fed781 in JSC::UnlinkedFunctionExecutable::unlinkedCodeBlockFor (this=0x7fffadf74260, vm=..., source=..., specializationKind=JSC::CodeForCall, debuggerMode=JSC::DebuggerOff, error=..., 
    parseMode=JSC::SourceParseMode::AsyncArrowFunctionBodyMode) at ../../Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:212
...
---


Probably related null-pointer crashes with:
---
new class extends Object {
  constructor() {
    var f = async()=>{ super() }
    f()
  }
}
---

and:
---
new class extends Object {
  constructor() {
    var f = async(a=super())=>{  }
    f(0)
  }
}
---

[GSkachkov]

I'll take a look. It seems that it lost constructor context, during execution of the async arrow function. I would expect that there should be SyntaxError, but spec does not said that, so I'll try to fix it.

[GSkachkov]

I prepared fix for both cases of superCall in arguments and async function body, later upload patch with tests. 
There is a small issue that I little bit worried. In case of using superCall in body of async function, we trying to put 'this' to @generatorThis property of generator object, but 'this' is empty because superCall was not called before, and we just fail silently in following line. 
https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/JSObject.h#L1450
I think we need add there some ASSERT that value is not empty. For now it is not part of this patch. Do I need add such assert in this patch, or create separate issue?

[GSkachkov]

Created attachment 308718 [details]
Patch

Patch

[Build Bot]

Comment on attachment 308718 [details]
Patch

Attachment 308718 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3650857

New failing tests:
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-maximal-flush-validate-no-cjit
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit-b3o1
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-small-pool
stress/async-arrow-functions-lexical-binding-in-class.js.no-ftl
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-validate-phases
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-put-stack-validate
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-b3o1
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager-no-cjit-validate
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-collect-continuously
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-validate-sampling-profiler
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-inline-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-llint
stress/async-arrow-functions-lexical-binding-in-class.js.default

[GSkachkov]

Comment on attachment 308718 [details]
Patch

Brocken tests

[GSkachkov]

I faced in error in binding this & superCall in arrow function https://bugs.webkit.org/show_bug.cgi?id=171543. Before I was sure that we always load `this` from virtual scope in arrow function within constructor.
I need to fix it first that back to this.

[GSkachkov]

Created attachment 309280 [details]
Patch

Patch, but need to await until https://bugs.webkit.org/show_bug.cgi?id=171543

[GSkachkov]

Created attachment 309286 [details]
Patch

Patch after rebase

[Build Bot]

Comment on attachment 309286 [details]
Patch

Attachment 309286 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3688675

New failing tests:
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-maximal-flush-validate-no-cjit
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit-b3o1
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-small-pool
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-put-stack-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-validate-phases
ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-b3o1
stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager-no-cjit-validate
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-inline-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-collect-continuously
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-validate-sampling-profiler
stress/async-arrow-functions-lexical-binding-in-class.js.no-ftl
stress/async-arrow-functions-lexical-binding-in-class.js.no-llint
stress/async-arrow-functions-lexical-binding-in-class.js.default

[Build Bot]

Comment on attachment 309286 [details]
Patch

Attachment 309286 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3688833

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html

[Build Bot]

Created attachment 309294 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[GSkachkov]

Created attachment 309295 [details]
Patch

Fix tests

[Saam Barati]

Comment on attachment 309295 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2851
> +    if (!(isDerivedConstructorContext() && m_codeBlock->parseMode() == SourceParseMode::AsyncArrowFunctionMode))

When do we do this?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
> +            emitLoadThisFromArrowFunctionLexicalEnvironment();

What happens if it’s never stored?

[GSkachkov]

Comment on attachment 309295 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2851
>> +    if (!(isDerivedConstructorContext() && m_codeBlock->parseMode() == SourceParseMode::AsyncArrowFunctionMode))
> 
> When do we do this?

Yes, we store it after calling 'super'
In FunctionCallValueNode::emitBytecode we do
if (generator.isDerivedConstructorContext() || doWeUseArrowFunctionInConstructor)
            generator.emitPutThisToArrowFunctionContextScope();

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
>> +            emitLoadThisFromArrowFunctionLexicalEnvironment();
> 
> What happens if it’s never stored?

We will receive TDZ error. Should be the same behavior as it is for common arrow function, that access 'this' before calling super.

[Yusuke Suzuki]

Comment on attachment 309295 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2851
>>> +    if (!(isDerivedConstructorContext() && m_codeBlock->parseMode() == SourceParseMode::AsyncArrowFunctionMode))
>> 
>> When do we do this?
> 
> Yes, we store it after calling 'super'
> In FunctionCallValueNode::emitBytecode we do
> if (generator.isDerivedConstructorContext() || doWeUseArrowFunctionInConstructor)
>             generator.emitPutThisToArrowFunctionContextScope();

If you do not set generatorThis, what happens with AsyncFunctionPrototype.js' asyncFunctionResume's @generatorThis access?

[Saam Barati]

Comment on attachment 309295 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
>>> +            emitLoadThisFromArrowFunctionLexicalEnvironment();
>> 
>> What happens if it’s never stored?
> 
> We will receive TDZ error. Should be the same behavior as it is for common arrow function, that access 'this' before calling super.

Do we store the empty value somewhere else?

[GSkachkov]

Comment on attachment 309295 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2851
>>>> +    if (!(isDerivedConstructorContext() && m_codeBlock->parseMode() == SourceParseMode::AsyncArrowFunctionMode))
>>> 
>>> When do we do this?
>> 
>> Yes, we store it after calling 'super'
>> In FunctionCallValueNode::emitBytecode we do
>> if (generator.isDerivedConstructorContext() || doWeUseArrowFunctionInConstructor)
>>             generator.emitPutThisToArrowFunctionContextScope();
> 
> If you do not set generatorThis, what happens with AsyncFunctionPrototype.js' asyncFunctionResume's @generatorThis access?

Hmm, I think @generatorThis has value `undefined`, when call generator.@generatorNext.@call(generator.@generatorThis with value @undefined, but this `undefined` value would not be used, because it intercepted and taken from virtual scope, when we access to thisValue

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
>>>> +            emitLoadThisFromArrowFunctionLexicalEnvironment();
>>> 
>>> What happens if it’s never stored?
>> 
>> We will receive TDZ error. Should be the same behavior as it is for common arrow function, that access 'this' before calling super.
> 
> Do we store the empty value somewhere else?

No I think we do not store empty value

[Saam Barati]

Comment on attachment 309295 [details]
Patch

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

>>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
>>>>> +            emitLoadThisFromArrowFunctionLexicalEnvironment();
>>>> 
>>>> What happens if it’s never stored?
>>> 
>>> We will receive TDZ error. Should be the same behavior as it is for common arrow function, that access 'this' before calling super.
>> 
>> Do we store the empty value somewhere else?
> 
> No I think we do not store empty value

How does the TDZ check work then?

[GSkachkov]

Comment on attachment 309295 [details]
Patch

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

>>>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4611
>>>>>> +            emitLoadThisFromArrowFunctionLexicalEnvironment();
>>>>> 
>>>>> What happens if it’s never stored?
>>>> 
>>>> We will receive TDZ error. Should be the same behavior as it is for common arrow function, that access 'this' before calling super.
>>> 
>>> Do we store the empty value somewhere else?
>> 
>> No I think we do not store empty value
> 
> How does the TDZ check work then?

Ohh I see why you mean. 
We create empty variable slot for `this` in virtual scope of ‘constructor’ in initializeArrowFunctionContextScopeIfNeeded method of the BytecodeGenerator, condition isThisUsedInInnerArrowFunction is true in case of async arrow function is true. 
When we access to `this` within async arrow function when we will try to load `this` from virtual scope and if we have not stored there any value before we would receive TDZ error (‘Cannot access uninitialized variable’).  
@generatorThis property of the generator that is taken during resume async function in butlins\AsyncFunctionPrototype.js within async arrow function in `constructor` has value `undefined`. When we access to `this` within async arrow function we don't use value from @generatorThis, because it intercepted by `ensureThis()` method,  and load `this` value from virtual scope of `constructor`

[GSkachkov]

Comment on attachment 309295 [details]
Patch

Hmm, I found one not css where TDZ error is not thrower, will investigate what`s wrong:
```
class ChildClass8 extends BaseClass {
    constructor() {
        const arr = async () => { let i  = this.id; super(); };
        arr();
    }
}
```

[GSkachkov]

Created attachment 311011 [details]
Patch

Add more tests & rebase

[Saam Barati]

Comment on attachment 311011 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 311011 [details]
Patch

Clearing flags on attachment: 311011

Committed r217577: <http://trac.webkit.org/changeset/217577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479868>

[GSkachkov]

*** Bug 172739 has been marked as a duplicate of this bug. ***