Arguments object bugs within closure in Firefox4

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: 59194618@qq.com, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: arguments with settimeout bugs

(function() {
		var args = arguments;
		function n() {
				args[0] = "caimc";
				alert.apply(null, args);
		}
		setTimeout(n,0);
})("cmc");

IN FF4 it alert undefined   should be ‘caimc’!

Reproducible: Always

Steps to Reproduce:
1.excute the code at details
2.
3.
Actual Results:  
undefined

Expected Results:  
caimc
(Reporter)

Updated

6 years ago
Summary: arguments settimeout → arguments wrong in settimeout function call
(Assignee)

Updated

6 years ago
Assignee: general → luke
(Reporter)

Comment 1

6 years ago
http://cmc3.cn/n/2011/04/03/257.html
Summary: arguments wrong in settimeout function call → Arguments obejct bugs within closure in Firefox4
Jeff, this sounds like the arguments stuff you were looking at recently.
This sounds a little like bug 478174.
(Assignee)

Comment 4

6 years ago
Not bug 478174; no call objects at play here.

I looked at this under a debugger.  A simpler shell-only test case is:

  function assert(x) {
      assertEq(x, "good");
  }
  (function() {
      var a = arguments;
      return function() {
          a[0] = "good";
          assert.apply(null, a);
      }
  })()();

what happens is that setting a[0] calls ArgSetter which first deletes then defines the property.  This causes a regular property to be defined (whose value is on obj->slots) and the value in obj->getArgsData()->slots to be set to MagicValue.  js_fun_apply uses GetElements which reads values out of obj->getArgsData()->slots.  The js_PrototypeHasIndexedProperties check in GetElements is supposed to guarantee that a MagicValue can safely be replaced by 'undefined'.  However, as this case demonstrates, the indexed property can be on the args object itself, so it seems like what we really need is js_ObjectOrPrototypeHasIndexedProperties.
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Arguments obejct bugs within closure in Firefox4 → Arguments object bugs within closure in Firefox4
(Assignee)

Comment 5

6 years ago
Oops, the shell test-case should be:

  function assert(x) {
      assertEq(x, "good");
  }
  (function() {
      var a = arguments;
      return function() {
          a[0] = "good";
          assert.apply(null, a);
      }
  })("bad")();

Also, my proposed js_ObjectOrPrototypeHasIndexedProperties isn't right because *every* args object has indexed properties.  It'll make the code a bit grosser but I think the fix is to just leave the optimized path when a magic value is encountered.
(Assignee)

Comment 6

6 years ago
Created attachment 524341 [details] [diff] [review]
minimize duplication in SplatApplyArgs

The bug currently exists in two places.  This patch refactors SplatApplyArgs so that the bug is in only one place.
Attachment #524341 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

6 years ago
Created attachment 524342 [details] [diff] [review]
fix GetElements

Fixed as comment 5 suggests.
Attachment #524342 - Flags: review?(jwalden+bmo)

Updated

6 years ago
Duplicate of this bug: 648634
Comment on attachment 524341 [details] [diff] [review]
minimize duplication in SplatApplyArgs

>         JSStackFrame *fp = f.regs.fp;
>-        if (!fp->hasOverriddenArgs() &&
>-            (!fp->hasArgsObj() ||
>-             (fp->hasArgsObj() && !fp->argsObj().isArgsLengthOverridden() &&
>-              !js_PrototypeHasIndexedProperties(cx, &fp->argsObj())))) {
>+        if (!fp->hasOverriddenArgs()) {
>+            uintN n;
>+            if (!fp->hasArgsObj()) {
>+                /* Extract the common/fast path where there is no args obj. */
>+                n = fp->numActualArgs();
>+                if (!BumpStack(f, n))
>+                    THROWV(false);
>+                Value *argv = JS_ARGV(cx, vp + 1 /* vp[1]'s argv */);
>+                fp->forEachCanonicalActualArg(CopyTo(argv));
>+            } else {
>+                /* Simulate the argument-pushing part of js_fun_apply: */
>+                JSObject *aobj = &fp->argsObj();
> 
>-            uintN n = fp->numActualArgs();
>-            if (!BumpStack(f, n))
>-                THROWV(false);
>+                /* Steps 4-5 */
>+                uintN length;
>+                if (!js_GetLengthProperty(cx, aobj, &length))
>+                    THROWV(false);
>+
>+                /* Step 6 */
>+                JS_ASSERT(length <= JS_ARGS_LENGTH_MAX);
>+                n = length;
>+
>+                if (!BumpStack(f, n))
>+                    THROWV(false);
>+
>+                /* Steps 7-8 */
>+                Value *argv = JS_ARGV(cx, vp + 1 /* vp[1]'s argv */);
>+                if (!GetElements(cx, aobj, n, argv))
>+                    THROWV(false);
>+            }
>+
>             f.regs.sp += n;

Incrementing the stack there looks wrong, if GetElements invokes a getter, causes GC, etc. that means the values copied into the new argv could be GC'd, etc.  Looks easy to fix, just that the worst-case consequences of this are not quite optimal.
Attachment #524341 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 10

6 years ago
Created attachment 525225 [details] [diff] [review]
minimize duplication in SplatApplyArgs v.2

Nice catch!
Attachment #524341 - Attachment is obsolete: true
Attachment #525225 - Flags: review?(jwalden+bmo)
Comment on attachment 524342 [details] [diff] [review]
fix GetElements

Strict mode is theoretically saner about all this (although not quite implementationally, at the moment), but please duplicate all the tests as strict mode code and run/test them.  Looks good assuming the previous patch is fixed.
Attachment #524342 - Flags: review?(jwalden+bmo) → review+
Attachment #525225 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/tracemonkey/rev/a4e83114bfee
http://hg.mozilla.org/tracemonkey/rev/0c727da2164d
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/a4e83114bfee
http://hg.mozilla.org/mozilla-central/rev/0c727da2164d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 14

6 years ago
This bug regressed into FF5.0. I was able to reproduce it into FF5.0.1 [Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0.1) Gecko/20100101 Firefox/5.0.1]
Claudio filed bug 679719 on comment 14.
You need to log in before you can comment on or make changes to this bug.