Bug 96227 - DFG misses arguments tear-off for function.arguments if 'arguments' is used
Summary: DFG misses arguments tear-off for function.arguments if 'arguments' is used
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:
Blocks:
 
Reported: 2012-09-09 22:08 PDT by Geoffrey Garen
Modified: 2012-09-10 14:48 PDT (History)
5 users (show)

See Also:

Attachments
Patch (5.09 KB, patch)
2012-09-09 22:09 PDT, Geoffrey Garen 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
compatibility test (1.07 KB, text/html)
2012-09-10 13:47 PDT, Geoffrey Garen 		no flags Details
compatibility test (1.07 KB, text/html)
2012-09-10 13:49 PDT, Geoffrey Garen 		no flags Details
compatibility test (1.07 KB, text/html)
2012-09-10 13:58 PDT, Geoffrey Garen 		no flags Details
compatibility test (1.19 KB, text/html)
2012-09-10 14:01 PDT, Geoffrey Garen 		no flags Details
compatibility test (1.20 KB, text/html)
2012-09-10 14:02 PDT, Geoffrey Garen 		no flags Details
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2012-09-09 22:08:18 PDT 
DFG misses arguments tear-off for function.arguments if 'arguments' is used
Comment 1 Geoffrey Garen 2012-09-09 22:09:14 PDT 
Created attachment 163039 [details]
Patch
Comment 2 Geoffrey Garen 2012-09-09 22:11:19 PDT 
Comment on attachment 163039 [details]
Patch

Uploading for discussion.

This patch eliminates aliasing of the local 'arguments' property, and makes function.arguments always tear off.

Another way to fix this bug, and keep our current behavior without regression, is to use watchpoints.
Comment 3 Geoffrey Garen 2012-09-09 22:14:04 PDT 
Here's what I know about the state of function.arguments:

- It's non-standard: Mozilla invented and then deprecated it; ES4 never mentioned it; and ES5 explicitly poisons it in strict mode.

- Compatibility matrix of arguments == function.arguments:
Firefox: never
IE: never, but function.arguments == function.arguments
Chrome: if caller uses 'arguments'
Safari: if caller uses 'arguments'

Compatibility matrix of what function.arguments contains:
Firefox: current snapshot
Chrome: current snapshot
Safari: current snapshot
IE: original snapshot

Based on this matrix, the best compatibility story by market share seems to be:
	- function.arguments doesn't alias 'arguments' (IE + Firefox)
	- function.arguments snapshots current argument values (Chrome + Firefox + Safari)
Comment 4 Filip Pizlo 2012-09-09 22:17:42 PDT 
Comment on attachment 163039 [details]
Patch

I think that your change implies that using 'arguments' should not cause the DFG to assume that arguments (i.e. the actual arguments, not the 'arguments' local) are captured.  You should probably make that part of this change, as well - or at least file a separate bug indicating that we should kill off that code in the future.

Other than that, I have no objections to this change.
Comment 5 WebKit Review Bot 2012-09-09 23:55:10 PDT 
Comment on attachment 163039 [details]
Patch

Attachment 163039 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13795743

New failing tests:
fast/js/dfg-tear-off-function-dot-arguments.html
Comment 6 Geoffrey Garen 2012-09-10 12:24:01 PDT 
Notions of "captured arguments" that may need fixup:
CodeBlock
capturedVars bitvector
    needs to match variableAccessData isCaptured flag
ByteCodeParser initialization

All isCaptured() queries other than VariableAccessData queries should just forward to the CodeBlock.
Comment 7 Geoffrey Garen 2012-09-10 13:47:54 PDT 
Created attachment 163198 [details]
compatibility test
Comment 8 Geoffrey Garen 2012-09-10 13:49:28 PDT 
Created attachment 163199 [details]
compatibility test
Comment 9 Geoffrey Garen 2012-09-10 13:58:00 PDT 
Created attachment 163203 [details]
compatibility test
Comment 10 Geoffrey Garen 2012-09-10 14:01:22 PDT 
Created attachment 163204 [details]
compatibility test
Comment 11 Geoffrey Garen 2012-09-10 14:02:53 PDT 
Created attachment 163205 [details]
compatibility test
Comment 12 Geoffrey Garen 2012-09-10 14:48:53 PDT 
Committed r128111: <http://trac.webkit.org/changeset/128111>