Bug 95659

Created attachment 161850 [details] Patch

Comment on attachment 161850 [details] Patch Wow, strict mode actually allowing us to be faster in some cases???? :D

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

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

Created attachment 162413 [details] Patch

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

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

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

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.

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

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

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

> It seems this commit broke Octane pdf.js test. On what platform?

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