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

Fixing more UIKit memory leaks #1861

Closed
wants to merge 2 commits into from

Conversation

jaredhms
Copy link
Contributor

Fixing a leak in the Create* path for our UIKit.Xaml controls, and a leak in UIButton's use of 'weakSelf'.

Fixes #1858.
Fixes #1859 .

@@ -184,7 +184,7 @@ - (void)_initUIButton {
_contentVerticalAlignment = UIControlContentVerticalAlignmentCenter;
_contentHorizontalAlignment = UIControlContentHorizontalAlignmentCenter;

__block UIButton* weakSelf = self;
__weak UIButton* weakSelf = self;
Copy link
Contributor

@yiyang-msft yiyang-msft Jan 27, 2017

Choose a reason for hiding this comment

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

UIButton [](start = 11, length = 8)

unrelated: in UIView.mm, line 758, is it also another misusage?

// Subscribe to the XAML node's input events
__block UIView* weakSelf = self; #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. UIView doesn't have ARC enabled, so you can't use __weak.


In reply to: 98295161 [](ancestors = 98295161)

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, block will +1 for reference count in arc but not for non-arc case.


In reply to: 98295339 [](ancestors = 98295339,98295161)

@yiyang-msft
Copy link
Contributor

:shipit:

@@ -36,6 +36,72 @@
ASSERT_TRUE([backingElement isKindOfClass:[WXFrameworkElement class]]);
}

@interface UIDeallocTestButton : UIButton {
NSCondition* _condition;
BOOL* _signaled;
Copy link
Contributor

@oliversa-msft oliversa-msft Jan 27, 2017

Choose a reason for hiding this comment

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

BOOL* [](start = 4, length = 5)

Why BOOL* and not just BOOL. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? so we can set the correct variable to signaled.


In reply to: 98299381 [](ancestors = 98299381)

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I see the usage now.


In reply to: 98299648 [](ancestors = 98299648,98299381)

[_condition lock];
*_signaled = YES;
[_condition signal];
[_condition unlock];
Copy link
Contributor

@oliversa-msft oliversa-msft Jan 27, 2017

Choose a reason for hiding this comment

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

I merged UXEvent into develop - should be able to leverage it now. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweet; that will be much cleaner


In reply to: 98299508 [](ancestors = 98299508)


TEST(UIButton, CheckForLeaks) {
Microsoft::WRL::WeakRef weakXamlElement;
{
Copy link
Contributor

@oliversa-msft oliversa-msft Jan 27, 2017

Choose a reason for hiding this comment

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

{ [](start = 4, length = 1)

Does scoping it like this mean all objects are freed at the end or could we still have some autoreleasepools that need to release objects? Just asking because that was one of the concerns with fixing #1757. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the plan. the VC goes out of scope, etc. we could add our own autoreleasepool but the test was passing without it.


In reply to: 98299778 [](ancestors = 98299778)

__block NSCondition* condition = [[NSCondition alloc] init];

// Create and render the button
dispatch_sync(dispatch_get_main_queue(), ^{
Copy link
Contributor

@oliversa-msft oliversa-msft Jan 27, 2017

Choose a reason for hiding this comment

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

async? #Resolved

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'm fine waiting for this to happen before I wait.


In reply to: 98299832 [](ancestors = 98299832)

@oliversa-msft
Copy link
Contributor

:shipit:

@jaredhms jaredhms closed this Jan 27, 2017
@jaredhms jaredhms deleted the uikit_leaks branch January 27, 2017 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants