Bug 95659 – Named functions should not allocate a scope object for their names

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

Named functions should not allocate a scope object for their names

[Geoffrey Garen]

Created attachment 161850 [details]
Patch

[Oliver Hunt]

Comment on attachment 161850 [details]
Patch

Wow, strict mode actually allowing us to be faster in some cases???? :D

[Gavin Barraclough]

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); } "); })()

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

Created attachment 162413 [details]
Patch

[Geoffrey Garen]

Old patch had a bug with constructed named functions (demonstrated by DOM attribute event listeners).

[Geoffrey Garen]

Committed r127698: <http://trac.webkit.org/changeset/127698>

[Csaba Osztrogonác]

(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?

[Geoffrey Garen]

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.

[Geoffrey Garen]

Committed r127810: <http://trac.webkit.org/changeset/127810>

[Csaba Osztrogonác]

(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?

[Yong Li]

(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?

[Geoffrey Garen]

> It seems this commit broke Octane pdf.js test.

On what platform?

[Yong Li]

(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.

[Cosmin Truta]

(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);