<AWT Dev> [9] Request for review: JDK-8145984 sun.lwawt.macosx.CAccessible leaks

[Sergey Bylokhov]

Looks fine to me.

On 29.04.16 13:08, Anton Tarasov wrote:
> Hi Sergey, Pete,
>
> I’ve updated the changes for jdk9, based on the final version:
>
> webrev: http://cr.openjdk.java.net/~ant/JDK-8145984/jdk9/webrev.2
>
> No principal difference, except for some adoption to the new code base and accessing “peer” via AWTAccessor in CAccessibility.getAWTView.
>
> Please, have a look for a sanity check. If everything is fine, I’m going to push it...
>
> Regards,
> Anton.
>
>> On 28 Apr 2016, at 15:05, Sergey Bylokhov <Sergey.Bylokhov at 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.Bylokhov at oracle.com
>>>> <mailto:Sergey.Bylokhov at 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.brunet at oracle.com
>>>>>> <mailto:peter.brunet at oracle.com>
>>>>>> <mailto:peter.brunet at 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.brunet at oracle.com>peter.brunet at oracle.com
>>>>>>>> <mailto:peter.brunet at 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.
>


-- 
Best regards, Sergey.