Last Comment Bug 647425 - Arguments object bugs within closure in Firefox4
: Arguments object bugs within closure in Firefox4
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows 7
-- normal (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
: Steven DeTar [:sdetar]
Mentors:
: 648634 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-02 04:12 PDT by 59194618@qq.com
Modified: 2011-08-23 22:18 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
minimize duplication in SplatApplyArgs (4.51 KB, patch)
2011-04-06 20:57 PDT, Luke Wagner [:luke]
Waldo: review-
Details | Diff | Splinter Review
fix GetElements (8.95 KB, patch)
2011-04-06 20:59 PDT, Luke Wagner [:luke]
Waldo: review+
Details | Diff | Splinter Review
minimize duplication in SplatApplyArgs v.2 (4.77 KB, patch)
2011-04-11 17:17 PDT, Luke Wagner [:luke]
Waldo: review+
Details | Diff | Splinter Review

Description User image 59194618@qq.com 2011-04-02 04:12:29 PDT
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 User image 59194618@qq.com 2011-04-03 22:56:12 PDT
http://cmc3.cn/n/2011/04/03/257.html
Comment 2 User image Boris Zbarsky [:bzbarsky, bz on IRC] 2011-04-04 22:53:28 PDT
Jeff, this sounds like the arguments stuff you were looking at recently.
Comment 3 User image Jeff Walden [:Waldo] 2011-04-06 11:17:35 PDT
This sounds a little like bug 478174.
Comment 4 User image Luke Wagner [:luke] 2011-04-06 16:19:59 PDT
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 User image Luke Wagner [:luke] 2011-04-06 16:59:39 PDT
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 User image Luke Wagner [:luke] 2011-04-06 20:57:51 PDT
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.
Comment 7 User image Luke Wagner [:luke] 2011-04-06 20:59:30 PDT
Created attachment 524342 [details] [diff] [review]
fix GetElements

Fixed as comment 5 suggests.
Comment 8 User image Sean Stangl [:sstangl] 2011-04-08 14:26:31 PDT
*** Bug 648634 has been marked as a duplicate of this bug. ***
Comment 9 User image Jeff Walden [:Waldo] 2011-04-11 16:53:46 PDT
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 User image Luke Wagner [:luke] 2011-04-11 17:17:09 PDT
Created attachment 525225 [details] [diff] [review]
minimize duplication in SplatApplyArgs v.2

Nice catch!
Comment 11 User image Jeff Walden [:Waldo] 2011-04-11 17:19:14 PDT
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 14 User image Claudio Procida 2011-08-17 08:20:39 PDT
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 User image Boris Zbarsky [:bzbarsky, bz on IRC] 2011-08-23 22:18:38 PDT
Claudio filed bug 679719 on comment 14.

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