Bug 220221 – Improve error message for uninitialized |this| in derived constructor

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Improve error message for uninitialized |this| in derived constructor

[Alexey Shvayka]

Created attachment 416882 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416882 [details]
Patch

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

We found that our bytecode generation is too generic, and emitting many bytecodes. This prevents us from performing inlining in https://bugs.webkit.org/show_bug.cgi?id=220204.
So, can you change this without increasing bytecode size?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2852
> +void BytecodeGenerator::emitTDZCheckForThisRegisterInDerivedConstructor()
> +{
> +    Ref<Label> thisIsNotInTDZ = newLabel();
> +    emitJumpIfFalse(emitIsEmpty(newTemporary(), thisRegister()), thisIsNotInTDZ.get());
> +    emitThrowReferenceError("'super()' must be called in derived constructor before accessing |this| or returning non-object."_s);
> +    emitLabel(thisIsNotInTDZ.get());

The problem is that,

[Yusuke Suzuki]

Comment on attachment 416882 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2852
>> +    emitLabel(thisIsNotInTDZ.get());
> 
> The problem is that,

The problem is that, previously, it was check_tdz. But now, it is more bytecodes. This will cause performance/memory-usage problem because it increases bytecode size.
I think that,

1. Since this is super rare case in practice, we should not emit bytecodes much for that pattern.
2. I think we should switch to the different solution: we should throw appropriate error message ad-hocly when we throw an error. Like, when we failed with check_tdz, we should investigate what causes it by checking bytecode stream / source code etc. and throws appropriate error.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #3)
> The problem is that, previously, it was check_tdz. But now, it is more
> bytecodes. This will cause performance/memory-usage problem because it
> increases bytecode size.

Yes, there is an increase in bytecode size, but only for derived constructors, which are super rare.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #4)
> (In reply to Yusuke Suzuki from comment #3)
> > The problem is that, previously, it was check_tdz. But now, it is more
> > bytecodes. This will cause performance/memory-usage problem because it
> > increases bytecode size.
> 
> Yes, there is an increase in bytecode size, but only for derived
> constructors, which are super rare.

Please see BytecodeGenerator::ensureThis(): emitTDZCheckForThisRegisterInDerivedConstructor(), that emits additional bytecodes, is guarded by `if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext())`.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #5)
> Please see BytecodeGenerator::ensureThis():
> emitTDZCheckForThisRegisterInDerivedConstructor(), that emits additional
> bytecodes, is guarded by `if (constructorKind() == ConstructorKind::Extends
> || isDerivedConstructorContext())`.

Discussed this on Slack: we should avoid having super rare things, like TDZ errors, in bytecode, and use slow paths instead. Emitting many bytecodes for error checking can penalize CodeBlock heuristics, telling DFG / FTL that the function is too large.

[Alexey Shvayka]

Created attachment 416887 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 416887 [details]
Patch

r=me

[EWS]

Committed r271120: <https://trac.webkit.org/changeset/271120>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416887 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72771016>