Bug 44074 - OnDetachingFrom for Behavior is never called, therefore memory can leak due to event handlers not being unbound
Summary: OnDetachingFrom for Behavior is never called, therefore memory can leak due t...
Status: RESOLVED FIXED
Alias: None
Product: Forms
Classification: Xamarin
Component: Forms (show other bugs)
Version: 2.3.2
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2016-09-07 23:25 UTC by Cliff Cawley
Modified: 2017-05-11 09:41 UTC (History)
8 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Sample to reproduce bug (54.77 KB, application/zip)
2016-09-07 23:25 UTC, Cliff Cawley
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Cliff Cawley 2016-09-07 23:25:23 UTC
Created attachment 17370 [details]
Sample to reproduce bug

== Issue ==
OnDetachingFrom is never called when the view is no longer active, therefore memory leaks can occur due to event handlers not being unbound or other code not being executed.

== Expected ==
OnDetachingFrom is called when the page goes out of context.

== Steps to reproduce ==
Open Solution and run either Droid or iOS. Click the button and notice the Debug log for OnAttached. Click the next button and notice no OnDetachingFrom

== Additional Notes ==
MainPage assignment is used in the example but the same thing happens if you use Navigation and push a view, then tap back.
Comment 1 Rui Marinho 2016-10-04 11:56:24 UTC
Still in 2.3.3
Comment 2 adrianknight89 2016-11-20 22:54:29 UTC
See https://github.com/xamarin/Xamarin.Forms/pull/553
Comment 3 Rui Marinho 2016-12-06 12:19:29 UTC
Should be fixed in 2.3.4-pre2
Comment 4 Ione Souza Junior 2016-12-08 19:36:25 UTC
As alternative way, you can implement destructor method in your behavior class and manually call OnDetachingFrom method.
Comment 5 Parmendra Kumar 2017-02-02 19:04:14 UTC
I have checked this issue with Xamarin.Forms 2.3.4.184-pre1 and this issue has been fixed in iOS and Still getting same issue with Android.

Screencast: https://www.screencast.com/t/fsQLsY3oQHc

ApplicationOutput: https://gist.github.com/Parmendrak/f91c3b69869d455f17f1119378395990

Hence reopened this issue with respect to Android. 


Thanks.
Comment 6 David Waterman 2017-02-13 11:28:24 UTC
The changes made for Bug 44074 and 51503 release in 2.3.4.184-pre2 are causing crashes when the ~ViewElement finalizer code executes on a UWP App; I haven't tried this on other platforms but I suspect they will also have problems.
The issue arises when views with bound triggers are garbage collected.
The changes to ~ViewElement cause calls to event handlers from the finalizer thread leading to COM exceptions. 
According to Miscrosoft https://msdn.microsoft.com/en-us/library/system.object.finalize(v=vs.110).aspx: "The Finalize method is used to perform cleanup operations on unmanaged resources".
These changes are cleaning up managed resources.

Here is an example crash:
System.Runtime.InteropServices.COMException was unhandled
  ErrorCode=-2147417842
  HResult=-2147417842
  Message=The application called an interface that was marshalled for a different thread. (Exception from HRESULT: 0x8001010E (RPC_E_WRONG_THREAD))
  Source=mscorlib
  StackTrace:
       at System.StubHelpers.EventArgsMarshaler.CreateNativePCEventArgsInstance(String name)
       at System.Runtime.InteropServices.WindowsRuntime.PropertyChangedEventArgsMarshaler.ConvertToNative(PropertyChangedEventArgs managedArgs)
       at Xamarin.Forms.BindableObject.OnPropertyChanged(String propertyName)
       at Xamarin.Forms.Element.OnPropertyChanged(String propertyName)
       at Xamarin.Forms.BindableObject.ClearValue(BindableProperty property, Boolean checkaccess)
       at Xamarin.Forms.BindingCondition.TearDown(BindableObject bindable)
       at Xamarin.Forms.TriggerBase.OnDetachingFrom(BindableObject bindable)
       at Xamarin.Forms.TriggerBase.Xamarin.Forms.IAttachedObject.DetachFrom(BindableObject bindable)
       at Xamarin.Forms.AttachedCollection`1.OnDetachingFrom(BindableObject bindable)
       at Xamarin.Forms.VisualElement.Finalize()
	   
And here is sample code to reproduce the crash; from a UWP application displaying Page1.cs press the Clear button followed by the Garbage button to force a garbage collection and crash.	   

using System;
using System.Collections.ObjectModel;
using System.Collections.Specialized;

using Xamarin.Forms;

namespace App4
{
    public class Page1 : ContentPage
    {
        StackLayout _layout = new StackLayout();
        StackLayout _itemsPanel = new StackLayout();
        DataTemplate _itemTemplate = new DataTemplate(CreateBoxView);

        public Page1()
        {
            var viewModel = new ObservableCollection<Item>()
            {
                new Item() { Subject = 65 },
                new Item() { Subject = 66 },
                new Item() { Subject = 65 },
            };

            viewModel.CollectionChanged += OnCollectionChanged;

            _itemsPanel.BindingContext = viewModel;

            foreach (var item in viewModel)
            {
                _itemTemplate.SetValue(BindingContextProperty, item);
                var view = (View)_itemTemplate.CreateContent();
                _itemsPanel.Children.Add(view);
            }
            _layout.Children.Add(_itemsPanel);

            var clearButton = new Button() { Text = "Clear", Command = new Command((o) => viewModel.Clear()) };
            _layout.Children.Add(clearButton);

            var collectButton = new Button() { Text = "Garbage", Command = new Command((o) => GC.Collect()) };
            _layout.Children.Add(collectButton);

            Content = _layout;
        }

        private void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            if (e.Action == NotifyCollectionChangedAction.Reset)
            {
                // reset the list
                _itemsPanel.Children.Clear();
            }
        }

        private static object CreateBoxView()
        {
            var boxView1 = new BoxView() { HeightRequest = 100, Color = new Color(0.55, 0.23, 0.147) };
            var setter1 = new Setter() { Property = BoxView.ColorProperty, Value = "#FF2879DD" };
            var trigger1 = new DataTrigger(typeof(BoxView)) { Binding = new Binding("Subject"), Value = 65 };
            trigger1.Setters.Add(setter1);
            boxView1.Triggers.Add(trigger1);
            return boxView1;
        }

        public class Item
        {
            public int Subject { get; set; }
        }
    }
}
Comment 7 Rui Marinho 2017-03-23 19:24:59 UTC
This issue is actualy fixed, on Android the GC has a diferent behavior so you must click more times and you eventually will get the Detached being called.
Comment 8 Rui Marinho 2017-03-23 19:28:58 UTC
David Waterman please open a new bug with a reproducion exemple and will triage and take a look. 

Thanks.
Comment 9 David Waterman 2017-03-24 12:35:14 UTC
My point is that the fix made for the original bug report is will cause crashes in UWP programs. Simply upgrading from XF 2.3.3 to 2.3.4.184-pre2 caused my UWP to crash due to the changes made for this bug report in the ~ViewElement.

The example code I pasted in comment 6 will demonstrate the problem if you use it build a simple UWP app with 2.3.4.184-pre2.