Bug 95024 – Don't allocate space for arguments and call frame if arguments aren't captured

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Don't allocate space for arguments and call frame if arguments aren't captured

[Geoffrey Garen]

Created attachment 160592 [details]
Patch

[Filip Pizlo]

Comment on attachment 160592 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSActivation.h:160
> -        // CallFrame
> -        registers[RegisterFile::ScopeChain].set(globalData, this, m_registers[RegisterFile::ScopeChain].get());
> -        registers[RegisterFile::Callee].set(globalData, this, m_registers[RegisterFile::Callee].get());
> -
>          // vars

Are you sure?  I seem to recall arguments tear-off using them for something weird. :-/

[Filip Pizlo]

Created attachment 160595 [details]
test case

I initially thought your patch failed to handle this, but now I'm pretty sure it's fine.  Anyway, I figure that this test case should be handy.

[Filip Pizlo]

Comment on attachment 160595 [details]
test case

I will land it myself once reviewed.

[Geoffrey Garen]

> Are you sure?  I seem to recall arguments tear-off using them for something weird. :-/

I, too, had this gut reaction -- and obviously the code was there for some reason at some point -- but I searched around and couldn't find a reason anymore.

The arguments object holds a direct reference to "callee" now. Perhaps that's why we used to need callee, and don't need it anymore.

As for the scope chain, I have no idea why we ever needed to preserve that. Pretty much any case you have an activation, it's owned by a function that also owns the scope chain. Also, by deduction: since we know we can merge scope chain and activation, it's hard to conceive of a case when one would be alive and one wouldn't.

Famous last words.

[Geoffrey Garen]

Comment on attachment 160595 [details]
test case

r=me on the test

[Geoffrey Garen]

Aha! Here's the bug that we missed: https://bugs.webkit.org/show_bug.cgi?id=82947.

I'll need to rethink what to do for the "argumentsGetter" case, and add a test for it.

[Geoffrey Garen]

The bug arises if you call the argumentsGetter after the activation has torn off. That's impossible in user code, but the web inspector gets special direct access to activation objects, and can make it happen.

[Geoffrey Garen]

Looks like I can fix the crash in a better way: https://bugs.webkit.org/show_bug.cgi?id=95033.

[Filip Pizlo]

Test landed in http://trac.webkit.org/changeset/126708

[Geoffrey Garen]

Comment on attachment 160592 [details]
Patch

Setting back to r+ based on Phil's earlier review, and the fix for the edge case we were worried about. (Side note: I was not able to reproduce the edge case even without its fix, but it's nice to be safe.)

[Geoffrey Garen]

Committed r126722: <http://trac.webkit.org/changeset/126722>