<AWT Dev> [8] Review request for CR 7079260 : InputContext leaks memory

[Artem Ananiev]

Looks fine.

Thanks,

Artem

On 2/6/2013 12:03 PM, Petr Pchelko wrote:
> Hello, Artem.
>
> Thank you for the review. Here is an updated version of the fix:
> http://cr.openjdk.java.net/~pchelko/7079260/webrev.03/
>
> I have got rid of the Weak Refs anti-pattern and added copyright to the test which I have forgotten.
>
> With best regards. Petr.
>
> On Feb 5, 2013, at 6:47 PM, Artem Ananiev wrote:
>
>>
>> On 2/5/2013 6:00 PM, Petr Pchelko wrote:
>>> Hello, Artem.
>>>
>>> Please have a look to the new version of this fix:
>>> http://cr.openjdk.java.net/~pchelko/7079260/webrev.02/
>>>
>>> As we have discussed offline, I've replaced the needResetXICClient reference to the weak ref. The second reference clientComponent in the CompositionAreaHandler is used quite similar to the needResedXICClient field, so I thought it is OK to use a Weak Ref there too, as it makes the code much cleaner.
>>
>> Indeed, the changes are clearer now. One comment about using WeakReference.get(). The following code is anti-pattern:
>>
>>   if (ref.get() != null) {
>>     ref.get().doSomethin();
>>   }
>>
>> It should be replaced with
>>
>>   SomeClass t = ref.get();
>>   if (t != null) {
>>     t.doSomething();
>>   }
>>
>> The chances ref.get() to return different values (null and non-null) from two subsequent calls are very low, but not zero.
>>
>> Thanks,
>>
>> Artem
>>
>>> With best regards. Petr.
>>>
>>> On Feb 4, 2013, at 4:52 PM, Artem Ananiev wrote:
>>>
>>>> Hi, Petr,
>>>>
>>>> I looked at the changes again and noticed that we now have two methods in InputMethodAdapter that look quite related to each other:
>>>>
>>>> protected void cleanClient(Component)
>>>>
>>>> and
>>>>
>>>> void setClientComponent(Component)
>>>>
>>>> In the only class where cleanClient() is overridden, cleanClient is often the same as needResetXICClient. All the above makes me believe we can fix the leak without introducing cleanClient(), but making setClientComponent() protected and using it in subclasses.
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 2/1/2013 6:45 PM, Petr Pchelko wrote:
>>>>> Hello, Artem.
>>>>>
>>>>> Sorry for the delay, I've got distracted by other issues.
>>>>>
>>>>> The new version of the fix is available at:
>>>>> http://cr.openjdk.java.net/~pchelko/7079260/webrev.01/
>>>>>
>>>>>> 1. In JTextComponent you need to override addNotify() to re-install caret change listener.
>>>>>
>>>>> I have all removed the changes in JTextComponent which were needed to clean the caret. There are rootset references from the caret timer thread to the component, however, these references are cleaned up on the next blink of the caret. I've tried to clean the caret on removeNotify and restore it's state on addNotify, however it does not seem possible without changes in public APIs and without adding some unnecessary fields.
>>>>>
>>>>> So, as this is a very short-timed memory leak and the component will certainly be cleaned up it half a second.
>>>>>
>>>>>> X11InputMethod can't be referenced in sun.awt.im code (in fact, X11* classes are absent on Windows at all).
>>>>> Done.
>>>>>
>>>>> With best regards. Petr.
>>>>>
>>>>> On Jan 18, 2013, at 5:22 PM, Artem Ananiev wrote:
>>>>>
>>>>>>
>>>>>> Minor comments:
>>>>>>
>>>>>> 1. In JTextComponent you need to override addNotify() to re-install caret change listener.
>>>>>>
>>>>>> 2. X11InputMethod can't be referenced in sun.awt.im code (in fact, X11* classes are absent on Windows at all).
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Artem
>>>>>>
>>>>>> On 1/18/2013 4:26 PM, Petr Pchelko wrote:
>>>>>>> Hello, this is a reminder.
>>>>>>>
>>>>>>> For your convenience:
>>>>>>>
>>>>>>> 7079260 : InputContext leaks memory
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7079260
>>>>>>>
>>>>>>> The new webrev is available here:
>>>>>>> http://cr.openjdk.java.net/~serb/petr/7079260/webrev/
>>>>>>>
>>>>>>> With best regards. Petr.
>>>>>>>
>>>>>>> On Jan 10, 2013, at 12:49 PM, Petr Pchelko wrote:
>>>>>>>
>>>>>>>> Hello.
>>>>>>>>
>>>>>>>> Sorry, I've forgot about licenses. I will add them before the push.
>>>>>>>>
>>>>>>>> With best regards. Petr.
>>>>>>>>
>>>>>>>> On Jan 9, 2013, at 4:38 PM, Petr Pchelko wrote:
>>>>>>>>
>>>>>>>>> Hello, here is the new version of the fix with the test attached.
>>>>>>>>>
>>>>>>>>> While writing the test I have noticed some additional references which also were not removed and could lead to a memory leak, so now the following references are cleaned:
>>>>>>>>> 1. References from X11InputMethod which were reported in an original CR
>>>>>>>>> 2. References from CompositionAreaHandler
>>>>>>>>> 3. References from the Caret timer. It is not critical, as these references were removed at the time of the next caret blink, however now it is cleaned immediately.
>>>>>>>>>
>>>>>>>>> The new webrev is available here:
>>>>>>>>> http://cr.openjdk.java.net/~serb/petr/7079260/webrev/
>>>>>>>>>
>>>>>>>>> Best, Petr.
>>>>>>>>>
>>>>>>>>> On Dec 21, 2012, at 7:16 PM, Sergey Bylokhov wrote:
>>>>>>>>>
>>>>>>>>>> Hi, Petr.
>>>>>>>>>> It would be good to have appropriate testcase for this issue too.
>>>>>>>>>>
>>>>>>>>>> 21.12.2012 16:57, Petr Pchelko wrote
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Could you please review the fix for the issue:
>>>>>>>>>>> 7079260 : InputContext leaks memory
>>>>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7079260
>>>>>>>>>>>
>>>>>>>>>>> The webrev is available at:
>>>>>>>>>>> http://cr.openjdk.java.net/~art/pchelko/7079260/webrev/
>>>>>>>>>>>
>>>>>>>>>>> The memory leak component in the test, provided in the description of the bug is still now collected with this fix, however now all the references are in netbeans code, not AWT.
>>>>>>>>>>>
>>>>>>>>>>> The fix was tested on Linux platform with toy apps and automatic tests related to im.
>>>>>>>>>>>
>>>>>>>>>>> Best, Petr.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>