647425 - Arguments object bugs within closure in Firefox4

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

http://cmc3.cn/n/2011/04/03/257.html

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jeff, this sounds like the arguments stuff you were looking at recently.

Comment 3

[Jeff Walden [:Waldo]]

This sounds a little like bug 478174.

Comment 4

[Luke Wagner [:luke]]

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.

Comment 5

[Luke Wagner [:luke]]

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.

Comment 6

[Luke Wagner [:luke]]

Attachment: minimize duplication in SplatApplyArgs

The bug currently exists in two places.  This patch refactors SplatApplyArgs so that the bug is in only one place.

Comment 7

[Luke Wagner [:luke]]

Attachment: fix GetElements

Fixed as comment 5 suggests.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Luke Wagner [:luke]]

Attachment: minimize duplication in SplatApplyArgs v.2

Nice catch!

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/a4e83114bfee
http://hg.mozilla.org/tracemonkey/rev/0c727da2164d

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/a4e83114bfee
http://hg.mozilla.org/mozilla-central/rev/0c727da2164d

Comment 14

[Claudio Procida]

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]

Comment 15

[Boris Zbarsky [:bzbarsky]]

Claudio filed bug 679719 on comment 14.