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
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.7
: P1 Major
Assigned To: Nobody
http://stackoverflow.com/questions/10...
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-17 07:50 PDT by Gavin Kistner
Modified: 2012-05-17 18:33 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Kistner 2012-05-17 07:50:07 PDT
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 Gavin Kistner 2012-05-17 07:54:00 PDT
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 Gavin Kistner 2012-05-17 12:13:35 PDT
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 Gavin Kistner 2012-05-17 12:15:35 PDT
(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 Alexey Proskuryakov 2012-05-17 12:19:32 PDT
Can you create a reduction that doesn't involve the developer console? It looks very dependent on what Web Inspector does now.
Comment 5 Gavin Kistner 2012-05-17 12:34:11 PDT
Created attachment 142529 [details]
Reproducing the problem without using the console
Comment 6 Gavin Kistner 2012-05-17 12:35:03 PDT
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 Gavin Kistner 2012-05-17 13:38:55 PDT
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 Oliver Hunt 2012-05-17 13:56:52 PDT
My guess is incorrect reification in an OSR exit, based purely on the symptoms
Comment 9 Oliver Hunt 2012-05-17 14:14:24 PDT
<rdar://problem/11477670>
Comment 10 Filip Pizlo 2012-05-17 17:53:24 PDT
(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 Filip Pizlo 2012-05-17 18:21:49 PDT
Created attachment 142602 [details]
the patch
Comment 12 Oliver Hunt 2012-05-17 18:27:51 PDT
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
Comment 13 Filip Pizlo 2012-05-17 18:33:18 PDT
(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().
Comment 14 Filip Pizlo 2012-05-17 18:33:38 PDT
Landed in http://trac.webkit.org/changeset/117523