Control ownership (lifecycle and Control-Container relationship)

Oct 23, 2010 at 11:21 PM

Dear Tom,

First I have to thank and praise you for such a great work. I can't find enough superlatives to express my consideration and exhuberation. Oustanding. Amazing. Inspiring.

 

Anyways. I got into some issues this other morning when I woke up and hopefully found the right solution now when it is tomorrow's morning already.

 

My premise was that a control should belong to only one parent (container) at the time and the container is responsible to dispose all the controls which it owns in the moment it is disposed.

Did I get that right?

 

I started by adding asserts into ControlCollection and than followed the code and data around the stacktrace investigating what's triggering them.

An assert would also be appropriate in Control's destructor to prevent the finalizer thread to play with UI data. But there were to many so for now I've opted for logging the orphanes to the console thus having an instant overview on what leaks.

 

 

and finally the dif (if you prefer I could send you a zip archive instead of the unified diff):

 

Index: Controls/Base/CompositeControlBase.cs
===================================================================
--- Controls/Base/CompositeControlBase.cs	(revision 52428)
+++ Controls/Base/CompositeControlBase.cs	(working copy)
@@ -37,9 +37,11 @@
         #region dtor
         protected override void Dispose(bool dispose)
         {
-            base.Dispose(dispose);
-            if (controls != null) controls.Clear();
             styles = null;
+
+			base.Dispose(dispose);
+
+			if (controls != null) controls.Clear(); // remark: called after base.Dispose(dispose) because it backs up GetVisibleControls() which is used by base.Dispose(bool)
         }
         #endregion
         #region props
Index: Controls/Base/Control.cs
===================================================================
--- Controls/Base/Control.cs	(revision 52428)
+++ Controls/Base/Control.cs	(working copy)
@@ -133,6 +133,7 @@
         /// 
         ~Control()
         {
+			Debug.WriteLine(string.Format("WARNING: Finalizing undisposed control [{0})]", this));
             this.ControlFlags |= ControlFlags.Disposing;
             Dispose(false);
             ControlFlags |= ControlFlags.Disposed;
Index: Controls/Base/Decorator.cs
===================================================================
--- Controls/Base/Decorator.cs	(revision 52428)
+++ Controls/Base/Decorator.cs	(working copy)
@@ -31,7 +31,10 @@
         protected virtual void OnControlChange(Control value)
         {
             VisibleControls.Clear();
-            if (value != null) VisibleControls.AddFirst(value);
+			if (value != null) {
+				value.RemoveFromParent();
+				VisibleControls.AddFirst(value);
+			}
             Invalidate(InvalidationFlag.LayoutAppearance);
         }
 
Index: Controls/Classes/ContainerGenerator.cs
===================================================================
--- Controls/Classes/ContainerGenerator.cs	(revision 52428)
+++ Controls/Classes/ContainerGenerator.cs	(working copy)
@@ -46,9 +46,9 @@
 
         public void ReleaseUnusedContainers()
         {
-            foreach (var data in oldContainers.Keys)
+            foreach (var dataAndContainerPair in oldContainers)
             {
-                this.ReleaseContainer(data, oldContainers[data]);
+                this.ReleaseContainer(dataAndContainerPair.Key, dataAndContainerPair.Value);
             }
             oldContainers.Clear();
 
Index: Controls/Classes/ControlCollection.cs
===================================================================
--- Controls/Classes/ControlCollection.cs	(revision 52428)
+++ Controls/Classes/ControlCollection.cs	(working copy)
@@ -65,6 +65,7 @@
         /// <param name="control" />Control to add.
         public void Add(Control control)
         {
+			Debug.Assert(control.Parent == null || control.Parent == Parent); // may equal Parent in case have been previously removed with ClearQuick
             AddLast(control);
             if (notify != null) notify.ControlAdded(control);
         }
@@ -75,6 +76,7 @@
         /// <param name="control" />Control to add.
         public void AddLast(Control control)
         {
+			Debug.Assert(control.Parent == null || control.Parent == Parent); // may equal Parent in case have been previously removed with ClearQuick
             control.NextControl = null;
             control.Parent = Parent;
 
@@ -133,8 +135,11 @@
         /// <param name="control" />Control to remove.
         public void Remove(Control control)
         {
+			bool removed = false;
             if (control == null) return;
+			Debug.Assert(control.Parent == Parent);
             control.Parent = null;
+
             Control prev = First;
             if (prev == null) return;
             if (prev == control)
@@ -142,6 +147,7 @@
                 First = control.NextControl;
                 control.NextControl = null;
                 if (control == Last) Last = null;
+				removed = true;
             }
             else
             {
@@ -152,13 +158,14 @@
                     {
                         prev.NextControl = control.NextControl;
                         if (control == Last) Last = prev;
-                        return;
+						removed = true;
+						break;
                     }
                     prev = current;
                     current = current.NextControl;
                 }
             }
-            if (notify != null) notify.ControlRemoved(control);
+            if (removed && notify != null) notify.ControlRemoved(control);
         }
 
         /// 
@@ -168,6 +175,8 @@
         /// <param name="replacingControl" />Control that replaces  the control.
         public void Replace(Control controlToReplace, Control replacingControl)
         {
+			bool replaced = false;
+			Debug.Assert(controlToReplace.Parent == Parent);
             replacingControl.Parent = Parent;
             replacingControl.NextControl = controlToReplace.NextControl;
             controlToReplace.Parent = null;
@@ -175,6 +184,7 @@
             if (controlToReplace == First)
             {
                 First = replacingControl;
+				replaced = true;
             }
             else
             {
@@ -185,12 +195,13 @@
                     if (next == controlToReplace)
                     {
                         first.NextControl = replacingControl;
+						replaced = true;
                         break;
                     }
                     first = next;
                 }
             }
-            if (notify != null)
+            if (replaced && notify != null)
             {
                 notify.ControlRemoved(controlToReplace);
                 notify.ControlAdded(replacingControl);
@@ -204,6 +215,7 @@
         /// <param name="controlToAppend" />Control to append.
         public void Append(Control fromControl, Control controlToAppend)
         {
+			Debug.Assert(fromControl.Parent == Parent);
             controlToAppend.Parent = Parent;
             if (fromControl.NextControl == null)
             {
@@ -227,11 +239,14 @@
         /// <param name="controlToPrepend" />Control to prepend.
         public void Prepend(Control fromControl, Control controlToPrepend)
         {
+			bool prepended = false;
+			Debug.Assert(fromControl.Parent == Parent);
             controlToPrepend.Parent = Parent;
             if (fromControl == First)
             {
                 First = controlToPrepend;
                 controlToPrepend.NextControl = fromControl;
+				prepended = true;
             }
             else
             {
@@ -243,12 +258,13 @@
                     {
                         controlToPrepend.NextControl = fromControl;
                         first.NextControl = controlToPrepend;
-                        return;
+						prepended = true;
+						break;
                     }
                     first = next;
                 }
             }
-            if (notify != null) notify.ControlAdded(controlToPrepend);
+            if (prepended && notify != null) notify.ControlAdded(controlToPrepend);
         }
 
         /// 
Index: Controls/Forms/Window.cs
===================================================================
--- Controls/Forms/Window.cs	(revision 52428)
+++ Controls/Forms/Window.cs	(working copy)
@@ -152,23 +152,34 @@
         /// <param name="dispose" />true to release both managed and unmanaged resources; false to release only unmanaged resources.
         protected override void Dispose(bool dispose)
         {
+			if (IsDisposed) return;
+
             if (threads != null) threads.Dispose();
             threads = null;
 
             if (toolbarContainer != null)
             {
-                toolbarContainer.Control = null;
+				toolbarContainer.Dispose();
+                //toolbarContainer.Control = null; // XXX reference is already released by Dispose
                 toolbarContainer = null;
             }
             DisposeObject(visibleToolbar);
             DisposeObject(toolbar);
+
+			KeyboardVisibilityProperty.StopAnimation(this);
             isKeyboardVisible = false;
-            if (keyboard != null)
+            if (keyboardContainer != null)
             {
-                VisibleControls.Remove(keyboard);
+                VisibleControls.Remove(keyboardContainer);
+				keyboardContainer.Dispose();
+				keyboardContainer = null;
             }
+
+			controlsPanel = null;
+
             base.Dispose(dispose);
-            controlsPanel = null;
+
+			Debug.Assert(VisibleControls.IsEmpty);
         }
 
         #endregion
Index: Controls/Items/ContainerItem.cs
===================================================================
--- Controls/Items/ContainerItem.cs	(revision 52428)
+++ Controls/Items/ContainerItem.cs	(working copy)
@@ -4,6 +4,7 @@
 using Silvermoon.Controls.Base;
 using System.Drawing;
 using Silvermoon.Shapes;
+using System.Diagnostics;
 
 namespace Silvermoon.Controls
 {
@@ -85,6 +86,7 @@
         {
             if (old != null)
             {
+				Debug.Assert(VisibleControls.Contains(old));
                 VisibleControls.Remove(old);
                 old.Dispose();
             }
Index: Controls/Navigation/NavigationBase.cs
===================================================================
--- Controls/Navigation/NavigationBase.cs	(revision 52428)
+++ Controls/Navigation/NavigationBase.cs	(working copy)
@@ -19,7 +19,7 @@
         protected class Tile : Control, IDecorator
         {
             protected readonly ScaleTransform ScaleTransform = new ScaleTransform();
-            private ControlCollection controls = new ControlCollection(null);
+			private ControlCollection controls;
 
             public INavigationNode Node;
 
@@ -32,6 +32,7 @@
             public Tile()
                 : base()
             {
+				controls = new ControlCollection(this);
                 Transformation = ScaleTransform;
             }
 
Index: Core/Renderer.cs
===================================================================
--- Core/Renderer.cs	(revision 52428)
+++ Core/Renderer.cs	(working copy)
@@ -391,6 +391,7 @@
         public void RenderControl(Control control)
         {
             if (control.IsDisposing) return;
+			Debug.Assert(!control.IsDisposed);
             //Debug.WriteLine("Begin RenderControl " + control.Id);
 #if DEBUG
             try
Index: Core/Screen.cs
===================================================================
--- Core/Screen.cs	(revision 52428)
+++ Core/Screen.cs	(working copy)
@@ -471,6 +471,7 @@
             if (w != null) ActiveWindow = w;
             if (!screenControls.Contains(control))
             {
+				control.RemoveFromParent();
                 AddControl(control);
             }
         }
Oct 26, 2010 at 6:55 AM

A more productive approach on hunting resource leaks would be to record the allocation stack and dump it in the finalizer.

        ~Control()
        {
		Debug.WriteLine(string.Format("WARNING: Finalizing undisposed control [{0}] allocated at {1}", this, this.DebugAllocationStack));
//... the rest unchanged ...
        }
		string DebugAllocationStack = DbgGetAllocationStack();
		private static string DbgGetAllocationStack() {
			try {
				throw new ApplicationException();
			} catch (ApplicationException e) {
				return e.StackTrace;
			}
			throw new Exception();
		}

Same for Texture and Shape to see what's causing the infamous "Call of DeleteXXX after context was disposed.".