Skip to content

Commit

Permalink
8275720: CommonComponentAccessibility.createWithParent isWrapped caus…
Browse files Browse the repository at this point in the history
…es mem leak

Reviewed-by: kizune, pbansal
  • Loading branch information
Anton Tarasov committed Oct 26, 2021
1 parent 7c88a59 commit 574f890
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 77 deletions.
Expand Up @@ -22,7 +22,7 @@
* questions.
*/

#import "CommonComponentAccessibility.h"
#import "ComponentWrapperAccessibility.h"

@interface CellAccessibility : CommonComponentAccessibility
@interface CellAccessibility : ComponentWrapperAccessibility
@end
Expand Up @@ -35,24 +35,6 @@ - (NSAccessibilityRole)accessibilityRole
return NSAccessibilityCellRole;;
}

- (NSArray *)accessibilityChildren
{
NSArray *children = [super accessibilityChildren];
if (children == NULL) {
NSString *javaRole = [self javaRole];
CommonComponentAccessibility *newChild = [CommonComponentAccessibility createWithParent:self
accessible:self->fAccessible
role:javaRole
index:self->fIndex
withEnv:[ThreadUtilities getJNIEnv]
withView:self->fView
isWrapped:NO];
return [NSArray arrayWithObject:newChild];
} else {
return children;
}
}

- (NSInteger)accessibilityIndex
{
return self->fIndex;
Expand Down
Expand Up @@ -65,20 +65,16 @@

+ (void) initializeRolesMap;

+ (CommonComponentAccessibility* _Nullable) getComponentAccessibility:(NSString* _Nonnull)role;
+ (CommonComponentAccessibility * _Nullable) getComponentAccessibility:(NSString * _Nonnull)role andParent:(CommonComponentAccessibility * _Nonnull)parent;
+ (Class _Nonnull) getComponentAccessibilityClass:(NSString* _Nonnull)role;
+ (Class _Nonnull) getComponentAccessibilityClass:(NSString * _Nonnull)role andParent:(CommonComponentAccessibility * _Nonnull)parent;

+ (NSArray* _Nullable)childrenOfParent:(CommonComponentAccessibility* _Nonnull)parent withEnv:(JNIEnv _Nonnull * _Nonnull)env withChildrenCode:(NSInteger)whichChildren allowIgnored:(BOOL)allowIgnored;
+ (NSArray* _Nullable)childrenOfParent:(CommonComponentAccessibility* _Nonnull)parent withEnv:(JNIEnv _Nonnull * _Nonnull)env withChildrenCode:(NSInteger)whichChildren allowIgnored:(BOOL)allowIgnored recursive:(BOOL)recursive;
+ (CommonComponentAccessibility* _Nullable) createWithParent:(CommonComponentAccessibility* _Nullable)parent accessible:(jobject _Nonnull)jaccessible role:(NSString* _Nonnull)javaRole index:(jint)index withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view;
+ (CommonComponentAccessibility* _Nullable) createWithParent:(CommonComponentAccessibility* _Nullable)parent withClass:(Class _Nonnull)classType accessible:(jobject _Nonnull)jaccessible role:(NSString* _Nonnull)javaRole index:(jint)index withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view;
+ (CommonComponentAccessibility* _Nullable) createWithAccessible:(jobject _Nonnull)jaccessible role:(NSString* _Nonnull)role index:(jint)index withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view;
+ (CommonComponentAccessibility* _Nullable) createWithAccessible:(jobject _Nonnull)jaccessible withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view;

// If the isWraped parameter is true, then the object passed as a parent was created based on the same java component,
// but performs a different NSAccessibilityRole of a table cell, or a list row, or tree row,
// and we need to create an element whose role corresponds to the role in Java.
+ (CommonComponentAccessibility* _Nullable) createWithParent:(CommonComponentAccessibility* _Nullable)parent accessible:(jobject _Nonnull)jaccessible role:(NSString* _Nonnull)javaRole index:(jint)index withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view isWrapped:(BOOL)wrapped;

// The current parameter is used to bypass the check for an item's index on the parent so that the item is created. This is necessary,
// for example, for AccessibleJTreeNode, whose currentComponent has index -1
+ (CommonComponentAccessibility* _Nullable) createWithAccessible:(jobject _Nonnull)jaccessible withEnv:(JNIEnv _Nonnull * _Nonnull)env withView:(NSView* _Nonnull)view isCurrent:(BOOL)current;
Expand Down
Expand Up @@ -218,9 +218,9 @@ + (void) initializeRolesMap {

/*
* If new implementation of the accessible component peer for the given role exists
* return the allocated class otherwise return nil to let old implementation being initialized
* return the component's class otherwise return CommonComponentAccessibility
*/
+ (CommonComponentAccessibility *) getComponentAccessibility:(NSString *)role
+ (Class) getComponentAccessibilityClass:(NSString *)role
{
AWT_ASSERT_APPKIT_THREAD;
if (rolesMap == nil) {
Expand All @@ -229,22 +229,22 @@ + (CommonComponentAccessibility *) getComponentAccessibility:(NSString *)role

NSString *className = [rolesMap objectForKey:role];
if (className != nil) {
return [NSClassFromString(className) alloc];
return NSClassFromString(className);
}
return [CommonComponentAccessibility alloc];
return [CommonComponentAccessibility class];
}

+ (CommonComponentAccessibility *) getComponentAccessibility:(NSString *)role andParent:(CommonComponentAccessibility *)parent
+ (Class) getComponentAccessibilityClass:(NSString *)role andParent:(CommonComponentAccessibility *)parent
{
AWT_ASSERT_APPKIT_THREAD;
if (rolesMap == nil) {
[self initializeRolesMap];
}
NSString *className = [rowRolesMapForParent objectForKey:[[parent class] className]];
if (className == nil) {
return [CommonComponentAccessibility getComponentAccessibility:role];
return [CommonComponentAccessibility getComponentAccessibilityClass:role];
}
return [NSClassFromString(className) alloc];
return NSClassFromString(className);
}

- (id)initWithParent:(NSObject *)parent withEnv:(JNIEnv *)env withAccessible:(jobject)accessible withIndex:(jint)index withView:(NSView *)view withJavaRole:(NSString *)javaRole
Expand Down Expand Up @@ -532,10 +532,11 @@ + (CommonComponentAccessibility *) createWithAccessible:(jobject)jaccessible rol

+ (CommonComponentAccessibility *) createWithParent:(CommonComponentAccessibility *)parent accessible:(jobject)jaccessible role:(NSString *)javaRole index:(jint)index withEnv:(JNIEnv *)env withView:(NSView *)view
{
return [CommonComponentAccessibility createWithParent:parent accessible:jaccessible role:javaRole index:index withEnv:env withView:view isWrapped:NO];
Class classType = [self getComponentAccessibilityClass:javaRole andParent:parent];
return [CommonComponentAccessibility createWithParent:parent withClass:classType accessible:jaccessible role:javaRole index:index withEnv:env withView:view];
}

+ (CommonComponentAccessibility *) createWithParent:(CommonComponentAccessibility *)parent accessible:(jobject)jaccessible role:(NSString *)javaRole index:(jint)index withEnv:(JNIEnv *)env withView:(NSView *)view isWrapped:(BOOL)wrapped
+ (CommonComponentAccessibility *) createWithParent:(CommonComponentAccessibility *)parent withClass:(Class)classType accessible:(jobject)jaccessible role:(NSString *)javaRole index:(jint)index withEnv:(JNIEnv *)env withView:(NSView *)view
{
GET_CACCESSIBLE_CLASS_RETURN(NULL);
DECLARE_FIELD_RETURN(jf_ptr, sjc_CAccessible, "ptr", "J", NULL);
Expand All @@ -548,11 +549,8 @@ + (CommonComponentAccessibility *) createWithParent:(CommonComponentAccessibilit
return [[value retain] autorelease];
}

// otherwise, create a new instance
CommonComponentAccessibility *newChild = [CommonComponentAccessibility getComponentAccessibility:javaRole andParent:parent];

// must init freshly -alloc'd object
[newChild initWithParent:parent withEnv:env withAccessible:jCAX withIndex:index withView:view withJavaRole:javaRole]; // must init new instance
CommonComponentAccessibility *newChild =
[[classType alloc] initWithParent:parent withEnv:env withAccessible:jCAX withIndex:index withView:view withJavaRole:javaRole];

// If creating a JPopupMenu (not a combobox popup list) need to fire menuOpened.
// This is the only way to know if the menu is opening; visible state change
Expand Down
@@ -0,0 +1,48 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2021, JetBrains s.r.o.. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
#import "CommonComponentAccessibility.h"

/**
* Some native a11y elements do not have direct peers in Java, like list rows and cells.
* However, these elements are required by Cocoa in order for a11y to work properly.
* The ComponentWrapperAccessibility interface provides a concept of wrapping an element
* originated from java (like a list item, or a table element) with a component
* which has a11y role required Cocoa (like NSAccessibilityRowRole, or NSAccessibilityCellRole)
* but does not have peer in java.
*
* The wrapping component becomes a parent of the wrapped child in the a11y hierarchy.
* The child component is created automatically on demand with the same set of arguments,
* except that it has a11y role of its java peer.
*
* It is important that only the wrapping component is linked with sun.lwawt.macosx.CAccessible
* and thus its lifecycle depends on the java accessible. So when the same java accessible is passed
* to create a native peer, the wrapping component is retrieved in case it has already been
* created (see [CommonComponentAccessibility createWithParent]). When the wrapping component is
* deallocated (as triggered from the java side) it releases the wrapped child.
*/
@interface ComponentWrapperAccessibility : CommonComponentAccessibility

@property (nonatomic, retain) CommonComponentAccessibility *wrappedChild;

@end
@@ -0,0 +1,57 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2021, JetBrains s.r.o.. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
#import "ComponentWrapperAccessibility.h"
#import "ThreadUtilities.h"

@implementation ComponentWrapperAccessibility

@synthesize wrappedChild;

- (NSAccessibilityRole)accessibilityRole {
@throw [NSException exceptionWithName:NSInternalInconsistencyException
reason:[NSString stringWithFormat:@"You must override -(NSAccessibilityRole)accessibilityRole in a subclass"]
userInfo:nil];
}

- (NSArray *)accessibilityChildren {
if (!wrappedChild) {
wrappedChild =
[[CommonComponentAccessibility alloc] initWithParent:self
withEnv:[ThreadUtilities getJNIEnv]
withAccessible:fAccessible
withIndex:0
withView:fView
withJavaRole:fJavaRole];
}
return [NSArray arrayWithObject:wrappedChild];
}

- (void)dealloc {
if (wrappedChild) {
[wrappedChild release];
}
[super dealloc];
}

@end
Expand Up @@ -22,7 +22,7 @@
* questions.
*/

#import "CommonComponentAccessibility.h"
#import "ComponentWrapperAccessibility.h"

@interface ListRowAccessibility : CommonComponentAccessibility <NSAccessibilityRow>
@interface ListRowAccessibility : ComponentWrapperAccessibility <NSAccessibilityRow>
@end
Expand Up @@ -38,25 +38,6 @@ - (NSAccessibilityRole)accessibilityRole
return NSAccessibilityRowRole;
}

- (NSArray *)accessibilityChildren
{
NSArray *children = [super accessibilityChildren];
if (children == NULL) {

// Since the row element has already been created, we should no create it again, but just retrieve it by a pointer, that's why isWrapped is set to YES.
CommonComponentAccessibility *newChild = [CommonComponentAccessibility createWithParent:self
accessible:self->fAccessible
role:self->fJavaRole
index:self->fIndex
withEnv:[ThreadUtilities getJNIEnv]
withView:self->fView
isWrapped:YES];
return [NSArray arrayWithObject:newChild];
} else {
return children;
}
}

- (NSInteger)accessibilityIndex
{
return [[self accessibilityParent] accessibilityIndexOfChild:self];
Expand Down
Expand Up @@ -72,14 +72,7 @@ - (NSArray *)accessibilityChildren
return children;
}
}

return [NSArray arrayWithObject:[CommonComponentAccessibility createWithParent:self
accessible:self->fAccessible
role:self->fJavaRole
index:self->fIndex
withEnv:env
withView:self->fView
isWrapped:YES]];
return [super accessibilityChildren];
}

- (NSInteger)accessibilityDisclosureLevel
Expand Down
Expand Up @@ -84,13 +84,15 @@ - (NSArray *)accessibilityChildren
(*env)->DeleteLocalRef(env, jkey);
}

CellAccessibility *child = [[CellAccessibility alloc] initWithParent:self
withEnv:env
withAccessible:jchild
withIndex:childIndex
withView:self->fView
withJavaRole:childJavaRole];
[children addObject:[[child retain] autorelease]];
CellAccessibility *child = (CellAccessibility *)
[CommonComponentAccessibility createWithParent:self
withClass:[CellAccessibility class]
accessible:jchild
role:childJavaRole
index:childIndex
withEnv:env
withView:self->fView];
[children addObject:child];

(*env)->DeleteLocalRef(env, jchild);
(*env)->DeleteLocalRef(env, jchildJavaRole);
Expand Down

3 comments on commit 574f890

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forantar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/backport jdk17u

@openjdk
Copy link

@openjdk openjdk bot commented on 574f890 Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forantar the backport was successfully created on the branch forantar-backport-574f8903 in my personal fork of openjdk/jdk17u. To create a pull request with this backport targeting openjdk/jdk17u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 574f8903 from the openjdk/jdk repository.

The commit being backported was authored by Anton Tarasov on 26 Oct 2021 and was reviewed by Alexander Zuev and Pankaj Bansal.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u:

$ git fetch https://github.com/openjdk-bots/jdk17u forantar-backport-574f8903:forantar-backport-574f8903
$ git checkout forantar-backport-574f8903
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u forantar-backport-574f8903

Please sign in to comment.