Bug 141881

Summary: Function name scope is only created on the function instance that triggered parsing rather than on every function instance that needs it
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 141887, 141174    
Attachments:
Description Flags
the basic idea
none
the patch
none
the patch
none
the patch msaboff: review+

Description Filip Pizlo 2015-02-22 17:13:52 PST
Our bytecode generation has a tendency to mistreat the reexecutability of closures.  For example, we have this silly tendency to use the scope of the current execution of a function to find the symbol tables of all future executions.  Of course, this just barely works.  And because of the tendency to mistreat the reexecutability of closures, and our silly tendency to use the current scope for predicting future resolutions, we have a hilarious bug where only the first execution of a function that needs a name scope actually gets that name scope.
Comment 1 Filip Pizlo 2015-02-22 17:15:09 PST
Test case below.  Note that this passes on the first execution of check(); the failure is experienced on execution #2.  This has nothing to do with JITing; it's just our mistreatment of the reexecutability of closures.

function foo() {
    return function bar(str) {
        var barBefore = bar;
        var result = eval(str);
        return [
            barBefore,
            bar,
            function () {
                return bar;
            },
            result
        ];
    }
}

function check() {
    var bar = foo();
    
    function verify(result, barAfter, evalResult) {
        if (result[0] !== bar)
            throw "Error: bad first entry: " + result[0];
        if (result[1] !== barAfter)
            throw "Error: bad first entry: " + result[1];
        var subResult = result[2]();
        if (subResult !== barAfter)
            throw "Error: bad second entry: " + result[2] + "; returned: " + subResult;
        if (result[3] !== evalResult)
            throw "Error: bad third entry: " + result[3] + "; expected: " + evalResult;
    }
    
    verify(bar("42"), bar, 42);
    verify(bar("bar"), bar, bar);
    verify(bar("var bar = 42; function fuzz() { return bar; }; fuzz()"), 42, 42);
}

for (var i = 0; i < 100; ++i)
    check();
Comment 2 Filip Pizlo 2015-02-22 17:29:09 PST
Ideally, the way this would work is that we'd have the bytecode create the scope. But that would break the current hack where we create the name scope just before bytecode linking so that linking sees the scope in the way.

Maybe a correct solution would be:

1) Bytecode creates the scope at the prologue.
2) Bytecode linking is given a phony function name scope.
Comment 3 Filip Pizlo 2015-02-22 18:20:12 PST
Created attachment 247090 [details]
the basic idea

This code is pretty shameful, but it's less wrong than trunk.
Comment 4 Filip Pizlo 2015-02-22 19:12:49 PST
Created attachment 247092 [details]
the patch
Comment 5 Filip Pizlo 2015-02-22 19:27:28 PST
Created attachment 247093 [details]
the patch

New patch because I realized that the bug title was incoherent.
Comment 6 Filip Pizlo 2015-02-22 19:28:31 PST
Created attachment 247094 [details]
the patch

It was still incoherent. Maybe it's more coherent now.
Comment 7 Michael Saboff 2015-02-23 10:10:26 PST
Comment on attachment 247094 [details]
the patch

r=me
Comment 8 Filip Pizlo 2015-02-23 10:14:40 PST
Landed in http://trac.webkit.org/changeset/180506