Hello, AWT team. It's been a while, so I am reminding. Could somebody please make a second review on this fix. Thank you.
For your convenience: Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7079260 Fix: http://cr.openjdk.java.net/~pchelko/7079260/webrev.03/ With best regards. Petr. On Feb 6, 2013, at 1:09 PM, Artem Ananiev wrote: > > 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. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>> >>
