Hi Sergey, Sounds good to me, thanks for the review!
Pete, If you don’t have any more concerns, I’ll list you as the second reviewer. Is that ok? Regards, Anton. > On 28 Apr 2016, at 15:05, Sergey Bylokhov <sergey.bylok...@oracle.com> wrote: > > Hi, Anton. > The fix looks fine. > I suggest to push it to jdk9 and wait at least one build before back-porting > to jdk8. > > On 13.04.16 19:52, Anton Tarasov wrote: >> Hi Sergey, >> >> update: http://cr.openjdk.java.net/~ant/JDK-8145984/jdk8u/webrev.3 >> >> >>> On 12 Apr 2016, at 15:49, Sergey Bylokhov <sergey.bylok...@oracle.com >>> <mailto:sergey.bylok...@oracle.com>> wrote: >>> >>> Hi, Anton. >>> On 10.01.16 13:12, Anton Tarasov wrote: >>>> >>>> http://cr.openjdk.java.net/~ant/JDK-8145984/jdk8u/webrev.2/ >>>> >>>> and the discussion with Sergey. >>> >>> I am a little bit worried about the changes in CGLLayer. Is it >>> possible that javaLayer will be collected in native when we tries to >>> use it? >> >> Good question. I revised the code again and found a number of misses in >> my fix related to weak refs null check. >> >> Most of the weak refs are used as params for the CAccessibility static >> methods which take care of null checks (I only found one miss). Here we >> don’t need to do anything in addition. >> >> For the rest I used the following pattern (advised in the JNI spec): >> >> jobject tmpLocalRef = NewLocalRef(weakGlobalRef) >> if (IsSameObject(tmpLocalRef, NULL)) { >> // return >> } >> // use tmpLocalRef >> DeleteLocalRef(tmpLocalRef) >> >> Also, I discovered more refs which need to be converted to WeakGlobal. >> Actually, nothing in the a11y code should hold java UI objects strongly >> (according to what we discussed previously). >> Those are in JavaAccessibilityAction.m. >> >> And one else missing local jobject in JavaComponentAccessibility.m, >> taken as GetObjectArrayElement, to delete. >> >> That’s basically it. Please look into the new version. >> >> Regards, >> Anton. >> >> >>> >>>> >>>>> On 04 Jan 2016, at 21:16, Pete Brunet <peter.bru...@oracle.com >>>>> <mailto:peter.bru...@oracle.com> >>>>> <mailto:peter.bru...@oracle.com>> wrote: >>>>> >>>>> Hi Anton, Why did you change CAccessible.dispose() from protected to >>>>> public? Shouldn't it be left protected? -Pete >>>> >>>> I did that due to introducing an interface with dispose() method. But >>>> with the latest fix, there’s no that change anymore. >>>> >>>> Regards, >>>> Anton. >>>> >>>>> >>>>> On 12/22/15 3:45 PM, Anton Tarasov wrote: >>>>>> Hi Pete, >>>>>> >>>>>> Thanks for the review! >>>>>> >>>>>>> On 22 Dec 2015, at 23:07, Pete Brunet >>>>>>> <<mailto:peter.bru...@oracle.com>peter.bru...@oracle.com >>>>>>> <mailto:peter.bru...@oracle.com>> wrote: >>>>>>> >>>>>>> Hi Anton, Some comments/questions: >>>>>>> - Some copyright dates need updating >>>>>> >>>>>> Indeed, I’ll update them. >>>>>> >>>>>>> - Line 1112 of JavaComponentAccessibility: does the release of >>>>>>> jaccessible cause a release of jparent? >>>>>> >>>>>> As I can see, jparent there is only a ref to jComponent, which in its >>>>>> turn is a JNIGlobalRef (or anyway is a class field). So, I don’t see >>>>>> the need to delete it… (or did I miss something?) >>>>>> >>>>>>> - Line 7155 of Component.java: is that the only place where this >>>>>>> means is needed? >>>>>> >>>>>> If you mean to call AC.dispose() than - yes, I think so. We rather >>>>>> don’t want to dispose the context until the Component goes out of the >>>>>> UI hierarchy, which is when Component.removeNotify() is _always_ >>>>>> getting called (for hw & lw components). >>>>>> >>>>>> Anton. >>>>>> >>>>>>> >>>>>>> Pete >>>>>>> >>>>>>> On 12/22/15 8:10 AM, Anton Tarasov wrote: >>>>>>>> Hi guys! >>>>>>>> >>>>>>>> Could you please review the problem I’ve filed and the suggested fix? >>>>>>>> >>>>>>>> bug: JDK-8145984 >>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8145984> >>>>>>>> sun.lwawt.macosx.CAccessible >>>>>>>> leaks >>>>>>>> webrev: http://cr.openjdk.java.net/~ant/JDK-8145984/jdk9/webrev.0 >>>>>>>> <http://cr.openjdk.java.net/%7Eant/JDK-8145984/jdk9/webrev.0> >>>>>>>> >>>>>>>> (This is to be addressed in 8u/9. The webrev for 8u is in JIRA, >>>>>>>> it’s identical except the paths.) >>>>>>>> >>>>>>>> Please, find the details in JIRA. >>>>>>>> >>>>>>>> Thanks! >>>>>>>> Anton. >>>>>>> >>>>>> >>>>> >>>> >>> >>> >>> -- >>> Best regards, Sergey. >> > > > -- > Best regards, Sergey.
