Bug 86733 - Setting array index -1 and looping over array causes bad behavior
: Setting array index -1 and looping over array causes bad behavior
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.7
: P1 Major
Assigned To:
: http://stackoverflow.com/questions/10...
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-05-17 07:50 PST by
Modified: 2012-05-17 18:33 PST (History)


Attachments
Reproducing the problem without using the console (415 bytes, text/html)
2012-05-17 12:34 PST, Gavin Kistner
no flags Details
the patch (7.23 KB, patch)
2012-05-17 18:21 PST, Filip Pizlo
oliver: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-05-17 07:50:07 PST
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
------- Comment #1 From 2012-05-17 07:54:00 PST -------
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.
------- Comment #2 From 2012-05-17 12:13:35 PST -------
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
------- Comment #3 From 2012-05-17 12:15:35 PST -------
(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.
------- Comment #4 From 2012-05-17 12:19:32 PST -------
Can you create a reduction that doesn't involve the developer console? It looks very dependent on what Web Inspector does now.
------- Comment #5 From 2012-05-17 12:34:11 PST -------
Created an attachment (id=142529) [details]
Reproducing the problem without using the console
------- Comment #6 From 2012-05-17 12:35:03 PST -------
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
------- Comment #7 From 2012-05-17 13:38:55 PST -------
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.
------- Comment #8 From 2012-05-17 13:56:52 PST -------
My guess is incorrect reification in an OSR exit, based purely on the symptoms
------- Comment #9 From 2012-05-17 14:14:24 PST -------
<rdar://problem/11477670>
------- Comment #10 From 2012-05-17 17:53:24 PST -------
(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.
------- Comment #11 From 2012-05-17 18:21:49 PST -------
Created an attachment (id=142602) [details]
the patch
------- Comment #12 From 2012-05-17 18:27:51 PST -------
(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
------- Comment #13 From 2012-05-17 18:33:18 PST -------
(In reply to comment #12)
> (From update of attachment 142602 [details] [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().
------- Comment #14 From 2012-05-17 18:33:38 PST -------
Landed in http://trac.webkit.org/changeset/117523