Bug 193308 – [JSC] Global lexical bindings can shadow global variables if it is `configurable = true`

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

[Yusuke Suzuki]

When we construct our GlobalProperty, GlobalLexicalVar, etc. mechanism, we assumed that lexical binding cannot shadow global object's properties. But it is wrong. Wc can shadow it if it's configurable attribute is true.

Let's see the example.

// script1.js
function foo()
{
    bar = 4;
}
function foo2()
{
    return bar;
}
foo();
print(foo2()); // => 4

// script2.js
const bar = 5;
print(foo2());  // => 4, but it should be 5!

Once the reference to "bar" is configured as GlobalProperty, then, it attempts to load the value from JSGlobalObject.
However, `const bar = 5` can shadow it, and we should not use GlobalProperty no longer for that.

[Yusuke Suzuki]

The super simple solution is leveraging the existing VarInjectionCheck mechanism. But it is not acceptable since almost GlobalProperty now become GlobalPropertyWithVarInjectionCheck.

I think we can leverage the nature of GlobalProperty load.

1. GlobalProperty performs structure check before loading like IC.
2. Shadowing should be super rare.

We can discard the existing mis-cache by perform structure transition for JSGlobalObject. And in the slow path, we can transform GlobalProperty to GlobalLexicalVar if necessary.

[Yusuke Suzuki]

Discussed with Saam. We should start with the slow, but memory-efficient and non-tricky way. Once the problematic thing happens, we start iterating all the CodeBlocks and convert instructions related to GlobalProperty to GlobalLexicalVar.
And we also fire the watchpoint for a name to discard DFG and FTL code. These codes will be recompiled with GlobalLexicalVar configuration, it does not cause the jettison again with the same watchpoint for the same name.

[Yusuke Suzuki]

Created attachment 358776 [details]
Patch

[Yusuke Suzuki]

Created attachment 358777 [details]
Patch

[Yusuke Suzuki]

We need to have watchpoint part next.

[Yusuke Suzuki]

Created attachment 358811 [details]
WIP

I believe it starts working

[Yusuke Suzuki]

<rdar://problem/45546542>

[Yusuke Suzuki]

Created attachment 358827 [details]
WIP

[EWS Watchlist]

Comment on attachment 358827 [details]
WIP

Attachment 358827 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10701874

New failing tests:
stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager
stress/const-lexical-binding-shadow-existing-global-property.js.dfg-eager
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1
stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1
stress/const-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-small-pool
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-validate-sampling-profiler
stress/let-lexical-binding-shadow-existing-global-property.js.default
stress/let-lexical-binding-shadow-existing-global-property-baseline.js.default
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-b3o1
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-maximal-flush-validate-no-cjit
stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-validate-phases
stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-collect-continuously
stress/let-lexical-binding-shadow-existing-global-property.js.no-ftl
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-put-stack-validate
stress/let-lexical-binding-shadow-existing-global-property.js.no-llint
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-inline-validate
apiTests

[EWS Watchlist]

Comment on attachment 358827 [details]
WIP

Attachment 358827 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10702008

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 358845 [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.13.6

[Yusuke Suzuki]

Created attachment 358855 [details]
Patch

[EWS Watchlist]

Comment on attachment 358855 [details]
Patch

Attachment 358855 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10704352

New failing tests:
stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit
stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1
stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-validate-sampling-profiler
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-small-pool
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate
stress/let-lexical-binding-shadow-existing-global-property.js.default
stress/let-lexical-binding-shadow-existing-global-property-baseline.js.default
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-b3o1
stress/let-lexical-binding-shadow-existing-global-property.js.dfg-maximal-flush-validate-no-cjit
stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-validate-phases
stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-collect-continuously
stress/let-lexical-binding-shadow-existing-global-property.js.no-ftl
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-put-stack-validate
stress/let-lexical-binding-shadow-existing-global-property.js.no-llint
stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-inline-validate
apiTests

[Yusuke Suzuki]

Created attachment 358872 [details]
Patch

[Yusuke Suzuki]

Created attachment 358875 [details]
Patch

[Yusuke Suzuki]

Created attachment 358877 [details]
Patch

[Yusuke Suzuki]

Created attachment 358879 [details]
Patch

[Yusuke Suzuki]

I think this patch works well on 64bit environments.
The remaining things are,

1. Adding more tests. With eval(), FTL, DFG etc.
2. Adding 32bit supports.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #18)
> I think this patch works well on 64bit environments.
> The remaining things are,
> 
> 1. Adding more tests. With eval(), FTL, DFG etc.
> 2. Adding 32bit supports.

And adding op_profile_type support.

[Yusuke Suzuki]

Created attachment 358912 [details]
WIP

OK, the patch becomes mature. It is discovered that op_profile_type does not need any care. After adding more tests, it becomes ready.

[Yusuke Suzuki]

Created attachment 358927 [details]
Patch

[Yusuke Suzuki]

Created attachment 358929 [details]
Patch

[Saam Barati]

Comment on attachment 358929 [details]
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

[Yusuke Suzuki]

Comment on attachment 358929 [details]
Patch

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

>> 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.

Oops, this remained. Fixed.

>> 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

OK, added. "Dynamic" happens when we attempt to put a value into a const binding, and it should throw a type error.

>> 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.)

Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed.
To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory.
That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745
>> +                        resolveType = needsVarInjectionChecks(originalResolveType) ? GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar;
> 
> Why is this needed? Shouldn't this be an assert?

We calculate the resolve type from the global scope. So even if we had a scope which uses eval, we cannot find it in the above abstractResolve. But we do not have the actual scope chain here.
But if the eval scope existed, this information must be existed in the originalResolveType since the originalResolveType should be GlobalProperty or GlobalPropertyWithVarInjectionChecks.
So, we can use this information to correct the result here.

>> 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

Make sense. Fixed.

>> 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.

OK, changed to `isStillValidOnMainThread()`.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:771
>> +        // to GlobalLexicalVar, we do not enter the code for GlobalProperty.
> 
> Not sure this comment is really needed.

Dropped.

>> 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

Yeah, nice. Added.

[Yusuke Suzuki]

Created attachment 358942 [details]
Patch for landing

[Saam Barati]

Comment on attachment 358929 [details]
Patch

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

>>> 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.)
> 
> Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed.
> To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory.
> That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here.

Can we do the same in byte code linking to be consistent?

>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745
>>> +                        resolveType = needsVarInjectionChecks(originalResolveType) ? GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar;
>> 
>> Why is this needed? Shouldn't this be an assert?
> 
> We calculate the resolve type from the global scope. So even if we had a scope which uses eval, we cannot find it in the above abstractResolve. But we do not have the actual scope chain here.
> But if the eval scope existed, this information must be existed in the originalResolveType since the originalResolveType should be GlobalProperty or GlobalPropertyWithVarInjectionChecks.
> So, we can use this information to correct the result here.

Got it. Makes sense.

[Yusuke Suzuki]

Comment on attachment 358929 [details]
Patch

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

>>>> 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.)
>> 
>> Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed.
>> To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory.
>> That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here.
> 
> Can we do the same in byte code linking to be consistent?

We can set it in CodeBlock::finishCreation.

[Yusuke Suzuki]

Note that, if it affects on performance in the measurement, we can add an additional complexity to avoid it,

1. have a counter, and bump it once we introduce a lexical binding shadowing an existing one
2. when the counter is overflow, we can iterate CodeBlock and rewrite them as we are doing now.
3. based on counter, in the slow path, we can rewrite itself
4. in DFG and FTL, we check the watchpoint, and if it is invalidated, we start converting it to GlobalLexicalVar or, we make it ForceOSRExit at the moment.

[Yusuke Suzuki]

Committed r239879: <https://trac.webkit.org/changeset/239879>

[Yusuke Suzuki]

Committed r239898: <https://trac.webkit.org/changeset/239898>