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+

Filip Pizlo
Reported 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.
Attachments
the basic idea (13.05 KB, patch)
2015-02-22 18:20 PST, Filip Pizlo
no flags
the patch (19.12 KB, patch)
2015-02-22 19:12 PST, Filip Pizlo
no flags
the patch (19.15 KB, patch)
2015-02-22 19:27 PST, Filip Pizlo
no flags
the patch (19.17 KB, patch)
2015-02-22 19:28 PST, Filip Pizlo
msaboff: review+
Filip Pizlo
Comment 1 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();
Filip Pizlo
Comment 2 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.
Filip Pizlo
Comment 3 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.
Filip Pizlo
Comment 4 2015-02-22 19:12:49 PST
Created attachment 247092 [details] the patch
Filip Pizlo
Comment 5 2015-02-22 19:27:28 PST
Created attachment 247093 [details] the patch New patch because I realized that the bug title was incoherent.
Filip Pizlo
Comment 6 2015-02-22 19:28:31 PST
Created attachment 247094 [details] the patch It was still incoherent. Maybe it's more coherent now.
Michael Saboff
Comment 7 2015-02-23 10:10:26 PST
Comment on attachment 247094 [details] the patch r=me
Filip Pizlo
Comment 8 2015-02-23 10:14:40 PST
Note You need to log in before you can comment on or make changes to this bug.