Bug 87755 – Fix scoping bug involving eval, with, and optimized non-locals

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Andy Wingo]

Bug 76285 introduced a regression in the handling of dynamic scope lookup.  Bug 87158 reports the regression, but it is being used for some other purpose.  The patch to be attached fixes bug 87158.

[Andy Wingo]

Created attachment 144577 [details]
Patch

[Michael Saboff]

Comment on attachment 144577 [details]
Patch

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

This fix seems a little like a band-aid.  Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution?  Then the code would break out of the loop at we'd generate the right resolve opcode.

My comment to open a new defect was incorrect.  I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk.  A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173
> +    size_t depthOfFirstScopeWithDynamicChecks = 0;

Why can't the logic for depth be modified so that it provides the right answer?

Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207
> +                ++depthOfFirstScopeWithDynamicChecks;

If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220
>          if ((flags & ResolveResult::DynamicFlag) && depth)
>              return ResolveResult::dynamicGlobalResolve(depth, scope);

Doesn't this code also need the depth before the first dynamic check?

[Andy Wingo]

(In reply to comment #2)
> (From update of attachment 144577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144577&action=review
> 
> This fix seems a little like a band-aid.  Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution?  Then the code would break out of the loop at we'd generate the right resolve opcode.

Michael, re
> 
> My comment to open a new defect was incorrect.  I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk.  A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173
> > +    size_t depthOfFirstScopeWithDynamicChecks = 0;
> 
> Why can't the logic for depth be modified so that it provides the right answer?
> 
> Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks?
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207
> > +                ++depthOfFirstScopeWithDynamicChecks;
> 
> If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one?
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220
> >          if ((flags & ResolveResult::DynamicFlag) && depth)
> >              return ResolveResult::dynamicGlobalResolve(depth, scope);
> 
> Doesn't this code also need the depth before the first dynamic check?