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

[Pete Brunet]

+1 from me too.

On 4/29/16 8:31 AM, Sergey Bylokhov wrote:
> 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.
>>
>
>