Bug 171456 - Deep nesting is leading to ReferenceError for hoisted function
Summary: Deep nesting is leading to ReferenceError for hoisted function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: GSkachkov
URL:
Keywords:
Depends on:
Blocks: 163208
  Show dependency treegraph
 
Reported: 2017-04-28 15:43 PDT by GSkachkov
Modified: 2017-04-29 16:22 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.34 KB, patch)
2017-04-29 03:22 PDT, GSkachkov
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2017-04-28 15:43:29 PDT
function boo () {
    {
        function foo() {}
    }
    if (true) {
        {
            {
                {
                    {
                        {
                           {
                                {
                                    {
                                        {
                                            let x = 0;
                                            print(x);
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
    print(!!foo); // ReferenceError: Can't find variable: foo, but should be true
};
boo();

Error occurs because we do not copy hosting candidates when we increase buffer for scope stack, so we need to add , m_sloppyModeHoistableFunctionCandidates(WTFMove(other.m_sloppyModeHoistableFunctionCandidates)) to Scope constructor

This error present in latest Safari and STP
Comment 1 GSkachkov 2017-04-28 15:45:48 PDT
Will prepare patch tomorrow with tests.
Comment 2 Saam Barati 2017-04-28 16:08:55 PDT
(In reply to GSkachkov from comment #0)
> function boo () {
>     {
>         function foo() {}
>     }
>     if (true) {
>         {
>             {
>                 {
>                     {
>                         {
>                            {
>                                 {
>                                     {
>                                         {
>                                             let x = 0;
>                                             print(x);
>                                         }
>                                     }
>                                 }
>                             }
>                         }
>                     }
>                 }
>             }
>         }
>     }
Oh man this is glorious. Nice find.
>     print(!!foo); // ReferenceError: Can't find variable: foo, but should be
> true
> };
> boo();
> 
> Error occurs because we do not copy hosting candidates when we increase
> buffer for scope stack, so we need to add ,
> m_sloppyModeHoistableFunctionCandidates(WTFMove(other.
> m_sloppyModeHoistableFunctionCandidates)) to Scope constructor
> 
> This error present in latest Safari and STP
Comment 3 GSkachkov 2017-04-29 03:22:45 PDT
Created attachment 308663 [details]
Patch

Fix
Comment 4 Yusuke Suzuki 2017-04-29 06:03:52 PDT
Comment on attachment 308663 [details]
Patch

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

Nice! r=me with comment.

> Source/JavaScriptCore/parser/Parser.h:228
>      {

Listing all the members are not a scalable way. Can we just do `Scope(Scope&&) = default;` here?
Comment 5 GSkachkov 2017-04-29 14:27:01 PDT
(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 308663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308663&action=review
> 
> Nice! r=me with comment.
> 
> > Source/JavaScriptCore/parser/Parser.h:228
> >      {
> 
> Listing all the members are not a scalable way. Can we just do
> `Scope(Scope&&) = default;` here?

Done. Thanks for review!
Comment 6 GSkachkov 2017-04-29 14:27:41 PDT
Patch landed https://trac.webkit.org/r215977
Comment 7 GSkachkov 2017-04-29 14:27:55 PDT
Comment on attachment 308663 [details]
Patch

Clear flags
Comment 8 Joseph Pecoraro 2017-04-29 16:22:51 PDT
Nice!