Last Comment Bug 718022 - can we further nerf f.arguments?
: can we further nerf f.arguments?
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: 659577 718134 733950
  Show dependency treegraph
 
Reported: 2012-01-13 12:19 PST by Luke Wagner [:luke]
Modified: 2012-03-07 16:31 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
preparation patch (shouldn't have observable change) (8.53 KB, patch)
2012-01-13 17:45 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
nerf 1-liner (and fix tests) (1.99 KB, patch)
2012-01-13 17:46 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
simpler and more direct patch (7.13 KB, patch)
2012-01-16 10:22 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
simpler and more direct patch (8.06 KB, patch)
2012-01-16 11:49 PST, Luke Wagner [:luke]
jwalden+bmo: review+
dmandelin: superreview+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-01-13 12:19:45 PST
Even with bug 592927, f.arguments still causes undue complexity (and gets in the way of the ideal solution for bug 659577).  Fortunately, other browsers (chrome and ie) "break" f.arguments even more than we currently do (scare quotes b/c f.arguments is unspecified in ES3 and specified-to-be-a-poison-pill in ES5 strict).  Already, in IE9, Chrome, and FF4+, 

  function g() { f.arguments[0] = 42; }
  function f(arg) { g(); return arg; }

f(10) returns 10.  In Chrome:

  function g() { f.arguments[0] = 42; }
  function f(arg) { g(); return arguments[0]; }

f(10) returns 10 when called from crankshaft.  In IE9:

  function g() { return f.arguments[0]; }
  function f(arg) { arg = 42; return g(); }

f(10) returns 10.

The conditions where f.arguments breaks is obviously pretty irregular.  Based on this and the fact that we didn't have a single reported breakage resulting from bug 592927, I doubt much code can be depending on these aliasing subtleties.  Thus, I propose the following nerfing extension: f.arguments creates a new non-aliasing copy of the actual arguments, always.

(If noone objects) the patch for this should be simple and back-out-able so we can use the same "send it down the chutes and back it out if it breaks the web" model as, say, the single-threaded runtime assert.
Comment 1 Luke Wagner [:luke] 2012-01-13 17:45:39 PST
Created attachment 588570 [details] [diff] [review]
preparation patch (shouldn't have observable change)
Comment 2 Luke Wagner [:luke] 2012-01-13 17:46:54 PST
Created attachment 588571 [details] [diff] [review]
nerf 1-liner (and fix tests)

I'll wait a bit longer for comment before requesting review on these.
Comment 3 Luke Wagner [:luke] 2012-01-13 21:51:06 PST
Green on try
Comment 4 Luke Wagner [:luke] 2012-01-16 10:22:16 PST
Created attachment 588927 [details] [diff] [review]
simpler and more direct patch

This patch nerfs a case the original missed by just doing what comment 0 said directly.  It also adds a comment.
Comment 5 Luke Wagner [:luke] 2012-01-16 11:49:24 PST
Created attachment 588949 [details] [diff] [review]
simpler and more direct patch

Oops, missed a test in src/tests (overwritten args are ignored).
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-17 02:21:34 PST
Comment on attachment 588949 [details] [diff] [review]
simpler and more direct patch

Review of attachment 588949 [details] [diff] [review]:
-----------------------------------------------------------------

It's really really early and my sleep schedule's all shot to pieces (hopefully to be put back together during the BOS->SFO flight, priming me for a very early night's sleep that has me waking up at a normal time tomorrow), but I think I can review such a short patch without too much danger of footgunning.

Note You need to log in before you can comment on or make changes to this bug.