Bug 151113 – calling super() a second time in a constructor should throw

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

See discussion here:
https://esdiscuss.org/topic/duplicate-super-call-behaviour
This seems to have interesting behavior. We should throw, but only after calling the super class's constructor a second time.

[Ryosuke Niwa]

According to the spec, we need to make op_create_this throw when "this" is already 
"filled".

[GSkachkov]

I'm working on patch for this issue. 
I've created new op code - op_is_empty to check if thisRegister is empty or unassigned before assign value returned from parent constructor(super). I implemented op_is_empty in LLInt and currently working on implementing it in JITOpcodes & DFG

[Ryosuke Niwa]

(In reply to comment #2)
> I'm working on patch for this issue. 
> I've created new op code - op_is_empty to check if thisRegister is empty or
> unassigned before assign value returned from parent constructor(super). I
> implemented op_is_empty in LLInt and currently working on implementing it in
> JITOpcodes & DFG

Why do you need a new op code for this?  Can't we just use check_tdz?

[Saam Barati]

(In reply to comment #3)
> (In reply to comment #2)
> > I'm working on patch for this issue. 
> > I've created new op code - op_is_empty to check if thisRegister is empty or
> > unassigned before assign value returned from parent constructor(super). I
> > implemented op_is_empty in LLInt and currently working on implementing it in
> > JITOpcodes & DFG
> 
> Why do you need a new op code for this?  Can't we just use check_tdz?

Doing that would introduce a synthetic try/catch around something we expect
to often be true.

[GSkachkov]

Created attachment 276044 [details]
Patch

Upload to test in differen ARCHs, more tests will be in next patch

[GSkachkov]

Created attachment 276047 [details]
Patch

Fix build for 32bit

[GSkachkov]

Created attachment 276048 [details]
Patch

Fix builds

[Build Bot]

Comment on attachment 276048 [details]
Patch

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

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html

[Build Bot]

Created attachment 276055 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276048 [details]
Patch

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

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 276048 [details]
Patch

Attachment 276048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1122834

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html

[Build Bot]

Created attachment 276059 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276048 [details]
Patch

Attachment 276048 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1122851

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html

[Build Bot]

Created attachment 276061 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 276086 [details]
Patch

Fix builds

[GSkachkov]

Created attachment 276090 [details]
Patch

New patch with new tests

[Ryosuke Niwa]

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

[GSkachkov]

Created attachment 276169 [details]
Patch

Added more tests

[Saam Barati]

Comment on attachment 276169 [details]
Patch

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

LGTM besides my suggestions in abstract interpreter
code gen.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:770
> +        generator.emitThrowReferenceError(ASCIILiteral("'super()' can't be called more than once in constructor."));

"in constructor" => "in a constructor"

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967
> +    case IsEmpty:

I think we can more aggressively constant fold this based on type information as well.
We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it.
We can fold to true if the speculated type is equal to SpecEmpty.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4405
> +    case IsEmpty: {        

This code is more easily written as a compare instruction.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4410
> +    case IsEmpty: {

I think the below code is subtly wrong because you just or the ValueTrue/False disregarding junk old
values in the register. But, regardless of that, this code is better written as a compare instruction 
+ "or ValueFalse". I believe we use this paradigm in other code in the DFG.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:179
> +void JIT::emit_op_is_empty(Instruction* currentInstruction)

Ditto with compare instruction.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:294
> +void JIT::emit_op_is_empty(Instruction* currentInstruction)

Ditto with compare instruction.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1214
> +_llint_op_is_empty:

Ditto

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1102
> +_llint_op_is_empty:

Ditto.

[GSkachkov]

Created attachment 276261 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 276169 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967
>> +    case IsEmpty:
> 
> I think we can more aggressively constant fold this based on type information as well.
> We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it.
> We can fold to true if the speculated type is equal to SpecEmpty.

I'll try to play, hope in with next patch will be fixed this comment

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4405
>> +    case IsEmpty: {        
> 
> This code is more easily written as a compare instruction.

Hope I did in way that you mean, but I don't know if it correct work in 32bit

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4410
>> +    case IsEmpty: {
> 
> I think the below code is subtly wrong because you just or the ValueTrue/False disregarding junk old
> values in the register. But, regardless of that, this code is better written as a compare instruction 
> + "or ValueFalse". I believe we use this paradigm in other code in the DFG.

Refactored

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:179
>> +void JIT::emit_op_is_empty(Instruction* currentInstruction)
> 
> Ditto with compare instruction.

Refactored

>> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:294
>> +void JIT::emit_op_is_empty(Instruction* currentInstruction)
> 
> Ditto with compare instruction.

Refactored

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1214
>> +_llint_op_is_empty:
> 
> Ditto

Refactored

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1102
>> +_llint_op_is_empty:
> 
> Ditto.

Refactored

[Saam Barati]

Comment on attachment 276261 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4414
> +        m_jit.compare64(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr());

Yup, this is what I meant.

I think this is better written as:
m_jit.comparePtr/compare64(JITCompiler::Equal, value.gpr(), TrustedImmPtr/TrustedImm32(JSValue()), result.gpr())

> Source/JavaScriptCore/jit/JITOpcodes.cpp:185
> +    compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0);

ditto here.

[Build Bot]

Comment on attachment 276261 [details]
Patch

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

New failing tests:
js/class-syntax-super.html

[Build Bot]

Created attachment 276267 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276261 [details]
Patch

Attachment 276261 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1143617

New failing tests:
js/class-syntax-super.html

[Build Bot]

Created attachment 276269 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276261 [details]
Patch

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

New failing tests:
js/class-syntax-super.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 276261 [details]
Patch

Attachment 276261 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1143813

New failing tests:
js/class-syntax-super.html

[Build Bot]

Created attachment 276279 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 276318 [details]
Patch

fix tests

[GSkachkov]

Created attachment 276338 [details]
Patch

fix build&tests

[GSkachkov]

Created attachment 276498 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 276261 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4414
>> +        m_jit.compare64(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr());
> 
> Yup, this is what I meant.
> 
> I think this is better written as:
> m_jit.comparePtr/compare64(JITCompiler::Equal, value.gpr(), TrustedImmPtr/TrustedImm32(JSValue()), result.gpr())

done

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:185
>> +    compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0);
> 
> ditto here.

done

[GSkachkov]

Comment on attachment 276169 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:967
>>> +    case IsEmpty:
>> 
>> I think we can more aggressively constant fold this based on type information as well.
>> We can fold to false if the speculated type for child1 doesn't have SpecEmpty in it.
>> We can fold to true if the speculated type is equal to SpecEmpty.
> 
> I'll try to play, hope in with next patch will be fixed this comment

It seems that I managed to do this.

[Keith Miller]

Comment on attachment 276498 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        Current patch implement check if 'super()' was called in constructor 
> +        more than once and raise RuntimeError if 'super()' called second time. 
> +        According to spec we need to raise error just after second super() 
> +        is finished, and before new this is assign 
> +        https://esdiscuss.org/topic/duplicate-super-call-behaviour. 
> +        To implement this behavior was introduced new op code - op_is_empty
> +        that is used to check if 'this' is empty.

I think the phrasing of the changelog would be clearer as:

Currently, our implementation checks if 'super()' was called in a constructor more than once and raises a RuntimeError before the second call. According to the spec we need to raise an error just after the second super() is finished and before the new 'this' is assigned https://esdiscuss.org/topic/duplicate-super-call-behaviour. To implement this behavior this patch adds a new op code, op_is_empty, that is used to check if 'this' is empty.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1097
> +            if (!(child.m_type & ~SpecEmpty)) {
> +                setConstant(node, jsBoolean(true));
> +                constantWasSet = true;
> +                break;
> +            }

I think this case is wrong. If the abstract interpreter has no information then child.m_type will be SpecNone (0) and this case will convert the IsEmpty check into a constant. I think a correct condition would be "child.m_type && !(child.m_type & ~SpecEmpty)".

[GSkachkov]

Created attachment 276660 [details]
Patch

Fix the latest comments

[Saam Barati]

Comment on attachment 276660 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1101
> +            if (!(child.m_type & SpecEmpty)) {

You also want this one to check child.m_type && ...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4419
> +        m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr());

I think this is probably correct, but I think it's way clearer as:
m_jit.comparePtr(Equal, value.gpr(), TrustedImm64(JSValue::encode(JSValue())), result.gpr())

> Source/JavaScriptCore/jit/JITOpcodes.cpp:185
> +    compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0);

ditto.

[GSkachkov]

Comment on attachment 276498 [details]
Patch

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

Thanks for review

>> Source/JavaScriptCore/ChangeLog:14
>> +        that is used to check if 'this' is empty.
> 
> I think the phrasing of the changelog would be clearer as:
> 
> Currently, our implementation checks if 'super()' was called in a constructor more than once and raises a RuntimeError before the second call. According to the spec we need to raise an error just after the second super() is finished and before the new 'this' is assigned https://esdiscuss.org/topic/duplicate-super-call-behaviour. To implement this behavior this patch adds a new op code, op_is_empty, that is used to check if 'this' is empty.

Yeah, fixed. English is not my mother tongue language. Thanks!

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1097
>> +            }
> 
> I think this case is wrong. If the abstract interpreter has no information then child.m_type will be SpecNone (0) and this case will convert the IsEmpty check into a constant. I think a correct condition would be "child.m_type && !(child.m_type & ~SpecEmpty)".

Done

[GSkachkov]

Comment on attachment 276660 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1101
>> +            if (!(child.m_type & SpecEmpty)) {
> 
> You also want this one to check child.m_type && ...

Done. What do I need to do to cover this code by tests?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4419
>> +        m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(ValueEmpty), result.gpr());
> 
> I think this is probably correct, but I think it's way clearer as:
> m_jit.comparePtr(Equal, value.gpr(), TrustedImm64(JSValue::encode(JSValue())), result.gpr())

Done by following code m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr())

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:185
>> +    compare64(Equal, regT0, TrustedImm32(ValueEmpty), regT0);
> 
> ditto.

Done

[GSkachkov]

Comment on attachment 276660 [details]
Patch

Thanks for review

[Csaba Osztrogonác]

Just to document, it was landed in https://trac.webkit.org/changeset/199712

[Csaba Osztrogonác]

and it caused a serious regression on 32 bit platforms, see bug156740 for details.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156741

[Saam Barati]

Comment on attachment 276660 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1221
> +    storei t3, PayloadOffset[cfr, t0, 8]

You need to store the Boolean tag as well here.

[GSkachkov]

Created attachment 276749 [details]
Patch

Fix tests on 32bit

[Saam Barati]

Comment on attachment 276749 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
> +                constantWasSet = true;

Doesn't setConstant already assign to this variable for us?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440
> +        m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr());

This should be TrustedImm64

[GSkachkov]

Comment on attachment 276749 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
>> +                constantWasSet = true;
> 
> Doesn't setConstant already assign to this variable for us?

I did as do in another 'case's. Anyway constantWasSet is local variable declared just above of the switch/case construction, but In setConstant we assign m_foundConstants of m_state object.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440
>> +        m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr());
> 
> This should be TrustedImm64

There is no function  for TrustedImm64 in MacroAssembler.h, only for TrustedImm32.  Do I need create new one?
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/MacroAssembler.h#L998

[Saam Barati]

(In reply to comment #49)
> Comment on attachment 276749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276749&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
> >> +                constantWasSet = true;
> > 
> > Doesn't setConstant already assign to this variable for us?
> 
> I did as do in another 'case's. Anyway constantWasSet is local variable
> declared just above of the switch/case construction, but In setConstant we
> assign m_foundConstants of m_state object.
> 
> >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4440
> >> +        m_jit.comparePtr(JITCompiler::Equal, value.gpr(), TrustedImm32(JSValue::encode(JSValue())), result.gpr());
> > 
> > This should be TrustedImm64
> 
> There is no function  for TrustedImm64 in MacroAssembler.h, only for
> TrustedImm32.  Do I need create new one?
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/assembler/
> MacroAssembler.h#L998
Right. I think you want TrustedImmPtr which is 64 bits on 64bit architectures.

[GSkachkov]

Created attachment 276848 [details]
Patch

Fix comments

[Saam Barati]

Ok my advice was wrong.
We don't have 64bit immediate compares on the architectures we care about.
We just want comparePtr or compare64(Equal, reg, TrustedImm32(0), resultReg)
(The imm value is sign extended).

And our MASM will do the right thing and convert this into a test of reg on itself.

[GSkachkov]

Created attachment 277388 [details]
Patch

Double check build

[GSkachkov]

Committed r200102: <http://trac.webkit.org/changeset/200102>
Patch is landed and issue can be closed

[GSkachkov]

Comment on attachment 277388 [details]
Patch

Clear flag on attachment

[Saam Barati]

marking as resolved.