Code review request: 6973831 NPE when printing stack trace of OOME

[Brian Goetz]

Looks good now.  Serialization logic is a little confusing but seems OK.

On Aug 11, 2010, at 4:53 PM, Mandy Chung wrote:

> On 08/11/10 12:35, Brian Goetz wrote:
>> This code has thread-safety issues.  There is no happens-before between the writes to suppressedExceptions (and its referenced state) and the places where it is iterated (e.g., line 576).
>> 
>> You could plug this hole by replacing the idiom at line 576 (and similar) with
>>   for (Throwable se : getSuppressedExceptions()).
>> 
> 
> Good point.  I fixed getSuppressedExceptions() be a synchronized method but missed the unsynchronized reads.
> 
>> This would be needed for all unsynchronized reads of suppressedException.  You could get away without it at the null check if you really were trying to squeeze some performance out and you didn't mind occasionally missing writes from other threads.
>> 
>> 
> 
> The updated webrev is at:
>   http://cr.openjdk.java.net/~mchung/6973831/webrev.01/
> 
> I leave the unsynchronized read in the readObject() method as the Throwable object is still being deserialized and the suppressedExceptions is going to be replaced with a new list.
> 
> Thanks
> Mandy
> 
>> On Aug 11, 2010, at 3:09 PM, Mandy Chung wrote:
>> 
>>> Fix for 6973831: NPE when printing stack trace of OOME
>>> 
>>> Webrev at:
>>>   http://cr.openjdk.java.net/~mchung/6973831/webrev.00/
>>> 
>>> The VM preallocates OutOfMemoryError objects to improve OOM diagnosability. The constructor is not invoked since the OOME object allocation is done at VM initialization time and thus the new suppressedExceptions field is null which causes NPE when Throwable.printStackTrace() method is called on the VM preallocated OOM object.
>>> 
>>> The fix is to initialize the suppressedExceptions field to null instead of Collections.emptyList().  An alternative could be to initialize the suppressedException field in the VM after VM initialization is finished.  It doesn't handle the case when any new field initialized to be non-null is added in the OOM object in the future and it would require a VM synchronized change.  The libraries fix is simpler and preferred.
>>> 
>>> Thanks
>>> Mandy
>