Bug 95659 - Named functions should not allocate a scope object for their names
Summary: Named functions should not allocate a scope object for their names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on: 95944 96065
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-02 15:11 PDT by Geoffrey Garen
Modified: 2012-10-11 12:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (32.57 KB, patch)
2012-09-02 15:56 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (45.80 KB, patch)
2012-09-05 21:54 PDT, Geoffrey Garen
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2012-09-02 15:11:02 PDT
Named functions should not allocate a scope object for their names
Comment 1 Geoffrey Garen 2012-09-02 15:56:10 PDT
Created attachment 161850 [details]
Patch
Comment 2 Oliver Hunt 2012-09-02 20:08:51 PDT
Comment on attachment 161850 [details]
Patch

Wow, strict mode actually allowing us to be faster in some cases???? :D
Comment 3 Gavin Barraclough 2012-09-03 11:47:19 PDT
Comment on attachment 161850 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:592
> +        emitOpcode(op_push_name_scope);

What if the function already shadows the function name? - doesn't this push scoped var override the function's scope? doesn't this invert the function name & a var defined within the function? – looking through your test cases, I'm guessing the answer is no, or else I don't think
    (function closure() { eval("var closure;"); closure = 1; return closure == 1 && !this.closure; })()
would work, but I haven't yet worked out why. :-)  Might be worth adding something like:
   javascript:(function foo(){ var foo = 42; eval(" with({}) { alert(foo); } "); })()
Comment 4 Geoffrey Garen 2012-09-04 18:19:15 PDT
> doesn't this invert the function name & a var defined within the function?

Good question -- I got this wrong in an initial version of the patch. Because we push the name scope *before* we push the activation scope, vars in the activation take precedence over the name in the name scope.
Comment 5 Geoffrey Garen 2012-09-05 21:54:02 PDT
Created attachment 162413 [details]
Patch
Comment 6 Geoffrey Garen 2012-09-05 21:54:43 PDT
Old patch had a bug with constructed named functions (demonstrated by DOM attribute event listeners).
Comment 7 Geoffrey Garen 2012-09-05 23:17:26 PDT
Committed r127698: <http://trac.webkit.org/changeset/127698>
Comment 8 Csaba Osztrogonác 2012-09-06 00:55:16 PDT
(In reply to comment #7)
> Committed r127698: <http://trac.webkit.org/changeset/127698>

It caused a regression - https://bugs.webkit.org/show_bug.cgi?id=95944
Could you check it, please?
Comment 9 Geoffrey Garen 2012-09-06 12:46:04 PDT
Rolled out in <http://trac.webkit.org/changeset/127774> because it broke
fast/dom/HTMLScriptElement/script-reexecution-pretty-diff.html.

Attribute event listeners assume they can blow away a function's scope chain after the function has been created. Have to think about how to fix this.
Comment 10 Geoffrey Garen 2012-09-06 18:42:20 PDT
Committed r127810: <http://trac.webkit.org/changeset/127810>
Comment 11 Csaba Osztrogonác 2012-09-06 21:51:18 PDT
(In reply to comment #10)
> Committed r127810: <http://trac.webkit.org/changeset/127810>

One more confusion: https://bugs.webkit.org/show_bug.cgi?id=96065
Could you check it, please?
Comment 12 Yong Li 2012-10-11 08:37:23 PDT
(In reply to comment #10)
> Committed r127810: <http://trac.webkit.org/changeset/127810>

It seems this commit broke Octane pdf.js test. It ends up with either crashes (in DFG::optionValueAdd(), where op1 has CellTag but 0 payload) or freezes. Issue disappears once this commit is reverted. Idea?
Comment 13 Geoffrey Garen 2012-10-11 09:58:00 PDT
> It seems this commit broke Octane pdf.js test.

On what platform?
Comment 14 Yong Li 2012-10-11 10:06:57 PDT
(In reply to comment #13)
> > It seems this commit broke Octane pdf.js test.
> 
> On what platform?

On QNX, ARMv7, DFG JIT. Seems classic JIT works well.
Comment 15 Cosmin Truta 2012-10-11 12:14:16 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > It seems this commit broke Octane pdf.js test.
> > 
> > On what platform?
> 
> On QNX, ARMv7, DFG JIT. Seems classic JIT works well.

In case this may be of help: sometimes it also assert-fails at
JSC::DFG::RegisterBank<JSC::DFG::GPRInfo>::retain
(this=0x77dfc4a0, reg=JSC::ARMRegisters::r2, name=4, spillOrder=5) at
Source/JavaScriptCore/dfg/DFGRegisterBank.h:187
    // 'index' should not currently be named, the new name must be valid.
    ASSERT(m_data[index].name == InvalidVirtualRegister);