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