[Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage #577

adrianknight89 · 2016-11-30T00:35:31Z

Description of Change

When a MasterDetailPage is nested under NavigationPage, hitting the back button triggers disposal of NavigationPage and results in a hard-crash.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=45702

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense

adrianknight89 · 2016-11-30T00:38:21Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

@@ -173,13 +152,52 @@ protected override void Dispose(bool disposing)
 				if (_drawerLayout != null && _drawerListener != null)
 				{
 					_drawerLayout.RemoveDrawerListener(_drawerListener);


The issue is _drawerLayout is disposed because of the above foreach loop. That block of code should be executed at the end so that we can remove drawer listeners safely before disposing the MDP.

adrianknight89 · 2016-11-30T00:38:52Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

+				{
+					_backgroundDrawable.Dispose();
+					_backgroundDrawable = null;
+				}


Dispose _backgroundDrawable in case it's not null.

adrianknight89 · 2016-11-30T00:39:41Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

@@ -173,13 +152,52 @@ protected override void Dispose(bool disposing)
 				if (_drawerLayout != null && _drawerListener != null)
 				{
 					_drawerLayout.RemoveDrawerListener(_drawerListener);
+				}
+
+				if (_drawerListener != null)


This can be moved to its own if block since the previous _drawerLayout != null && _drawerListener != null may evaluate to false.

adrianknight89 · 2016-11-30T00:40:22Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

@@ -499,7 +514,7 @@ void RegisterToolbar()
 			if (renderer == null)
 				return;

-			_drawerLayout = (DrawerLayout)renderer;
+			_drawerLayout = renderer;


renderer is always a DrawerLayout.

adrianknight89 · 2016-11-30T00:40:44Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

+					navController.PopToRootRequested -= OnPoppedToRoot;
+					navController.InsertPageBeforeRequested -= OnInsertPageBeforeRequested;
+					navController.RemovePageRequested -= OnRemovePageRequested;
+				}


Execute this logic at the end.

hartez

Looks good, just needs some minor changes.

hartez · 2016-12-29T23:37:19Z

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45702.cs

+			};
+		}
+	}
+}


This whole test class can probably just be dropped. It doesn't test the issue in 45702, and I'm not sure there even is a way to test that from inside ControlGallery.

hartez · 2016-12-29T23:37:54Z

Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs


 				Current = null;

+				// dispose child renderers at the end to avoid compliations with MasterDetailPage being nested under NavigationPage


Let's update this comment to something more descriptive, like
"We dispose the child renderers after cleaning up everything related to Drawer Layout in case one of the children is a Master Detail Page (which may dispose of the Drawer Layout)"

adrianknight89 · 2016-12-30T00:09:45Z

Done.

hartez · 2016-12-30T17:20:27Z

@adrianknight89 Sorry for the trouble, but could you rebase this? I think the .pfx files are out of date and it's failing to build (on our CI server).

adrianknight89 · 2016-12-30T17:24:19Z

Done.

hartez · 2016-12-30T17:24:35Z

@adrianknight89 Thanks!

hartez · 2016-12-30T22:19:11Z

Only failing test is the broken CellsGalleryImageUrlCellList test on iOS 10; not an issue with these changes.

…tailPage (xamarin#577) * fix navigation page dispose crash * changes after review

* Add pressed and released events to Button * Update ButtonRenderer.cs * Apply safely casting to android button renderer * Use safety casting for Android buttin renderer * [Windows] Fix modal pages being laid out below soft buttons (#395) * Add sample HanselForms and TwitterDemo to ControlGallery (#651) * [Controls] Add Hanselforms sample * Remove extra twitter sample * [Controls]Add TwitterDemo sample * [Controls] Fix build * Slider should show user-set value on initial load (#378) * [UWP] Use toolbar foreground color on primary items (#640) * Avoid duplicating code in OmPlatform (#591) * [iOS] Entry should not pass a newline to the next responder (#397) * UITextField should not return so that the next field does not get passed a newline * Added code sample * [XamlC] import members on x:Static and factories (#642) * [Xaml] support short Properties for PropertyCondition (#645) * Xamlc compile data triggers (#648) * [Xaml] DataTrigger and PropertyCondition no longer use a ServiceProvider * [XamlC] avoid generating ServiceProvider for unused ProvideValue * fix tests * Fix comment typo * [UWP] Fix TextBox style for foreground focus color (#618) * Adding image to use for CellsGalleryImageUrlCellList UI test * Update ImageCellListPage to use an image we control; Update CellsGalleryImageUrlCellList test to wait longer than 1s for images to load if necessary * fix nre when changing content in datepickerselected (#494) * Make CellsGalleryImageUrlCellList test finish early if possible * [iOS] Change keyboard type while keyboard is visible (#443) * Change keyboard while changing text * add sample code * [Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage (#577) * fix navigation page dispose crash * changes after review * [XamlC] detect duplicate x:Name at compile time (#655) * [XamlC] detect duplicate x:Name at compile time * invoking methods with the right arguments produces better results * Make UWP toolbar display rules consistent with other platforms (#638) * Allow subscriber to be collected if MessagingCenter is the only reference to it (#617) * Repro * Make messaging center callbacks weak references * Preserve attribute * Fix test method name * Watch for collection of actual delegate target instead of wrapper delegate * Preserve the original platform instance when changing main page * Better tests for lambda situations * Update tests, make callback target a weakreference if it's the subscriber * Ensure old Platform MessagingCenter subs are gone before creating new Platform * [iOS] Prevent multiple ListView cells from being swiped simultaneously (#578) * disable multiple cell swipe * add sample code * refactored * convert to weakreference * remove null setting * change weakreference setting place * remove if * revert isopen changes * add instructions * [WinRT/UWP] Apply BackgroundColor to Stepper buttons (#581) * [WinRT/UWP] Apply BackgroundColor to Stepper buttons * Add explanatory text; use nameof * Move explanatory text to a label * Return group instead of internal class (#461) * [iOS/Android] Move Map camera to correct region on layout change (#548) * Move to region on layout change * remove visibility check * [iOS] Platform specifics for controlling Picker SelectedIndex change behavior (#540) * picker selected index could change when picker view is dismissed * use enum * [iOS] Ignore intermittent failing test on XTC (#666) * [UITest] Update to UITest 2.0.5 (#665) * Rebase the current branch onto upstream latest

dnfclas added the cla-already-signed label Nov 30, 2016

adrianknight89 commented Nov 30, 2016

View reviewed changes

rmarinho approved these changes Nov 30, 2016

View reviewed changes

hartez self-assigned this Dec 8, 2016

hartez suggested changes Dec 29, 2016

View reviewed changes

hartez approved these changes Dec 30, 2016

View reviewed changes

adrianknight89 added 2 commits December 30, 2016 11:23

fix navigation page dispose crash

b231a4a

changes after review

2c6b1f5

hartez merged commit ee608ba into xamarin:master Dec 30, 2016

rookiejava pushed a commit to rookiejava/Xamarin.Forms that referenced this pull request Jan 9, 2017

[Android] Fix NavigationPage dispose crash when it parents a MasterDe…

35317ce

…tailPage (xamarin#577) * fix navigation page dispose crash * changes after review

samhouts added this to the 2.5.1 milestone May 5, 2018

samhouts modified the milestones: 2.5.1, 2.3.4 Jun 27, 2018

mattleibow added a commit that referenced this pull request Jan 28, 2021

[Share] Renamed DataTransfer to Share. Fixes #577 (#586)

911e65e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage #577

[Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage #577

adrianknight89 commented Nov 30, 2016 •

edited by samhouts

Loading

adrianknight89 Nov 30, 2016 •

edited

Loading

adrianknight89 Nov 30, 2016

adrianknight89 Nov 30, 2016

adrianknight89 Nov 30, 2016

adrianknight89 Nov 30, 2016

hartez left a comment

hartez Dec 29, 2016

hartez Dec 29, 2016

adrianknight89 commented Dec 30, 2016

hartez commented Dec 30, 2016 •

edited

Loading

adrianknight89 commented Dec 30, 2016

hartez commented Dec 30, 2016

hartez commented Dec 30, 2016


		Current = null;

		// dispose child renderers at the end to avoid compliations with MasterDetailPage being nested under NavigationPage

[Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage #577

[Android] Fix NavigationPage dispose crash when it parents a MasterDetailPage #577

Conversation

adrianknight89 commented Nov 30, 2016 • edited by samhouts Loading

Description of Change

Bugs Fixed

PR Checklist

adrianknight89 Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

adrianknight89 Nov 30, 2016

Choose a reason for hiding this comment

adrianknight89 Nov 30, 2016

Choose a reason for hiding this comment

adrianknight89 Nov 30, 2016

Choose a reason for hiding this comment

adrianknight89 Nov 30, 2016

Choose a reason for hiding this comment

hartez left a comment

Choose a reason for hiding this comment

hartez Dec 29, 2016

Choose a reason for hiding this comment

hartez Dec 29, 2016

Choose a reason for hiding this comment

adrianknight89 commented Dec 30, 2016

hartez commented Dec 30, 2016 • edited Loading

adrianknight89 commented Dec 30, 2016

hartez commented Dec 30, 2016

hartez commented Dec 30, 2016

adrianknight89 commented Nov 30, 2016 •

edited by samhouts

Loading

adrianknight89 Nov 30, 2016 •

edited

Loading

hartez commented Dec 30, 2016 •

edited

Loading