harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Deakin <oliver.dea...@googlemail.com>
Subject Re: [jira] Commented: (HARMONY-6009) [classlib][beans] NPE launching swingset2 example
Date Fri, 15 May 2009 12:29:05 GMT
I have committed this fix at repo revision r775105.

Regards,
Oliver


Oliver Deakin wrote:
> Hi,
>
> I've just hit this very same defect and can recreate it 2 out of 3 
> runs. I've tested the patch [1] and this fixes the SwingSet case. It 
> looks like it is indeed the lack of synchronization on ArrayList that 
> causes the problem. Looking at the ArrayList code there are lines 
> where we do things like:
>
>  array[firstIndex++] = null;
>
> so it is quite possible that we could hit a timing hole between 
> nullifying the entry and updating the index giving us the 
> NullPointerException in this JIRA. If noone has any objections Ill 
> apply the patch soon.
>
> Regards,
> Oliver
>
> [1]
> Index: PropertyChangeSupport.java
> ===================================================================
> --- PropertyChangeSupport.java    (revision 774748)
> +++ PropertyChangeSupport.java    (working copy)
> @@ -256,8 +256,11 @@
>         }
>
>         // Collect up the global listeners
> -        PropertyChangeListener[] gListeners = globalListeners
> -                .toArray(new PropertyChangeListener[0]);
> +        PropertyChangeListener[] gListeners;
> +        synchronized(this) {
> +            gListeners = globalListeners.toArray(new 
> PropertyChangeListener[0]);
> +        }
> +
>         // Fire the events for global listeners
>         for (int i = 0; i < gListeners.length; i++) {
>             gListeners[i].propertyChange(event);
>
>
> Tim Ellison wrote:
>> Kevin Zhou wrote:
>>  
>>> The globalListeners can only be modified by synchronized
>>> "addPropertyChangeListener" and "removePropertyChangeListener".
>>> In addition, these listeners may also be serialized or deserialized by
>>> some applications via "writeObject" and "readObject" methods.
>>> But all of the above methods has excluded the null value from
>>> globalListeners.
>>>     
>>
>> Yep.
>>
>>  
>>> In addition, since this defect can not be reproduced at my site. Thus I
>>> don't think it can be given a null in Beans code.
>>> If any code can add a null value, this should also occurs on my site.
>>>     
>>
>> I'll try to recreate it too.  While it was reliably reproducible for me,
>> I've since rebuilt and can't reproduce it now ;-/
>>
>>  
>>> Assume one scenario as follows:
>>> If one thread calls doFirePropertyChange method, successfully acquires
>>> an array of all the global listeners and stops at line263; another
>>> thread calls removePropertyChangeListener; the 1st thread start to call
>>> propertyChange method, but one of listeners in the acquired array is
>>> null, then it throws NPE. (This may also happen when calling
>>> addPropertyChangeListener method.)
>>> Do you think this could happen on our code?
>>>     
>>
>> I don't think this can happen.
>>
>> When the globalListeners list is copied into the gListeners array (lines
>> 259/260) then any removals from globalListeners will not affect 
>> gListeners.
>>
>> In theory, since ArrayList is not synchronized, we could be unlucky and
>> get a remove() during the toArray() that causes problems.
>>
>> I wouldn't want to synchronize the whole doFirePropertyChange() method,
>> since the propertyChange() callback may take some indeterminate time to
>> execute and we don't want to lock the list for long.  We could
>> synchronize(this) for the .toArray() call.
>>
>> Regards,
>> Tim
>>
>>   
>

-- 
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


Mime
View raw message