Code review request for 6990094 "ObjectInputStream cloneArray doesn't handle short[]"

[Joe Darcy]

Hi Peter.

Peter Jones wrote:
> Joe,
>
> On Dec 3, 2010, at 11:06 AM, Joe Darcy wrote:
>   
>> Please review this simple fix for
>>
>>   6990094 "ObjectInputStream cloneArray doesn't handle short[]"
>>   http://cr.openjdk.java.net/~darcy/6990094.0/
>>
>> The complete patch is
>>
>> --- old/src/share/classes/java/io/ObjectInputStream.java    2010-12-03 00:31:24.000000000 -0800
>> +++ new/src/share/classes/java/io/ObjectInputStream.java    2010-12-03 00:31:24.000000000 -0800
>> @@ -3498,8 +3498,8 @@
>>            return ((int[]) array).clone();
>>        } else if (array instanceof long[]) {
>>            return ((long[]) array).clone();
>> -        } else if (array instanceof double[]) {
>> -            return ((double[]) array).clone();
>> +        } else if (array instanceof short[]) {
>> +            return ((short[]) array).clone();
>>        } else {
>>            throw new AssertionError();
>>        }
>>     
>
> Ouch, I recall reviewing this code when it was added, but evidently missed this.  FWIW, the change looks good to me [peterjones].
>
>   
>> You'll notice there is no regression test for this change.  One justification is that the fix is in the "obviously no bugs" category. [1] There is an if-else instanceof chain over Object arrays and arrays of each primitive type; there are two checks for double and none for short so changing the one of the double checks to short is "obviously correct."
>>     
>
> Agreed.
>
>   
>> Also, I've taken a stab a writing an explicit regression test for this condition, but the problem only manifests in cases beyond my direct serialization experience where a class has overridden the readUnshared method.
>>     
>
> I think that there was a test to go along with the bug fix that added this code (although evidently it didn't cover the short[] case).  I don't see it in the hg repository now, I wonder if that's because the bug had the security keyword.
>   

Off-list, Alan found the a related closed test and Stuart and I have 
developed an explicit test that tickles this bug:

    http://cr.openjdk.java.net/~darcy/6990094.1/

I plan to shortly push the fix with the test, although the test might be 
renamed or tweaked first,

Thanks,

-Joe