Arguments object bugs within closure in Firefox4

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: 59194618, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

8 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

8 years ago
Summary: arguments settimeout → arguments wrong in settimeout function call
Assignee

Updated

8 years ago
Assignee: general → luke
Reporter

Comment 1

8 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

8 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

8 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

8 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

8 years ago
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

8 years ago
Fixed as comment 5 suggests.
Attachment #524342 - Flags: review?(jwalden+bmo)
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

8 years ago
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+

Comment 14

8 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]
You need to log in before you can comment on or make changes to this bug.