[ios] fix memory leaks in ContentView #15193

jonathanpeppers · 2023-05-19T21:16:22Z

Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS
Related to: #14664

There are a couple cycles on iOS that causes memory leaks in all controls based on ContentView:

Microsoft.Maui.Controls.ContentView ->
- ContentViewHandler ->
- Microsoft.Maui.Platform.ContentView ->
- CrossPlatformArrange/Measure property ->
Microsoft.Maui.Controls.ContentView cycle

To fix this, I made CrossPlatformArrange/Measure properties obsolete, using the weak View property directly instead. This applies to various other controls like Border, RadioButton, SwipeItemView, SwipeView.

I did not yet fix ScrollView that appears to be more complicated to solve. For now, it supports both code paths.

ScrollView will continue to have leaks in:

Closures with captured variables

maui/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

Line 195 in 3b62095

return (rect) =>

maui/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

Line 227 in 3b62095

return (widthConstraint, heightConstraint) =>

CrossPlatformArrange/Measure

maui/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

Lines 182 to 186 in 3b62095

    
           	CrossPlatformMeasure = ConstrainToScrollView(scrollView.CrossPlatformMeasure, platformScrollView, scrollView), 
        
           	Tag = ContentPanelTag 
        
           }; 
        
           contentContainer.CrossPlatformArrange = ArrangeScrollViewContent(scrollView.CrossPlatformArrange, contentContainer, platformScrollView, scrollView);

mattleibow · 2023-05-24T07:01:13Z

src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

 				View = scrollView.PresentedContent,
+#pragma warning disable CS0618 // Type or member is obsolete
 				CrossPlatformMeasure = ConstrainToScrollView(scrollView.CrossPlatformMeasure, platformScrollView, scrollView),
+#pragma warning restore CS0618
 				Tag = ContentPanelTag
 			};

+#pragma warning disable CS0618 // Type or member is obsolete
 			contentContainer.CrossPlatformArrange = ArrangeScrollViewContent(scrollView.CrossPlatformArrange, contentContainer, platformScrollView, scrollView);
+#pragma warning restore CS0618


@hartez Is this something we need to consider for this PR? @jonathanpeppers will this still cause scroll views to live forever?

Maybe we need a new way that allows for also attaching static methods? This way we can still use the delegates, but be static so they don't capture anything?

@PureWeen what were your findings for using static methods? Did that help?

Yea, if I was able to make a delegate static that would always fix the issue. I think this came down to how much we want to try and protect someone (us?) that's using these APIs. Instead of using a delegate, if you just have to set the View that's a path that's easy for us to make safe. That being said, it is possible to just use statics and make these not leak. Closures being very leaky on iOS isn't anything that's going away, so if you're writing iOS code and you don't have proper tests, or know how to make code not leak it's probably going to leak.

From a design perspective, if we favor the delegates I would be in favor of keeping them and relying on testing and possibly future tooling to keep us safe.

But you can still create leaks with static methods, see the closures here:

maui/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

Line 195 in 3b62095

return (rect) =>

maui/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs

Line 227 in 3b62095

return (widthConstraint, heightConstraint) =>

Should we just avoid using all delegates? Otherwise we need to make it a build error if you use non-static ones -- or closures.

mattleibow · 2023-05-24T07:04:37Z

src/Core/src/Platform/iOS/ContentView.cs

+		[Obsolete("Use View instead. This member causes memory leaks.")]
 		internal Func<double, double, Size>? CrossPlatformMeasure { get; set; }
+		[Obsolete("Use View instead. This member causes memory leaks.")]


@jonathanpeppers @hartez since this is internal, should we maybe change the API so we can use a static delegate and rather pass around some sort of state - I am looking at the code for scroll view and the fact that it seems to sort of make some decisions with these 2 delegates.

I left these here for ScrollView to be fixed later.

How do we enforce the delegate will always be static?

src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt

hartez · 2023-05-24T15:44:59Z

I've got an alternative version of this in the works that I think fixes the problem and preserves the original design intent. Hopefully I can get a PR in soon, I've just got to put out some other fires first. Then we can compare and see how we feel about each option.

jonathanpeppers · 2023-05-24T15:55:25Z

@hartez do you have any qualms about merging this, and add your changes later? We have an email thread that says "CUSTOMER URGENT DTS" in all caps.

rmarinho · 2023-05-28T12:10:05Z

/rebase

rmarinho · 2023-05-30T23:56:17Z

/rebase

Context: https://github.com/jonathanpeppers/MemoryLeaksOniOS Related to: dotnet#14664 There are a couple cycles on iOS that causes memory leaks in all controls based on `ContentView`: * `Microsoft.Maui.Controls.ContentView` -> * `ContentViewHandler` -> * `Microsoft.Maui.Platform` -> * `CrossPlatformArrange/Measure` property -> * `Microsoft.Maui.Controls.ContentView` cycle To fix this, I made `CrossPlatformArrange/Measure` properties obsolete, using the weak `View` property directly instead. This applies to various other controls like `Border`, `RadioButton`, `SwipeItemView`, `SwipeView`. I did not yet fix `ScrollView` that appears to be more complicated to solve. For now, it supports both code paths. `ScrollView` will continue to have leaks in: * Closures with captured variables * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L195 * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L227 * `CrossPlatformArrange/Measure` * https://github.com/dotnet/maui/blob/3b620952425ca3c4542c4632e05d09eb4f583e22/src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs#L182-L186

jonathanpeppers · 2023-06-22T19:14:18Z

Closing this one in favor of: #15303

As the other one also resolves issues around ScrollView.

jonathanpeppers marked this pull request as ready for review May 20, 2023 00:51

jonathanpeppers requested review from hartez, PureWeen and mandel-macaque May 20, 2023 00:51

Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label May 22, 2023

jonathanpeppers added the platform/iOS 🍎 label May 23, 2023

mattleibow reviewed May 24, 2023

View reviewed changes

rolfbjarne approved these changes May 24, 2023

View reviewed changes

hartez mentioned this pull request May 26, 2023

Formalize virtual layout concepts and fix iOS memory leaks #15303

Merged

github-actions bot force-pushed the iOSContentViewLeaks branch from 1d23135 to 030cc1c Compare May 28, 2023 12:10

jonathanpeppers force-pushed the iOSContentViewLeaks branch from 030cc1c to 99c8590 Compare May 30, 2023 14:44

jonathanpeppers added 2 commits May 30, 2023 23:57

Put back method, so no API changes

7091309

github-actions bot force-pushed the iOSContentViewLeaks branch from 99c8590 to 7091309 Compare May 30, 2023 23:57

jonathanpeppers closed this Jun 22, 2023

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] fix memory leaks in ContentView #15193

[ios] fix memory leaks in ContentView #15193

jonathanpeppers commented May 19, 2023 •

edited

Loading

mattleibow May 24, 2023

mattleibow May 24, 2023

mattleibow May 24, 2023

PureWeen May 24, 2023 •

edited

Loading

jonathanpeppers May 24, 2023

mattleibow May 24, 2023

jonathanpeppers May 24, 2023

hartez commented May 24, 2023 •

edited

Loading

jonathanpeppers commented May 24, 2023 •

edited

Loading

rmarinho commented May 28, 2023

rmarinho commented May 30, 2023

jonathanpeppers commented Jun 22, 2023

	CrossPlatformMeasure = ConstrainToScrollView(scrollView.CrossPlatformMeasure, platformScrollView, scrollView),
	Tag = ContentPanelTag
	};

	contentContainer.CrossPlatformArrange = ArrangeScrollViewContent(scrollView.CrossPlatformArrange, contentContainer, platformScrollView, scrollView);

[ios] fix memory leaks in ContentView #15193

[ios] fix memory leaks in ContentView #15193

Conversation

jonathanpeppers commented May 19, 2023 • edited Loading

mattleibow May 24, 2023

Choose a reason for hiding this comment

mattleibow May 24, 2023

Choose a reason for hiding this comment

mattleibow May 24, 2023

Choose a reason for hiding this comment

PureWeen May 24, 2023 • edited Loading

Choose a reason for hiding this comment

jonathanpeppers May 24, 2023

Choose a reason for hiding this comment

mattleibow May 24, 2023

Choose a reason for hiding this comment

jonathanpeppers May 24, 2023

Choose a reason for hiding this comment

hartez commented May 24, 2023 • edited Loading

jonathanpeppers commented May 24, 2023 • edited Loading

rmarinho commented May 28, 2023

rmarinho commented May 30, 2023

jonathanpeppers commented Jun 22, 2023

jonathanpeppers commented May 19, 2023 •

edited

Loading

PureWeen May 24, 2023 •

edited

Loading

hartez commented May 24, 2023 •

edited

Loading

jonathanpeppers commented May 24, 2023 •

edited

Loading