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

Anton Tarasov anton.tarasov at jetbrains.com
Wed Dec 30 20:56:38 UTC 2015 

> On 30 Dec 2015, at 20:23, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
> 
> What is the reason of changes in the CGLLayer? This link should be cleared when the native part of CGLLayer is deallocated, and if CGLayer.m.dealloc() is not called means that we have the native leak, no(but probably we should release javaLayer manually?)?

CGLLayer doesn’t seem to be released manually, yes. As far as I see, its lifecycle if tied to AWTView which is tied to JavaComponentAccessibility. So, it’s in the same chain and as soon as it strongly references its java peer, it will keep CAccessible (in the chain on the java side) from reclamation.

> 
> On 30/12/15 19:25, Anton Tarasov wrote:
>> Well, I tried that. However, there’re problems.
>> 
>> 1) Java accessible objects are strongly referenced from the native code.
>> This will prevent those objects from enqueueing into the reference queue.
> 
> Yes, but why we cannot use weak refs in case of native->java. In this case CAccessible will be collected and it will deallocates the native part?

We can, read the rest please.

Regards,
Anton.

> 
>> 2) j2d Disposer by default wraps objects with PhantomReference. This
>> means that by the time the object will be enqueued and thus accessible
>> for disposal, all its WeakReferences will be cleared. So, there will be
>> no way to get the object ‘this’ ref from the dispose method (nor via
>> PhantomReference, which doesn’t provide access to its referent at all).
>> For instance, if I’m adding something like this to CFRetainedResource.java:
>> 
>> private static class CFDisposerRecordimplements DisposerRecord {
>>     WeakReference<CFRetainedResource>weakThis;
>> 
>>     CFDisposerRecord(WeakReference<CFRetainedResource> weakThis) {
>>         this.weakThis = weakThis;
>>     }
>> 
>>     @Override
>> public void dispose() {
>>         CFRetainedResource cf =weakThis.get();
>>         if (cf !=null) {
>>             cf.dispose();
>>         }
>>      }
>> }
>> 
>> I will get null ‘cf’… (also, note that DisposerRecord can’t strongly
>> reference the referent, as DisposerRecord is not wrapped with a
>> phantom/weak reference).
>> 
>> So, as far as I understand, j2d Disposer can’t access the victim object.
>> Even in Window.java where it’s passed a WeakReference(this) it doesn’t
>> directly access the referent. In other cases it works with primitive
>> types, like ‘long’ native pointers.
>> 
>> It’s possible to rewrite CAccessible.dispose in that way, but many of
>> the rest CFRetainedResource extenders relies on ‘this’ in their disposal.
>> 
>> As a solution, I changed the related native refs to ‘weak’ and left
>> CFRetainedResource.finalize() as a working horse… I suspect it could be
>> used just because of the problem N2, originally when native refs were
>> ‘weak’ (accessibility then came with ‘strong’ refs). However, I’ve
>> changed only a part of the potentially related refs (in
>> JavaComponentAccessibility.m, CGLLayer.m). For instance,
>> JavaAccessibilityAction.m still uses strong refs.
>> 
>> Also, I encountered one more issue. It’s in [JavaComponentAccessibility
>> parent]. The point is that getAccessibleParent returns an owner window
>> for toplevels (having an owner window). But the method doesn’t expect a
>> separate toplevel and uses ‘fView’ in JavaComponentAccessibility
>> creator. This is wrong, because this ‘fView’ will be retained for that
>> CF object, created for a toplevel not related to that ‘fView’. This is a
>> leak, at least. It can be reproducible with the sample I originally
>> provided for the bug. Run it so that the frame is shown unfocused
>> (quickly switch to another window after the launch). Then make it a key
>> window again and repeat the steps described in JIRA. The leak is there.
>> 
>> I solved it by retrieving a correct ‘view’ of a toplevel.
>> 
>> (And yet another issue. I naively missed the fact that an Accessible is
>> not necessarily a Component. For those Accessibles, the original scheme
>> I suggested won't work. But this one will.)
>> 
>> The webrev I've posted is for 8u. I’ll make one for 9 when and if the
>> fix is approved.
>> 
>> Thanks & Happy New Year! =)
>> Anton.
>> 
>>> 
>>> On 23/12/15 10:59, Anton Tarasov wrote:
>>>> Sure, I would also appreciate if someone could apply the fix and do a
>>>> sanity check on your side.
>>>> 
>>>> (Yes, I’m aware that Android Studio team is working extensively on a11y
>>>> support. OSX issues are pending resolution indeed…)
>>>> 
>>>> I’ve updated the copyrights:
>>>> 
>>>> http://cr.openjdk.java.net/~ant/JDK-8145984/jdk9/webrev.1
>>>> 
>>>> Thanks,
>>>> Anton.
>>>> 
>>>>> On 23 Dec 2015, at 00:55, Pete Brunet <peter.brunet at oracle.com
>>>>> <mailto:peter.brunet at oracle.com>> wrote:
>>>>> 
>>>>> Thanks for the reply Anton and thanks for the fix you are working on.
>>>>> It will be good to get at least one other set of eyes on the changes
>>>>> as I am not very familiar with that code (but am starting to because
>>>>> I'm looking into fixing some accessibility issues raised by the
>>>>> Andriod Studio team).  -Pete
>>>>> 
>>>>> 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.
>> 
> 
> 
> -- 
> Best regards, Sergey.

More information about the awt-dev mailing list