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.

Sign up for GitHub

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

Fix various Uikit memory leaks #1803

Merged
merged 4 commits into from
Jan 26, 2017
Merged

Fix various Uikit memory leaks #1803

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f735bd
Fixing some UIKit/WinRT projections memory leaks. Any time we alloca…
jaredhms Jan 23, 2017
f861dca
Fixing leak in Autolayout and its usage of ClSlackVariables.
jaredhms Jan 25, 2017
5ba191e
Updating libcassowary build output with memory leak fix.
jaredhms Jan 25, 2017
8decb17
Incorporating CR feedback.
jaredhms Jan 26, 2017
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
6 changes: 6 additions & 0 deletions Frameworks/AutoLayout/AutoLayout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,12 @@ - (void)autoLayoutLayoutSubviews {
- (BOOL)autoLayoutInvalidateContentSize {
AutoLayoutProperties* layoutProperties = self._autoLayoutProperties;

if (DEBUG_AUTO_LAYOUT_LIGHT) {
TraceVerbose(TAG, L"autoLayoutInvalidateContentSize: %hs(0x%p).",
object_getClassName(self),
self);
}

CGSize newContentSize = [self intrinsicContentSize];
if (CGSizeEqualToSize(layoutProperties->_intrinsicContentSize, newContentSize)) {
if (DEBUG_AUTO_LAYOUT_LIGHT) {
Expand Down
5 changes: 2 additions & 3 deletions Frameworks/UIKit/UIButton.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ - (void)initAccessibility {
- (void)setImage:(UIImage*)image forState:(UIControlState)state {
_states[state].image = image;

// NOTE: check if image is nil before creating inspetableImage
// NOTE: check if image is nil before creating inspectableImage
// ConvertUIImageToWUXMImageBrush:nil creates a valid imageBrush with null comObj
// which isn't what we want
if (image) {
Expand Down Expand Up @@ -552,8 +552,7 @@ - (void)setTitleColor:(UIColor*)color forState:(UIControlState)state {
// ConvertUIColorToWUColor:nil creates a valid WUColor with null comObj
// which isn't what we want
if (color) {
WUColor* convertedColor = XamlUtilities::ConvertUIColorToWUColor(color);
WUXMSolidColorBrush* titleColorBrush = [WUXMSolidColorBrush makeInstanceWithColor:convertedColor];
WUXMSolidColorBrush* titleColorBrush = [[WUXMSolidColorBrush makeInstanceWithColor:XamlUtilities::ConvertUIColorToWUColor(color)] autorelease];
Copy link
Contributor

@yiyang-msft yiyang-msft Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

autorelease [](start = 138, length = 11)

is there anything preventing us from turning on arc on UIButton/UILabel? #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.

good question; i'll give it a shot

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for pushing this!

In reply to: 97875917 [](ancestors = 97875917,97843932)

if (titleColorBrush) {
_states[state].inspectableTitleColor = [titleColorBrush comObj];
Copy link

@DHowett-MSFT DHowett-MSFT Jan 25, 2017 •
edited by jaredhms
Loading

Choose a reason for hiding this comment

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

😲 we are literally allocating an objective-c thing that wraps a com thing to get at the com thing and discard the objective-c thing.

what the actual heck #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.

If this becomes a hot path, we can pull in WRL.

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

}
Expand Down
4 changes: 2 additions & 2 deletions Frameworks/UIKit/UILabel.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ - (void)adjustTextLayerSize {
[_textBlock setFontStyle:static_cast<WUTFontStyle>([_font _fontStyle])];
[_textBlock setFontStretch:static_cast<WUTFontStretch>([_font _fontStretch])];

[_textBlock setFontFamily:[WUXMFontFamily makeInstanceWithName:[_font _compatibleFamilyName]]];
[_textBlock setFontFamily:[[WUXMFontFamily makeInstanceWithName:[_font _compatibleFamilyName]] autorelease]];

[_textBlock setTextAlignment:XamlUtilities::ConvertUITextAlignmentToWXTextAlignment(_alignment)];

Expand All @@ -122,7 +122,7 @@ - (void)adjustTextLayerSize {
if (_isHighlighted && _highlightedTextColor != nil) {
color = _highlightedTextColor;
}
[_textBlock setForeground:[WUXMSolidColorBrush makeInstanceWithColor:XamlUtilities::ConvertUIColorToWUColor(color)]];
[_textBlock setForeground:[[WUXMSolidColorBrush makeInstanceWithColor:XamlUtilities::ConvertUIColorToWUColor(color)] autorelease]];

[self invalidateIntrinsicContentSize];
[self setNeedsDisplay];
Expand Down
1 change: 1 addition & 0 deletions Frameworks/UIKit/XamlUtilities.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// THE SOFTWARE.
//
//******************************************************************************
#import "AssertARCEnabled.h"
#import "XamlUtilities.h"

#import <UIKit/UIActivityIndicatorView.h>
Expand Down
11 changes: 8 additions & 3 deletions deps/3rdparty/cassowary-0.60/c++/ClSimplexSolver.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,9 @@ ClSimplexSolver::RemoveConstraintInternal(const ClConstraint *const pcn)
cerr << "delete@ " << pexpr << endl;
#endif
delete pexpr;

// Delete the ClAbstractVarable that backs marker, else we leak
delete marker.get_pclv();
Copy link
Contributor

@yiyang-msft yiyang-msft Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

marker [](start = 11, length = 6)

wasn't quite sure how these are used coupled with autolayout, we can sync offline. #Resolved

}

// Delete any error variables. If cn is an inequality, it also
Expand All @@ -453,9 +456,11 @@ ClSimplexSolver::RemoveConstraintInternal(const ClConstraint *const pcn)
{
ClVariable v = (*it);
if (v != marker)
{
RemoveColumn(v);
}
{
RemoveColumn(v);
// Delete the ClAbstractVarable that backs v, else we leak
delete v.get_pclv();
}
}
}

Expand Down
1 change: 1 addition & 0 deletions deps/3rdparty/cassowary-0.60/c++/ClVariable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class ClVariable {
}

ClAbstractVariable *get_pclv() const { return pclv; }
ClAbstractVariable *get_pclv() { return pclv; }
bool IsNil() const { return pclv == NULL; }

virtual FDNumber DesiredValue() const
Expand Down
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/ARM/cassowary-0.60-Debug.lib
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/ARM/cassowary-0.60-Debug.pdb
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/ARM/cassowary-0.60.lib
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/ARM/cassowary-0.60.pdb
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/x86/cassowary-0.60-Debug.lib
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/x86/cassowary-0.60-Debug.pdb
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/x86/cassowary-0.60.lib
View file
Open in desktop
Git LFS file not shown
4 changes: 2 additions & 2 deletions deps/prebuilt/Universal Windows/x86/cassowary-0.60.pdb
View file
Open in desktop
Git LFS file not shown
1 change: 1 addition & 0 deletions deps/prebuilt/include/cassowary-0.60/ClVariable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class ClVariable {
}

ClAbstractVariable *get_pclv() const { return pclv; }
ClAbstractVariable *get_pclv() { return pclv; }
bool IsNil() const { return pclv == NULL; }

virtual FDNumber DesiredValue() const
Expand Down
46 changes: 43 additions & 3 deletions samples/WOCCatalog/WOCCatalog/AutoLayoutViewController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ - (instancetype)init {
@end


@implementation AutoLayoutViewController
@implementation AutoLayoutViewController {
CenteredAutoLayoutLabel* _addRemoveLabel;
};

- (void)viewDidLoad {
[super viewDidLoad];
Expand All @@ -64,7 +66,7 @@ - (void)viewDidLoad {
[self.view addConstraints: [NSLayoutConstraint constraintsWithVisualFormat:@"V:[bottomLabel(30)]-[bottomGuide]" options:NO metrics:nil views:NSDictionaryOfVariableBindings(bottomLabel, bottomGuide)]];

UIButton* button = [UIButton buttonWithType:UIButtonTypeRoundedRect];

[button setTitle:@"Button For Baseline" forState:UIControlStateNormal];
[button setTitle:@"Highlighted State Changes Intrinsic Content Size" forState:UIControlStateHighlighted];
[button setContentHuggingPriority:251 forAxis:UILayoutConstraintAxisVertical];
Expand All @@ -73,7 +75,7 @@ - (void)viewDidLoad {
button.layer.cornerRadius = 5.0f;
button.backgroundColor = [UIColor lightGrayColor];
[self.view addSubview:button];

[self.view addConstraint:[NSLayoutConstraint constraintWithItem:button attribute:NSLayoutAttributeBottom relatedBy:NSLayoutRelationEqual toItem:bottomLabel attribute:NSLayoutAttributeTop multiplier:1.0 constant:-8.0f]];
[self.view addConstraint:[NSLayoutConstraint constraintWithItem:button attribute:NSLayoutAttributeLeft relatedBy:NSLayoutRelationEqual toItem:button.superview attribute:NSLayoutAttributeLeft multiplier:1.0 constant:20.0f]];

Expand Down Expand Up @@ -107,6 +109,44 @@ - (void)viewDidLoad {

[self.view addConstraints: [NSLayoutConstraint constraintsWithVisualFormat:@"|-[label1]-[label2(label1)]-[label3(label1)]-[label4(label1)]-|" options:NO metrics:nil views:NSDictionaryOfVariableBindings(label1, label2, label3, label4)]];
[self.view addConstraints: [NSLayoutConstraint constraintsWithVisualFormat:@"V:[topLabel]-[label1]-[label2(label1)]-[label3(label1)]-[label4(label1)]-[button]" options:NO metrics:nil views:NSDictionaryOfVariableBindings(topLabel, button, label1, label2, label3, label4)]];

UIButton* button2 = [UIButton buttonWithType : UIButtonTypeRoundedRect];
[button2 setTitle:@"Add a new label" forState:UIControlStateNormal];
Copy link

@DHowett-MSFT DHowett-MSFT Jan 25, 2017 •
edited by jaredhms
Loading

Choose a reason for hiding this comment

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

The change in this file does not appear to match anything related to the pull request. #Resolved

Copy link
Contributor Author

@jaredhms jaredhms Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Autolayout testing #Resolved


[button2 setTitle:@"Delete the new label" forState:UIControlStateSelected];
[button2 setTitle:@"Delete the new label" forState:UIControlStateSelected | UIControlStateHighlighted];

[button2 setTitleColor:[UIColor blueColor] forState:UIControlStateHighlighted];
[button2 setTitleColor:[UIColor blueColor] forState:UIControlStateHighlighted | UIControlStateSelected];

[button2 setContentHuggingPriority:251 forAxis:UILayoutConstraintAxisVertical];
button2.contentEdgeInsets = UIEdgeInsetsMake(10, 10, 10, 10);
button2.translatesAutoresizingMaskIntoConstraints = NO;
button2.layer.cornerRadius = 5.0f;
button2.backgroundColor = [UIColor lightGrayColor];
[button2 addTarget:self action:@selector(_button2TouchUp:) forControlEvents:UIControlEventTouchUpInside];
[self.view addSubview:button2];

[self.view addConstraint:[NSLayoutConstraint constraintWithItem:button2 attribute:NSLayoutAttributeLeft relatedBy:NSLayoutRelationEqual toItem:buttonLabel attribute:NSLayoutAttributeRight multiplier:1.0 constant:8.0f]];
[self.view addConstraint:[NSLayoutConstraint constraintWithItem:button2 attribute:NSLayoutAttributeBaseline relatedBy:NSLayoutRelationEqual toItem:buttonLabel attribute:NSLayoutAttributeBaseline multiplier:1.0 constant:0]];
}

- (void)_button2TouchUp:(UIButton*)button {
BOOL wasSelected = button.isSelected;
[button setSelected:!wasSelected];

if (wasSelected) {
// Delete the label
[_addRemoveLabel removeFromSuperview];
_addRemoveLabel = nil;
} else {
// Add the new label
_addRemoveLabel = [CenteredAutoLayoutLabel new];
_addRemoveLabel.text = @"Temporary";
_addRemoveLabel.backgroundColor = [UIColor purpleColor];
[_addRemoveLabel sizeToFit];
[self.view addSubview:_addRemoveLabel];
}
}

@end