Bug 141881 - Function name scope is only created on the function instance that triggered parsing rather than on every function instance that needs it
Summary: Function name scope is only created on the function instance that triggered p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 141887 141174
  Show dependency treegraph
 
Reported: 2015-02-22 17:13 PST by Filip Pizlo
Modified: 2015-02-23 10:14 PST (History)
11 users (show)

See Also:


Attachments
the basic idea (13.05 KB, patch)
2015-02-22 18:20 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (19.12 KB, patch)
2015-02-22 19:12 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (19.15 KB, patch)
2015-02-22 19:27 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (19.17 KB, patch)
2015-02-22 19:28 PST, Filip Pizlo
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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