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

[Rémi Forax]

Le 11/08/2010 22:53, Mandy Chung a écrit :
>  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

Brian, Mandy,
It seems this is not the sole thread-safety issue,
access to fields 'cause' or 'stackTrace' aren't thread safe too and 
detailMessage is not final.

Rémi


>
>> 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
>