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

[Anton Tarasov]

Hi Sergey,

update: http://cr.openjdk.java.net/~ant/JDK-8145984/jdk8u/webrev.3 <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> 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>> 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> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20160413/f1a517d5/attachment-0001.html>