<AWT Dev> [9] Request for review: JDK-8145984 sun.lwawt.macosx.CAccessible leaks
Anton Tarasov
anton.tarasov at jetbrains.com
Fri Apr 29 10:08:41 UTC 2016
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.
More information about the awt-dev
mailing list