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 all 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
34 changes: 12 additions & 22 deletions Frameworks/UIKit/UIButton.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
//
//******************************************************************************

#import "AssertARCEnabled.h"
#import "Starboard.h"
#import <StubReturn.h>
#import "StubReturn.h"

#import <UIKit/NSString+UIKitAdditions.h>
#import <UIKit/UIButton.h>
Expand Down Expand Up @@ -173,11 +174,11 @@ - (void)_initUIButton {
WXCTextBlock* templateText = rt_dynamic_cast([WXCTextBlock class], [_xamlButton getTemplateChild:@"buttonText"]);

if (templateText) {
_proxyLabel.attach([[_UILabel_Proxy alloc] initWithXamlElement:templateText font:[UIFont buttonFont]]);
_proxyLabel = [[_UILabel_Proxy alloc] initWithXamlElement:templateText font:[UIFont buttonFont]];
}

if (templateImage) {
_proxyImageView.attach([[_UIImageView_Proxy alloc] initWithXamlElement:templateImage]);
_proxyImageView = [[_UIImageView_Proxy alloc] initWithXamlElement:templateImage];
}

_contentVerticalAlignment = UIControlContentVerticalAlignmentCenter;
Expand Down Expand Up @@ -269,7 +270,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 @@ -443,14 +444,14 @@ - (CGRect)titleRectForContentRect:(CGRect)contentRect {
@Status Interoperable
*/
- (void)setEnabled:(BOOL)enabled {
_xamlButton.get().isEnabled = enabled;
_xamlButton.isEnabled = enabled;
}

/**
@Status Interoperable
*/
- (BOOL)isEnabled {
return _xamlButton.get().isEnabled;
return _xamlButton.isEnabled;
}

/**
Expand Down Expand Up @@ -500,7 +501,7 @@ - (UIImage*)currentBackgroundImage {
@Status Interoperable
*/
- (void)setTitle:(NSString*)title forState:(UIControlState)state {
_states[state].title.attach([title copy]);
_states[state].title = [title copy];

// NOTE: check if title is nil before creating inspectableTitle
// createString:nil creates a valid rtString with null comObj
Expand Down Expand Up @@ -552,8 +553,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)];
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 Expand Up @@ -804,16 +804,6 @@ - (BOOL)showsTouchWhenHighlighted {
- (void)dealloc {
XamlRemovePointerEvents([_xamlButton comObj]);
XamlRemoveLayoutEvent([_xamlButton comObj]);

for (auto& state : _states) {
state.second.backgroundImage = nil;
state.second.image = nil;
state.second.textColor = nil;
state.second.title = nil;
}

_xamlButton = nil;
[super dealloc];
}

/**
Expand All @@ -826,7 +816,7 @@ + (UIButton*)buttonWithType:(UIButtonType)type {
[ret setTitleColor:[UIColor colorWithRed:0.0f green:0.47843137f blue:1.0f alpha:1.0f] forState:UIControlStateNormal];
}

return [ret autorelease];
return ret;
}

/**
Expand Down Expand Up @@ -889,15 +879,15 @@ - (UIColor*)currentTitleColor {
@Notes Returns a mock UILabel that proxies some common properties and selectors to the underlying TextBlock
*/
- (UILabel*)titleLabel {
return (UILabel*)[[_proxyLabel retain] autorelease];
return (UILabel*)_proxyLabel;
}

/**
@Status Caveat
@Notes Returns a mock UIImageView that proxies some common properties and selectors to the underlying Image
*/
- (UIImageView*)imageView {
return (UIImageView*)[[_proxyImageView retain] autorelease];
return (UIImageView*)_proxyImageView;
}

/**
Expand Down
26 changes: 5 additions & 21 deletions Frameworks/UIKit/UILabel.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
//
//******************************************************************************

#import "AssertARCEnabled.h"
#import "Starboard.h"

#import <UIKit/NSString+UIKitAdditions.h>
Expand Down Expand Up @@ -102,9 +103,8 @@ - (void)adjustTextLayerSize {
[_textBlock setText:_text];
[_textBlock setFontSize:[_font pointSize]];

StrongId<WUTFontWeight> fontWeight;
fontWeight.attach([WUTFontWeight new]);
fontWeight.get().weight = static_cast<unsigned short>([_font _fontWeight]);
WUTFontWeight* fontWeight =[WUTFontWeight new];
fontWeight.weight = static_cast<unsigned short>([_font _fontWeight]);
[_textBlock setFontWeight:fontWeight];
[_textBlock setFontStyle:static_cast<WUTFontStyle>([_font _fontStyle])];
[_textBlock setFontStretch:static_cast<WUTFontStretch>([_font _fontStretch])];
Expand Down Expand Up @@ -306,7 +306,7 @@ - (void)setText:(NSString*)newStr {
newStr = [newStr description];
}
if (newStr == nil || ![_text isEqual:newStr]) {
_text.attach([newStr copy]);
_text = [newStr copy];

if (_adjustFontSize) {
[self adjustFontSizeToFit];
Expand All @@ -323,7 +323,7 @@ - (void)setText:(NSString*)newStr {
*/
- (void)setAttributedText:(NSAttributedString*)newStr {
UNIMPLEMENTED();
_attributedText.attach([newStr copy]);
_attributedText = [newStr copy];
[self setText:[_attributedText string]];
}

Expand Down Expand Up @@ -381,7 +381,6 @@ - (void)setTextColor:(UIColor*)color {
return;
}
if (![_textColor isEqual:color]) {
[[_textColor retain] autorelease];
_textColor = color;
[self adjustTextLayerSize];
}
Expand Down Expand Up @@ -622,21 +621,6 @@ - (CGRect)textRectForBounds:(CGRect)bounds limitedToNumberOfLines:(NSInteger)max
return ret;
}

/**
@Status Interoperable
*/
- (void)dealloc {
_text = nil;
_font = nil;
_textColor = nil;
_shadowColor = nil;
_highlightedTextColor = nil;
_savedBackgroundColor = nil;
_attributedText = nil;

[super dealloc];
}

/**
@Status Interoperable
*/
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
8 changes: 4 additions & 4 deletions Frameworks/include/UIViewInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,17 @@ class UIViewPrivateState : public LLTreeNode<UIViewPrivateState, UIView> {
userInteractionEnabled = YES;
multipleTouchEnabled = NO;
contentMode = UIViewContentModeScaleToFill;
currentTouches.attach([[NSMutableArray alloc] initWithCapacity:16]);
gestures.attach([NSMutableArray new]);
constraints.attach([NSMutableArray new]);
currentTouches = [[NSMutableArray alloc] initWithCapacity:16];
gestures = [NSMutableArray new];
constraints = [NSMutableArray new];
translatesAutoresizingMaskIntoConstraints = YES;
_isChangingParent = false;
_constraintsNeedUpdate = false;
_contentHuggingPriority.height = 250.0f;
_contentHuggingPriority.width = 250.0f;
_contentCompressionResistancePriority.height = 750.0f;
_contentCompressionResistancePriority.width = 750.0f;
_layoutGuides.attach([NSMutableArray new]);
_layoutGuides = [NSMutableArray new];

memset(&_resizeRoundingError, 0, sizeof(_resizeRoundingError));

Expand Down
8 changes: 6 additions & 2 deletions build/UIKit/lib/UIKitLib.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIBarButtonItem.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIBarItem.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIBezierPath.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIButton.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIButton.mm" >
<ObjectiveCARC>true</ObjectiveCARC>
</ClangCompile>
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIButtonContent.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIClassSwapper.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIColor.mm" />
Expand All @@ -57,7 +59,9 @@
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIImageCachedObject.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIImageNibPlaceholder.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIImageView.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UILabel.mm" />
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UILabel.mm" >
<ObjectiveCARC>true</ObjectiveCARC>
</ClangCompile>
<ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\UIKit\UIButtonProxies.mm">
<ObjectiveCARC>true</ObjectiveCARC>
</ClangCompile>
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
Loading