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 #1863

Merged
merged 6 commits into from
Jan 30, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Frameworks/UIKit/UIButton.mm
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ - (void)_initUIButton {
_contentVerticalAlignment = UIControlContentVerticalAlignmentCenter;
_contentHorizontalAlignment = UIControlContentHorizontalAlignmentCenter;

__block UIButton* weakSelf = self;
__weak UIButton* weakSelf = self;
XamlControls::HookButtonPointerEvents(_xamlButton,
^(RTObject* sender, WUXIPointerRoutedEventArgs* e) {
// We mark the event as handled here. The method _processPointerPressedCallback
Expand Down
12 changes: 9 additions & 3 deletions Frameworks/UIKit/XamlControls.mm
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
// Button
////////////////////////////////////////////////////////////////////////////////////
WXCButton* CreateButton() {
ComPtr<IInspectable> inspectable(XamlCreateButton());
ComPtr<IInspectable> inspectable;
// Use Attach for transfer-ownership semantics, else we +1 and leak
inspectable.Attach(XamlCreateButton());
Copy link
Contributor

@rajsesh rajsesh Jan 27, 2017

Choose a reason for hiding this comment

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

Your real problem is the XamlCreateButton api design. If it returned HRESULT and an out pointer, you have to try hard to leak it. With the current model, it is easy to mess it up. #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 suppose an out param would make it a bit more difficult to leak.


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

return _createRtProxy([WXCButton class], inspectable.Get());
}

Expand All @@ -53,7 +55,9 @@ void HookLayoutEvent(WXCButton* button, WUXIPointerEventHandler layoutHook) {
// ContentDialog
////////////////////////////////////////////////////////////////////////////////////
WXCContentDialog* CreateContentDialog() {
ComPtr<IInspectable> inspectable(XamlCreateContentDialog());
ComPtr<IInspectable> inspectable;
// Use Attach for transfer-ownership semantics, else we +1 and leak
inspectable.Attach(XamlCreateContentDialog());
return _createRtProxy([WXCContentDialog class], inspectable.Get());
}

Expand Down Expand Up @@ -95,7 +99,9 @@ void XamlContentDialogSetDestructiveButtonIndex(WXCContentDialog* contentDialog,
// Label
////////////////////////////////////////////////////////////////////////////////////
WXCGrid* CreateLabel() {
Microsoft::WRL::ComPtr<IInspectable> inspectable(XamlCreateLabel());
Microsoft::WRL::ComPtr<IInspectable> inspectable;
// Use Attach for transfer-ownership semantics, else we +1 and leak
inspectable.Attach(XamlCreateLabel());
return _createRtProxy([WXCGrid class], inspectable.Get());
}

Expand Down
5 changes: 5 additions & 0 deletions tests/functionaltests/FunctionalTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ extern void UIActivityIndicatorViewGetXamlElement();

extern void UIButtonCreateXamlElement();
extern void UIButtonGetXamlElement();
extern void UIButtonCheckForLeaks();
extern void UIButtonTitleColorChanged();
extern void UIButtonTextChanged();

Expand Down Expand Up @@ -606,6 +607,10 @@ class UIKitTests {
FrameworkHelper::RunOnUIThread(&UIButtonGetXamlElement);
}

TEST_METHOD(UIButton_CheckForLeaks) {
UIButtonCheckForLeaks();
}

TEST_METHOD(UIButton_TitleColorChanged) {
UIButtonTitleColorChanged();
}
Expand Down
57 changes: 57 additions & 0 deletions tests/functionaltests/Tests/UIKitTests/UIButtonTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,63 @@
ASSERT_TRUE([backingElement isKindOfClass:[WXFrameworkElement class]]);
}

@interface UIDeallocTestButton : UIButton {
std::shared_ptr<UXEvent> _event;
}
@end

@implementation UIDeallocTestButton

- (void)setDeallocEvent:(std::shared_ptr<UXEvent>)event {
_event = event;
}

-(void)dealloc {
_event->Set();
[super dealloc];
}
@end

TEST(UIButton, CheckForLeaks) {
Microsoft::WRL::WeakRef weakXamlElement;
{
__block UIButtonWithControlsViewController* buttonVC = [[UIButtonWithControlsViewController alloc] init];
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.

__block [](start = 8, length = 7)

I might have missed this before but do you really need __block here. I am under the impression, it should be retained correctly. #Resolved

UXTestAPI::ViewControllerPresenter testHelper(buttonVC);

__block UIDeallocTestButton* testButton = nil;
__block auto event = UXEvent::CreateAuto();

// Create and render the button
dispatch_sync(dispatch_get_main_queue(), ^{
testButton = [[UIDeallocTestButton alloc] initWithFrame:CGRectMake(100, 100, 100, 100)];
testButton.backgroundColor = [UIColor redColor];
[testButton setDeallocEvent:event];
[buttonVC.view addSubview:testButton];
});

// Grab a weak reference to the backing Xaml element
Microsoft::WRL::ComPtr<IInspectable> xamlElement([[testButton xamlElement] comObj]);
ASSERT_TRUE_MSG(SUCCEEDED(xamlElement.AsWeak(&weakXamlElement)), "Failed to acquire a weak reference to the backing Xaml element.");
xamlElement = nullptr;

// Free the button
dispatch_sync(dispatch_get_main_queue(), ^{
// Nil out testButton and remove it from its superview
[testButton removeFromSuperview];
[testButton release];
testButton = nil;
});

// Validate that dealloc was called
ASSERT_TRUE_MSG(event->Wait(c_testTimeoutInSec), "FAILED: Waiting for dealloc call failed!");
}

// Validate that we can no longer acquire a strong reference to the UIButton's backing Xaml element
Microsoft::WRL::ComPtr<IInspectable> xamlElement;
weakXamlElement.As(&xamlElement);
ASSERT_EQ_MSG(xamlElement.Get(), nullptr, "Unexpectedly able to reacquire a strong reference to the backing Xaml element.");
}

TEST(UIButton, TitleColorChanged) {
__block auto uxEvent = UXEvent::CreateManual();
__block auto xamlSubscriber = std::make_shared<XamlEventSubscription>();
Expand Down