Formalize virtual layout concepts and fix iOS memory leaks #15303

hartez · 2023-05-26T23:07:18Z

Description of Change

Proposed alternative to #15193

Formalizes the concept of the layout backing controls having CrossPlatformMeasure and CrossPlatformArrange methods which they call during the layout process; also allows the handlers to make modifications as necessary to those methods (e.g., normalizing the ScrollView behavior across all the platforms).

This includes the GC tests from #15193 and makes the iOS implementation of ICrossPlatformBacking utilize a weak reference to avoid circular references on that platform.

Issues

Helps #14664, but TBD if it fixes it.

github-actions · 2023-05-26T23:12:54Z

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

AmrAlSayed0 · 2023-05-30T06:53:44Z

src/Core/src/Platform/Android/ContentViewGroup.cs

@@ -42,6 +41,21 @@ public ContentViewGroup(Context context, IAttributeSet attrs, int defStyleAttr,
 			_context = context;
 		}

+		public ICrossPlatformLayout? CrossPlatformLayout
+		{
+			get; set;


I'm not sure about this but I think the platform view should never have a strong reference to the virtual view.
I think the strong references should flow like this:
VirtualView/VirtualView's Properties (eg. ItemSource) --> Handler/Manager/PlatformView

Handler/Manager --> PlatformView/Native Listeners

but whenever you need the opposite direction it should be a weak reference. It's meaningless to have a platform view that the user have already lost the reference to its VirtualView/Handler.

I don't know how to post a comment without it being a review 😅. Feel free to dismiss this one.

That's the point of this PR, to make that weak reference possible. The old implementation was implicitly keeping a strong reference through the internal CrossPlatformMeasure/CrossPlatformArrange Funcs. This way, we have the option of making the reference a weak one (and still allow handlers to modify the CPM/CPA implementations).

The iOS implementation in this PR already uses a weak reference for this. Since we don't currently have leaks on Windows/Android, I've left this as a strong reference to avoid the extra overhead of the weak reference target check. So we could change those to weak references in the future, if we need to.

That said, I could be convinced to make all 3 platforms use weak references just for the symmetry, assuming the performance impact of the weak reference checks is small.

@jonathanpeppers @PureWeen @mattleibow any thoughts on that?

The above property is Android only, so there isn't an issue there? Really only need to be concerned about cycles on iOS/Catalyst.

jonathanpeppers

This looks like it is on the right track to get rid of the delegates for CrossPlatformArrange/Measure.

I would also update these tests:

maui/src/Controls/tests/DeviceTests/Elements/NavigationPage/NavigationPageTests.cs

Lines 268 to 273 in 130f788

    
           					Content = new VerticalStackLayout 
        
           					{ 
        
           						new Label(), 
        
           						new Button(), 
        
           						new CollectionView(), 
        
           					}

maui/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs

Lines 981 to 986 in 130f788

    
           					Content = new VerticalStackLayout 
        
           					{ 
        
           						new Label(), 
        
           						new Button(), 
        
           						new CollectionView(), 
        
           					}

To include a ScrollView & ContentView. In debugging these issues on iOS, I sometimes found these tests would find further problems...

jonathanpeppers · 2023-05-30T15:22:32Z

src/Core/src/Platform/Android/ContentViewGroup.cs

@@ -42,6 +41,21 @@ public ContentViewGroup(Context context, IAttributeSet attrs, int defStyleAttr,
 			_context = context;
 		}

+		public ICrossPlatformLayout? CrossPlatformLayout
+		{
+			get; set;


The above property is Android only, so there isn't an issue there? Really only need to be concerned about cycles on iOS/Catalyst.

jonathanpeppers · 2023-05-30T15:36:42Z

If this "formalizes layout concepts", should there also be a .md file that describes the design? It would go in here:

https://github.com/dotnet/maui/tree/main/docs

src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs

src/Core/src/Core/ILayout.cs

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

rmarinho · 2023-05-31T00:02:57Z

/rebase

Add proxy to avoid UIScrollView event handlers preventing handler GC;

hartez · 2023-06-02T22:57:23Z

If this "formalizes layout concepts", should there also be a .md file that describes the design? It would go in here:

https://github.com/dotnet/maui/tree/main/docs

Assuming this PR gets merged and the layout docs PR gets merged, I'll update the docs to include the changes here.

# Conflicts: # src/Controls/src/Core/Frame/Frame.cs # src/Controls/src/Core/HandlerImpl/ScrollView/ScrollView.Impl.cs # src/Controls/src/Core/HandlerImpl/TemplatedView/TemplatedView.Impl.cs # src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt

jonathanpeppers

I went through the latest changes, and I like this PR over: #15193

However, I'm probably not qualified to know if the iOS/ScrollView changes are OK. Maybe we have to manually test those?

ghost · 2023-07-27T18:35:09Z

🚨 API change(s) detected @davidbritch FYI

AmrAlSayed0 reviewed May 30, 2023

View reviewed changes

jonathanpeppers reviewed May 30, 2023

View reviewed changes

jstedfast requested changes May 30, 2023

View reviewed changes

github-actions bot force-pushed the formalize-virtuallayout branch from 8bd93cc to be866e8 Compare May 31, 2023 00:03

Eilon added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Jun 1, 2023

hartez and others added 5 commits June 1, 2023 10:52

iOS

3b241a9

Windows updates

bb2cbfb

Android

d7fd010

Auto-format source code

1fa10e5

Added ScrollView/ContentView to more tests;

1f4cbe0

Add proxy to avoid UIScrollView event handlers preventing handler GC;

hartez force-pushed the formalize-virtuallayout branch from be866e8 to 1f4cbe0 Compare June 1, 2023 21:36

Add public API tracking info for Tizen

bf65e73

PureWeen requested a review from jstedfast June 5, 2023 15:07

samhouts mentioned this pull request Jun 21, 2023

[net7.0 / net8.0] [iOS] Memory leak when CollectionView.ItemsSource changes #14664

Closed

4 tasks

jonathanpeppers approved these changes Jun 21, 2023

View reviewed changes

- fix windows merge

9d64a1d

PureWeen approved these changes Jun 21, 2023

View reviewed changes

jstedfast approved these changes Jun 21, 2023

View reviewed changes

jonathanpeppers added 2 commits June 22, 2023 12:08

[android] ScrollView leak test requires 2-sec delay

30ed62c

Use AssertionExtensions.Wait()

ad129a6

jonathanpeppers mentioned this pull request Jun 22, 2023

[ios] fix memory leaks in ContentView #15193

Closed

PureWeen approved these changes Jun 22, 2023

View reviewed changes

PureWeen merged commit 5419846 into main Jun 22, 2023

PureWeen deleted the formalize-virtuallayout branch June 22, 2023 21:47

jonathanpeppers added the memory-leak 💦 Memory usage grows / objects live forever label Jul 12, 2023

jonathanpeppers mentioned this pull request Jul 17, 2023

.NET 8 Preview 6 Android crashes with System.MissingMethodException #16166

Closed

PureWeen added t/breaking 💥 needs-breaking-change-doc-created labels Jul 27, 2023

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

samhouts added the platform/iOS 🍎 label Feb 1, 2024

samhouts added the fixed-in-8.0.0-preview.6.8686 Look for this fix in 8.0.0-preview.6.8686! label Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formalize virtual layout concepts and fix iOS memory leaks #15303

Formalize virtual layout concepts and fix iOS memory leaks #15303

hartez commented May 26, 2023 •

edited by jonathanpeppers

Loading

github-actions bot commented May 26, 2023

AmrAlSayed0 May 30, 2023

AmrAlSayed0 May 30, 2023

hartez May 30, 2023

jonathanpeppers May 30, 2023

jonathanpeppers left a comment

jonathanpeppers May 30, 2023

jonathanpeppers commented May 30, 2023

rmarinho commented May 31, 2023

hartez commented Jun 2, 2023

jonathanpeppers left a comment

ghost commented Jul 27, 2023

	Content = new VerticalStackLayout
	{
	new Label(),
	new Button(),
	new CollectionView(),
	}

Formalize virtual layout concepts and fix iOS memory leaks #15303

Formalize virtual layout concepts and fix iOS memory leaks #15303

Conversation

hartez commented May 26, 2023 • edited by jonathanpeppers Loading

Description of Change

Issues

github-actions bot commented May 26, 2023

AmrAlSayed0 May 30, 2023

Choose a reason for hiding this comment

AmrAlSayed0 May 30, 2023

Choose a reason for hiding this comment

hartez May 30, 2023

Choose a reason for hiding this comment

jonathanpeppers May 30, 2023

Choose a reason for hiding this comment

jonathanpeppers left a comment

Choose a reason for hiding this comment

jonathanpeppers May 30, 2023

Choose a reason for hiding this comment

jonathanpeppers commented May 30, 2023

rmarinho commented May 31, 2023

hartez commented Jun 2, 2023

jonathanpeppers left a comment

Choose a reason for hiding this comment

ghost commented Jul 27, 2023

hartez commented May 26, 2023 •

edited by jonathanpeppers

Loading