Bug 6295 - Disposing ToolStrips may lead to crashes
Summary: Disposing ToolStrips may lead to crashes
Status: NEW
Alias: None
Product: Class Libraries
Classification: Mono
Component: Windows.Forms ()
Version: master
Hardware: PC Linux
: Lowest normal
Target Milestone: Community
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2012-07-26 06:17 UTC by knittl89+bugs
Modified: 2017-09-01 11:56 UTC (History)
1 user (show)

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


Attachments
Check if we are actually disposing before calling Dispose on child objects (3.17 KB, patch)
2012-07-26 11:03 UTC, knittl89+bugs
Details
Do not re-calculate AutoSize when assigning null as owner (1.07 KB, patch)
2012-07-26 11:04 UTC, knittl89+bugs
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 for Bug 6295 on GitHub or Developer Community if you have new information to add and do not yet see a matching new report.

If the latest results still closely match this report, you can use the original description:

  • Export the original title and description: GitHub Markdown or Developer Community HTML
  • Copy the title and description into the new report. Adjust them to be up-to-date if needed.
  • Add your new information.

In special cases on GitHub you might also want the comments: GitHub Markdown with public comments

Related Links:
Status:
NEW

Description knittl89+bugs 2012-07-26 06:17:05 UTC
Disposing ToolStrip(DropDownItem)s can lead to crashes under certain circumstances.
Avoid the crashes by checking if we are actually disposing and only then call Dispose() on child objects.
This has been worked out with alan from IRC.

Also, do not calculate the size of the ToolStripItem InternalOwner setter, when it gets set to null.

Here are two patches that avoid the crashes for me:

From d726d849e1d7cf6a7203d66fca2091e529b1211a Mon Sep 17 00:00:00 2001
From: Daniel Knittl-Frank <knittl89+git@googlemail.com>
Date: Wed, 23 May 2012 16:16:01 +0200
Subject: [PATCH 1/2] Properly dispose ToolStrip objects and child objects

Check if `dispose' is actually set, and only then call Dispose on
children. This patch was written with the help of alan from #monodev
IRC.

Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
---
 .../System.Windows.Forms/ToolStrip.cs              | 24 ++++++++++++----------
 .../System.Windows.Forms/ToolStripDropDownItem.cs  | 17 +++++++--------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStrip.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStrip.cs
index 350fa05..45f3ff8 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStrip.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStrip.cs
@@ -727,19 +727,21 @@ namespace System.Windows.Forms
 		{
 			if (!IsDisposed) {
 
-				// Event Handler must be stopped before disposing Items.
-				Events.Dispose();
+				if(disposing) {
+					// Event Handler must be stopped before disposing Items.
+					Events.Dispose();
 
-				CloseToolTip (null);
-				// ToolStripItem.Dispose modifes the collection,
-				// so we iterate it in reverse order
-				for (int i = Items.Count - 1; i >= 0; i--)
-					Items [i].Dispose ();
-					
-				if (this.overflow_button != null && this.overflow_button.drop_down != null)
-					this.overflow_button.drop_down.Dispose ();
+					CloseToolTip (null);
+					// ToolStripItem.Dispose modifes the collection,
+					// so we iterate it in reverse order
+					for (int i = Items.Count - 1; i >= 0; i--)
+						Items [i].Dispose ();
 
-				ToolStripManager.RemoveToolStrip (this);
+					if (this.overflow_button != null && this.overflow_button.drop_down != null)
+						this.overflow_button.drop_down.Dispose ();
+
+					ToolStripManager.RemoveToolStrip (this);
+				}
 				base.Dispose (disposing);
 			}
 		}
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripDropDownItem.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripDropDownItem.cs
index d7ad8e9..4b429c9 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripDropDownItem.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripDropDownItem.cs
@@ -173,14 +173,15 @@ namespace System.Windows.Forms
 		protected override void Dispose (bool disposing)
 		{
 			if (!IsDisposed) {
-				if (this.HasDropDownItems)
-					foreach (ToolStripItem tsi in this.DropDownItems)
-						if (tsi is ToolStripMenuItem)
-							ToolStripManager.RemoveToolStripMenuItem ((ToolStripMenuItem)tsi);
-					
-				if (drop_down != null)
-					ToolStripManager.RemoveToolStrip (drop_down);
-				
+				if(disposing) {
+					if (this.HasDropDownItems)
+						foreach (ToolStripItem tsi in this.DropDownItems)
+							if (tsi is ToolStripMenuItem)
+								ToolStripManager.RemoveToolStripMenuItem ((ToolStripMenuItem)tsi);
+
+					if (drop_down != null)
+						ToolStripManager.RemoveToolStrip (drop_down);
+				}
 				base.Dispose (disposing);
 			}
 		}
-- 
1.7.10.2.742.g71bcd6c


From 5087c606084f6155df24c7ec216c4edfe555ab5f Mon Sep 17 00:00:00 2001
From: Daniel Knittl-Frank <knittl89+git@googlemail.com>
Date: Wed, 23 May 2012 18:15:26 +0200
Subject: [PATCH 2/2] Do not calculate autosize of ToolStripItem when owner
 gets set to null

Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
---
 mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripItem.cs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripItem.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripItem.cs
index 3a3c4eb..68b6091 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripItem.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/ToolStripItem.cs
@@ -1896,7 +1896,8 @@ namespace System.Windows.Forms
 			set {
 				if (this.owner != value) {
 					this.owner = value;
-					this.CalculateAutoSize ();
+					if (this.owner != null)
+						this.CalculateAutoSize ();
 					OnOwnerChanged (EventArgs.Empty);
 				}
 			}
-- 
1.7.10.2.742.g71bcd6c
Comment 1 knittl89+bugs 2012-07-26 11:03:58 UTC
Created attachment 2263 [details]
Check if we are actually disposing before calling Dispose on child objects
Comment 2 knittl89+bugs 2012-07-26 11:04:41 UTC
Created attachment 2264 [details]
Do not re-calculate AutoSize when assigning null as owner