Bug 86733 – Setting array index -1 and looping over array causes bad behavior

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Kistner]

Steps to reproduce:

1) Open a page with the following code on Safari 5.1.7 on OS X
   (Or use this link: http://jsfiddle.net/TzCm9/)

   <label>p: <input id="p" size="3"></label>
   <script type="text/javascript">
     var p = document.getElementById('p');
     p.onkeyup = function(){
       var a = "10 20 30 40".split(/\s+/);
       foo(a, p.value*1);
     } 

     function foo(a,p){
       var count=a.length, i=0, x;
       if (p) a[i=-1]=p;
       while (i<10000) x = a[i++ % count];
       console.dir(a);
     }
     </script>

2) Open the Developer Console
3) Focus the input and type <1><Backspace><2>
4) Expand console output for the arrays.

EXPECTED OUTPUT:
a) The first and third arrays displayed should have a property named "-1" with values "1" and "2" (respectively)
b) Safari is stable

ACTUAL OUTPUT:
a) The first array has a "-1" property displayed.
   The third array has a property "4294967295" displayed (but a `length` of 4).
b) Occasionally continuing to interact with the page causes the docked Developer Tools window to disappear, and/or all tabs in Safari to become non-responsive. (Hence the "Critical" severity.)

NOTES:
This bug does not reproduce on Safari 5.1.7 on Windows 7.
This bug does not reproduce on Chrome or Firefox on OS X.
This bug does not reproduce if you delete the `while` loop.


See additional discussion, including a screenshot of the console, here:
http://stackoverflow.com/questions/10629083/unexplained-behavior-in-safari-with-negative-array-indices

[Gavin Kistner]

One additional note: I experienced the unstable behavior multiple times over multiple restarts of Safari, but was unable to consistently reproduce it. When it does not occur immediately, it still may occur if you repeatedly type a character followed by a backspace many times.

[Gavin Kistner]

Here is a simpler test case:

setInterval(function(){
  var a=[10,20,30,40], i=-1, x, c=a.length;
  a[-1] = 42;
  while (i<10000) x = a[i++ % a];
  console.log(a[-1],a[4294967295]);
},100);

The above code produces the following console output:

     42 undefined
     undefined 42
     42 undefined
37x  undefined 42
     42 undefined
     undefined 42
     42 undefined
41x  undefined 42
     42 undefined
     undefined 42
     42 undefined
41x  undefined 42
     42 undefined
     undefined 42
     42 undefined
37x  undefined 42

[Gavin Kistner]

(In reply to comment #2)
>   while (i<10000) x = a[i++ % a];

I intended to write:
   while (i<10000) x = a[i++ % c];

but either version results in similar output.

[Alexey Proskuryakov]

Can you create a reduction that doesn't involve the developer console? It looks very dependent on what Web Inspector does now.

[Gavin Kistner]

Created attachment 142529 [details]
Reproducing the problem without using the console

[Gavin Kistner]

I've added an attachment that shows the bug without using the console. on Safari 5.1.7 on OS X I see this in the textarea:


42:undefined
42:undefined
undefined:42
42:undefined
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
42:undefined
undefined:42
42:undefined
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42
undefined:42

[Gavin Kistner]

Further investigation shows that the same problem occurs with `-2` set as a value, but the problem is fixed if the key is explicitly set as the string "-1" instead of the integer -1.

[Oliver Hunt]

My guess is incorrect reification in an OSR exit, based purely on the symptoms

[Oliver Hunt]

<rdar://problem/11477670>

[Filip Pizlo]

(In reply to comment #8)
> My guess is incorrect reification in an OSR exit, based purely on the symptoms

Nope, OSR is fine.  It's the slow path C function that the DFG calls for out-of-bounds indices.  It assumes that the value is non-negative even though the whole point of the function is to handle both negative and too-large positive indices.

[Filip Pizlo]

Created attachment 142602 [details]
the patch

[Oliver Hunt]

Comment on attachment 142602 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142602&action=review

r=me, but switch to Identifier::from() rather than using toString()

> Source/JavaScriptCore/dfg/DFGOperations.cpp:465
> +    Identifier property(exec, jsNumber(index).toString(exec)->value(exec));
> +    PutPropertySlot slot(true);

Use Identifier::from(exec, index)

> Source/JavaScriptCore/dfg/DFGOperations.cpp:482
> +    Identifier property(exec, jsNumber(index).toString(exec)->value(exec));

ditto

[Filip Pizlo]

(In reply to comment #12)
> (From update of attachment 142602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142602&action=review
> 
> r=me, but switch to Identifier::from() rather than using toString()
> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:465
> > +    Identifier property(exec, jsNumber(index).toString(exec)->value(exec));
> > +    PutPropertySlot slot(true);
> 
> Use Identifier::from(exec, index)
> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:482
> > +    Identifier property(exec, jsNumber(index).toString(exec)->value(exec));
> 
> ditto

Ah!  Changed to use ::from().

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/117523