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 Thu, 14 May 2009 16:59:44 GMT
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