Bug 185995 – for-in loops should preserve and restore the TDZ stack for each of its internal loops.

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

[Mark Lam]

This is because there's not guarantee that any of the loop bodies will have executed.  Hence, there's no guarantee that the TDZ variables will have beeb initialized after each loop body.

<rdar://problem/40173142>

[Mark Lam]

Created attachment 341342 [details]
proposed patch.

[Saam Barati]

Comment on attachment 341342 [details]
proposed patch.

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

r=me

> JSTests/stress/regress-185995.js:5
> +        "var list = { 'a' : 5 };" + "\n" +
> +        "for(const { x = x } in list)" + "\n" +
> +        "    x();";

Why not just write this code out in the try instead of eval?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3079
> +    BytecodeGenerator::PreservedTDZStack preservedTDZStack;
> +    generator.preserveTDZStack(preservedTDZStack);

Not a big deal, but I think it'd make sense if this just were an RAII. Could just be a SetForScope<....> and you can use it in each scope below.

[Mark Lam]

Comment on attachment 341342 [details]
proposed patch.

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

>> JSTests/stress/regress-185995.js:5
>> +        "    x();";
> 
> Why not just write this code out in the try instead of eval?

Because in my mind, I conflated ReferenceError with SyntaxError.  I'll switch to doing it in the try block.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3079
>> +    generator.preserveTDZStack(preservedTDZStack);
> 
> Not a big deal, but I think it'd make sense if this just were an RAII. Could just be a SetForScope<....> and you can use it in each scope below.

I'll re-write it as an RAII BytecodeGenerator::TDZStackPreservationScope.

[Mark Lam]

Comment on attachment 341342 [details]
proposed patch.

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

>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3079
>>> +    generator.preserveTDZStack(preservedTDZStack);
>> 
>> Not a big deal, but I think it'd make sense if this just were an RAII. Could just be a SetForScope<....> and you can use it in each scope below.
> 
> I'll re-write it as an RAII BytecodeGenerator::TDZStackPreservationScope.

On second thought, I'll keep it as is.  This saves on the amount of mallocs and copying of the TDZ stack ... not that the stack should be all that big.

[Mark Lam]

Created attachment 341345 [details]
patch for landing.

[Mark Lam]

Created attachment 341346 [details]
patch for landing.

[Mark Lam]

Comment on attachment 341346 [details]
patch for landing.

Thanks for the review.  Landing this now.

[WebKit Commit Bot]

Comment on attachment 341346 [details]
patch for landing.

Clearing flags on attachment: 341346

Committed r232219: <https://trac.webkit.org/changeset/232219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.