RFR: 7148497: javax.management.MBeanAttributeInfo.hashCode throws NullPointerException

[Karen Kinnear]

Thank you for checking Frederic.

thanks,
Karen

On Apr 13, 2012, at 10:43 AM, Frederic Parain wrote:

> Karen,
> 
> As far as I can tell, getDescriptor() never returns null.
> 
> In the javax.management package, getDescriptor() calls
> ImmutableDescriptor.nonNullDescriptor(descriptor).clone()
> which returns a reference to a singleton empty Descriptor
> instance if the descriptor field is null.
> 
> The getDescriptor() method is redefined in some classes of
> the javax.management.modelmbean package, but these implementations
> never return null either.
> 
> So, unless I missed some code, it is safe to use getDescriptor()
> without null check.
> 
> Fred
> 
> On 4/13/12 3:54 PM, Karen Kinnear wrote:
>> Frederic,
>> 
>> Looks good. And I like the refactoring.
>> 
>> A question - does the Descriptor field have the same potential for being null? e.g.
>> in MBeanFeatureInfo.java new line 160 - do you need to check getDescriptor() == null as well?
>> in MBeanConstructorinfo.java line 197, do you need the alternative null checking?
>> MBeanAttributeInfo.java line 293?
>> (there may be other lines, I didn't do a thorough search)
>> 
>> thanks,
>> Karen
>> 
>> 
>> 
>> On Apr 13, 2012, at 9:01 AM, Staffan Larsen wrote:
>> 
>>> This is much easier to read!
>>> 
>>> In MBeanFeatureInfo.hasSameName() this check is not needed:
>>> 
>>> 274         if(info.getName() == null) {
>>> 275            return false;
>>> 276         }
>>> 
>>> since String.equals() with a null argument works as expected (return false). The same is true for the implementation in hasSameDescription().
>>> 
>>> When calling hasSameName() and hasSameDescription() the "this." is not needed, but perhaps using it makes the intent clearer?
>>> 
>>> Thanks,
>>> /Staffan
>>> 
>>> 
>>> 
>>> 
>>> On 13 apr 2012, at 14:45, Frederic Parain wrote:
>>> 
>>>> Thanks Staffan and Dmitry for your feedback,
>>>> here's a new webrev with a refactored code for
>>>> name and descriptions fields which are common to
>>>> most MBean*Info classes:
>>>> 
>>>> http://cr.openjdk.java.net/~fparain/7148497/webrev.01/
>>>> 
>>>> Thanks,
>>>> 
>>>> Fred
>>>> 
>>>> On 4/12/12 8:18 AM, Dmitry Samersoff wrote:
>>>>> Frederic,
>>>>> 
>>>>> I'm second to Staffan.
>>>>> 
>>>>> Looks good to me but would prefer to have it refactored, e.g. create a
>>>>> method equalOrNull();
>>>>> 
>>>>> -Dmitry
>>>>> 
>>>>> 
>>>>> On 2012-04-10 18:33, Staffan Larsen wrote:
>>>>>> Looks good! (Although I wish there was a better way to write code like
>>>>>> this).
>>>>>> 
>>>>>> /Staffan
>>>>>> 
>>>>>> On 10 apr 2012, at 15:50, Frederic Parain wrote:
>>>>>> 
>>>>>>> Greetings,
>>>>>>> 
>>>>>>> This is a request for review for CR
>>>>>>> 7148497: javax.management.MBeanAttributeInfo.hashCode throws
>>>>>>> NullPointerException
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7148497
>>>>>>> 
>>>>>>> Even if the initial CR only refers to MBeanAttributeInfo.hashCode(),
>>>>>>> the problem is not limited to the MBeanAttributeInfo.hashCode(), but
>>>>>>> is common to all javax.management.MBean*Info classes:
>>>>>>> MBeanAttributeInfo, MBeanConstructorInfo, MBeanFeatureInfo,
>>>>>>> MBeanInfo, MBeanNotificationInfo, MBeanOperationInfo and
>>>>>>> MBeanParameterInfo.
>>>>>>> 
>>>>>>> The root cause of the problem is that these classes can be
>>>>>>> instantiated with some null fields (mainly, but not limited to, the
>>>>>>> name and type fields), and these fields are dereferenced
>>>>>>> unconditionally in some methods. Unconditional dereferencing is used
>>>>>>> in the hashCode method but also in the equals() method.
>>>>>>> 
>>>>>>> The proposed fix improves implementation of equals() and hashCode()
>>>>>>> method to handle cases where some fields are null without throwing
>>>>>>> a NPE.
>>>>>>> 
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~fparain/7148497/webrev.00/
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> 
>>>>>>> Fred
>>>>>>> 
>>>>>>> --
>>>>>>> Frederic Parain - Oracle
>>>>>>> Grenoble Engineering Center - France
>>>>>>> Phone: +33 4 76 18 81 17
>>>>>>> Email: Frederic.Parain at oracle.com
>>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> --
>>>> Frederic Parain - Oracle
>>>> Grenoble Engineering Center - France
>>>> Phone: +33 4 76 18 81 17
>>>> Email: Frederic.Parain at oracle.com
>>>> 
>>> 
>> 
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: Frederic.Parain at oracle.com
>