can we further nerf f.arguments?

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 588570 [details] [diff] [review]
preparation patch (shouldn't have observable change)
Assignee: general → luke
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 718134
(Assignee)

Comment 3

5 years ago
Green on try
(Assignee)

Updated

5 years ago
Attachment #588570 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #588571 - Flags: superreview?(dmandelin)
Attachment #588571 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

5 years ago
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.
Attachment #588570 - Attachment is obsolete: true
Attachment #588571 - Attachment is obsolete: true
Attachment #588570 - Flags: review?(jwalden+bmo)
Attachment #588571 - Flags: superreview?(dmandelin)
Attachment #588571 - Flags: review?(jwalden+bmo)
Attachment #588927 - Flags: superreview?(dmandelin)
Attachment #588927 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

5 years ago
Created attachment 588949 [details] [diff] [review]
simpler and more direct patch

Oops, missed a test in src/tests (overwritten args are ignored).
Attachment #588927 - Attachment is obsolete: true
Attachment #588927 - Flags: superreview?(dmandelin)
Attachment #588927 - Flags: review?(jwalden+bmo)
Attachment #588949 - Flags: superreview?(dmandelin)
Attachment #588949 - Flags: review?(jwalden+bmo)
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.
Attachment #588949 - Flags: review?(jwalden+bmo) → review+
Attachment #588949 - Flags: superreview?(dmandelin) → superreview+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fe039c4685
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/96fe039c4685
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 733950
You need to log in before you can comment on or make changes to this bug.