Bug 220221

Summary: Improve error message for uninitialized |this| in derived constructor
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Alexey Shvayka 2021-01-01 11:50:42 PST
Improve error message for uninitialized |this| in derived constructor
Comment 1 Alexey Shvayka 2021-01-01 11:53:51 PST
Created attachment 416882 [details]
Patch
Comment 2 Yusuke Suzuki 2021-01-01 13:27:48 PST
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,
Comment 3 Yusuke Suzuki 2021-01-01 13:30:13 PST
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.
Comment 4 Alexey Shvayka 2021-01-01 13:33:19 PST
(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.
Comment 5 Alexey Shvayka 2021-01-01 13:36:34 PST
(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())`.
Comment 6 Alexey Shvayka 2021-01-01 20:04:54 PST
(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.
Comment 7 Alexey Shvayka 2021-01-01 20:05:05 PST
Created attachment 416887 [details]
Patch
Comment 8 Yusuke Suzuki 2021-01-02 11:23:12 PST
Comment on attachment 416887 [details]
Patch

r=me
Comment 9 EWS 2021-01-02 11:28:08 PST
Committed r271120: <https://trac.webkit.org/changeset/271120>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416887 [details].
Comment 10 Radar WebKit Bug Importer 2021-01-02 11:29:16 PST
<rdar://problem/72771016>