[webkit-reviews] review granted: [Bug 193308] [JSC] Global lexical bindings can shadow global variables if it is `configurable = true` : [Attachment 358929] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 193308: [JSC] Global lexical bindings can shadow global variables if it is
`configurable = true`
https://bugs.webkit.org/show_bug.cgi?id=193308

Attachment 358929: Patch

https://bugs.webkit.org/attachment.cgi?id=358929&action=review




--- Comment #23 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 358929
  --> https://bugs.webkit.org/attachment.cgi?id=358929
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:26
> +	   In JSC, we cache GlobalProperty resolve type and its associated
information in op_resolve_type, op_get_from_scope, op_put_to_scope, and
op_profile_type.

This patch doesn't deal with profile_type.

> Source/JavaScriptCore/ChangeLog:37
> +	   to GlobalLexicalVar (or Dynamic precisely). So, the subsequent LLInt
code just works well.

you should explain the Dynamic for "const" thing

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2698
> +		       metadata.localScopeDepth = op.depth;

Is this actually correct? This seems wrong, since previously, if the depth was
N, we should now be at N-1. However, I think this will make us stay at N. (I
don't think we end up actually using this for global lexical variables, but
it's weird.)

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745
> +			   resolveType =
needsVarInjectionChecks(originalResolveType) ?
GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar;

Why is this needed? Shouldn't this be an assert?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6191
> +		      
m_graph.globalProperties().addLazily(DesiredGlobalProperty(globalObject,
identifierNumber));

I think we should also do this for get_from_scope and put_to_scope. I'm not
sure if we break this rule today (I don't think so), but I don't think we
should rely on resolve_scope always happening before
put_to_scope/get_from_scope

> Source/JavaScriptCore/dfg/DFGPlan.cpp:574
> +bool Plan::syncInMainThreadForValidation()

I'm not a fan of this name. Maybe "isStillValid" is enough? Or,
"isStillValidOnMainThread" or something similar.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:771
> +	   // Here, we get constant value for GlobalProperty and
GlobalPropertyWithVarInjectionChecks.
> +	   // This is because UnresolvedProperty already has a resolveType
check. If a resolveType is changed
> +	   // to GlobalLexicalVar, we do not enter the code for GlobalProperty.

Not sure this comment is really needed.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1716
> +	   // otherwise lexical bindings can change the result of GlobalVar
queries too.

Might be worth one more sentence saying we won't be able to declare a global
lexical variable w/ this same name b/c configurable=false