Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS A11y memory leak #7244

Merged
merged 10 commits into from
Jan 15, 2019
Merged

iOS A11y memory leak #7244

merged 10 commits into from
Jan 15, 2019

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Dec 18, 2018

This has been split out from #6879

The current implementation of the Accessibility Bridge on iOS leads to retain cycles. This patch can be applied independently of the othe retain cycle related work, and hopefully makes this easier to review/track.

@dnfield
Copy link
Contributor Author

dnfield commented Dec 18, 2018

/cc @jason-simmons since you recently touched this as well

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Looks good to me, but can you have one of the ObjC experts take a look at well, e.g. @cbracken?

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Semantics objects can be accessed from both the platform and UI threads. On the other hand all fml::WeakPtr managed objects must be created, used and destroyed on one thread. "Use" of the object includes the get() call. This makes the current usage of WeakPtr's thread unsafe.

I suggest using the std::weak_ptr instead.

Otherwise, lgtm (though I still have to convince myself of this in a debugger).

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 16, 2019
@dnfield dnfield deleted the a11y_memory_leak branch January 29, 2019 17:22
@@ -123,8 +126,7 @@ - (void)dealloc {
[_children removeAllObjects];
[_children release];
_parent = nil;
[_container release];
_container = nil;
_container.get().semanticsObject = nil;
Copy link

@pedia pedia Feb 22, 2019

Choose a reason for hiding this comment

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

this really worked? maybe better:

_container.release(); 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't remember the details now, but this was part of breaking the circular references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't remember the details now, but this was part of breaking the circular references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants