harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Zhou <zhoukevi...@gmail.com>
Subject Re: [jira] Commented: (HARMONY-6009) [classlib][beans] NPE launching swingset2 example
Date Thu, 20 Nov 2008 06:01:44 GMT
Tim Ellison wrote:
> Kevin Zhou wrote:
>   
>> Tim Ellison wrote:
>>     
>>> Hi Kevin,
>>>
>>> Kevin Zhou wrote:
>>>  
>>>       
>>>> I think this defect may be a thread-safe problem.
>>>> I read HARMONY's PropertyChangeSupport code and found that:
>>>> It employs ArrayList to store the property change listeners which is not
>>>> thread-safe.
>>>> This defect occurs in a private doFirePropertyChange method.
>>>> This method may asynchronously use the list of
>>>> PropertyChangeListener, which
>>>> contains a null value when modification (add or remove) of the list
>>>> has not
>>>> finished.
>>>>     
>>>>         
>>> Yes, I put in some trace code and there is a null stored in the
>>> globalListeners list which causes the NPE.
>>>
>>> However, I don't see where that null could have been stored.  Can you
>>> suggest what part of the code has stored the null in there?  Every add
>>> to the list first checks for null.  I'll take a look and see if I can
>>> catch it in the test.
>>>
>>> I won't apply your 'check-for-null' patch since I think that just masks
>>> the underlying problem.
>>>
>>> Regards,
>>> Tim
>>>
>>>   
>>>       
>> Hi Tim,
>> This patch doesn't only check-for-null.
>> It also changes the private doFirePropertyChange method to synchronized
>> method. This may resolve the asynchronous operation on globalListeners
>> list.
>>     
>
> Yes, I saw that. How will that help?
>
>   
>> I will try to find what part of the code has stored the null.
>> Due to license cares, I think it difficult to find the setting-null code
>> which may be inside SwingSet2.jar.
>>     
>
> But the globalListeners is a private variable, so the only way it can be
> given a null element is if the code successfully calls a method that
> adds a null, or we let the private List escape.  I don't see either of
> those in the Beans code, do you?  Clearly I'm missing something for this
> to happen.
>
> Regards,
> Tim
>
>   
Hi Tim,

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

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?
Thanks :)

Mime
View raw message