RFR: 8171219: Missing checks in sparse array shift() implementation

Hannes Wallnöfer hannes.wallnoefer at oracle.com
Thu Dec 15 13:13:15 UTC 2016


I guess shift is just not a very common operation on sparse arrays. It’s almost like mutually exclusive use cases. Still good to have them fixed.

Thanks for the reviews!
Hannes


> Am 15.12.2016 um 13:47 schrieb Attila Szegedi <szegedia at gmail.com>:
> 
> +1
> 
> Wow, this is lots of fixes. Some of these bugs are so glaring I’m astonished they we did not spot them earlier, like failing to set a new length in shiftLeft. It’s great you fixed these.
> 
> Attila.
> 
>> On 15 Dec 2016, at 11:48, Hannes Wallnöfer <hannes.wallnoefer at oracle.com> wrote:
>> 
>> Please review:
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171219
>> Webrev: http://cr.openjdk.java.net/~hannesw/8171219/webrev/
>> 
>> This fixes a number of bugs around the Array.prototype.shift on sparse array data (and its underlying dense data). Unlikely as it may seem, all these issues where exposed by the short test script included in this webrev:
>> 
>> - UntouchedArrayData did not convert to a real ArrayData instance when ensuring place for first element (index 0)
>> - Dense array data did not check their size before trying System.arraycopy in shiftLeft
>> - Dense array data did not set the new length in shiftLeft, thereby leaving empty slots
>> - Sparse array data did not ensure space in the underlying dense array when moving elements from sparse to dense in shiftLeft
>> - Sparse array data did not delete new in-between elements when moving elements from sparse to dense in shiftLeft
>> - Sparse array data did not set the length after shrinking underlying dense data in shiftRight, thereby creating new empty slots
>> - DeletedRangeArrayData could not return the underlying data if it turned empty in shiftLeft, causing problems later on.
>> 
>> I also moved the invocation of ensure() on the underlying dense array from SparseArrayData.ensure() to where it is actually required, which is cleaner and safe since underlying is a private field.
>> 
>> Thanks,
>> Hannes
> 



More information about the nashorn-dev mailing list