[2020-02][System.Drawing] Work around libgdiplus Metafile dispose ordering (#20828)

* [System.Drawing] Work around libgdiplus Metafile dispose ordering

* Bump API snapshot submodule

Co-authored-by: monojenkins <jo.shields+jenkins@xamarin.com>

3 files changed, 119 insertions, 2 deletions

diff --git a/external/api-snapshot b/external/api-snapshot
-Subproject e6b00a477284e92f37df1ab942a289253a49502
+Subproject 808e8a9e4be8d38e097d2b0919cac37bc195844
diff --git a/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs b/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs
index b28654bae37..a35359cf01d 100644
--- a/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs
+++ b/mcs/class/System.Drawing/System.Drawing.Imaging/Metafile.cs
@@ -41,6 +41,93 @@ namespace System.Drawing.Imaging {
 	[Editor ("System.Drawing.Design.MetafileEditor, " + Consts.AssemblySystem_Drawing_Design, typeof (System.Drawing.Design.UITypeEditor))]
 	public sealed class Metafile : Image {
 
+		// Non-null if a graphics instance was created using
+		// Graphics.FromImage(this) The metadata holder is responsible for
+		// freeing the nativeImage if the Metadata instance is disposed before
+		// the Graphics instance.
+		private MetafileHolder _metafileHolder;
+
+		// A class responsible for disposing of the native Metafile instance
+		// if it needs to outlive the managed Metafile instance.
+		//
+		// The following are both legal with win32 GDI+:
+		//     Metafile mf = ...; // get a metafile instance
+		//     Graphics g = Graphics.FromImage(mf); // get a graphics instance
+		//     g.Dispose();  mf.Dispose(); // dispose of the graphics instance first
+		//   OR
+		//     mf.Dispose(); g.Dispose();  // dispose of the metafile instance first
+		//
+		// ligbgdiplus has a bug where disposing of the metafile instance first will
+		// trigger a use of freed memory when the graphics instance is disposed, which
+		// could lead to crashes when the native memory is reused.
+		//
+		// The metafile holder is designed to take ownership of the native metafile image
+		// when the managed Metafile instance is disposed while a Graphics instance is still
+		// not disposed (ie the second code pattern above) and to keep the native image alive until the graphics
+		// instance is disposed.
+		//
+		// Note that the following throws, so we only ever need to keep track of one Graphics
+		// instance at a time:
+		//	Metafile mf = ...;  // get a metafile instance
+		//	Graphics g = Graphics.FromImage(mf);
+		//	Graphics g2 = Graphics.FromImage(mf);  // throws OutOfMemoryException on GDI+ on Win32
+		internal sealed class MetafileHolder : IDisposable
+		{
+			private bool _disposed;
+			private IntPtr _nativeImage;
+
+
+			internal bool Disposed { get => _disposed; }
+			internal MetafileHolder()
+			{
+				_disposed = false;
+				_nativeImage = IntPtr.Zero;
+			}
+
+			~MetafileHolder() => Dispose(false);
+
+			public void Dispose()
+			{
+				Dispose(true);
+				GC.SuppressFinalize(this);
+			}
+
+			internal void Dispose(bool disposing)
+			{
+				if (!_disposed)
+				{
+					IntPtr nativeImage = _nativeImage;
+					_nativeImage = IntPtr.Zero;
+					_disposed = true;
+					if (nativeImage != IntPtr.Zero)
+					{
+						Status status = GDIPlus.GdipDisposeImage(nativeImage);
+						GDIPlus.CheckStatus(status);
+					}
+				}
+			}
+
+			internal void MetafileDisposed(IntPtr nativeImage)
+			{
+				_nativeImage = nativeImage;
+			}
+
+			internal void GraphicsDisposed()
+			{
+				Dispose();
+			}
+		}
+
+		internal MetafileHolder AddMetafileHolder()
+		{
+			// If _metafileHolder is not null and hasn't been disposed yet, there's already a graphics instance associated with
+			// this metafile, the native code will return an error status.
+			if (_metafileHolder != null && !_metafileHolder.Disposed)
+				return null;
+			_metafileHolder = new MetafileHolder();
+			return _metafileHolder;
+		}
+
 		// constructors
 
 		internal Metafile (IntPtr ptr)
@@ -324,6 +411,21 @@ namespace System.Drawing.Imaging {
 			GDIPlus.CheckStatus (status);
 		}
 
+		protected override void Dispose(bool disposing)
+		{
+			if (_metafileHolder != null && !_metafileHolder.Disposed)
+			{
+				// There's a graphics instance created from this Metafile,
+				// transfer responsibility for disposing the nativeImage to the
+				// MetafileHolder
+				_metafileHolder.MetafileDisposed(nativeObject);
+				_metafileHolder = null;
+				nativeObject = IntPtr.Zero;
+			}
+
+			base.Dispose(disposing);
+		}
+
 		// methods
 
 		public IntPtr GetHenhmetafile ()
diff --git a/mcs/class/System.Drawing/System.Drawing/Graphics.cs b/mcs/class/System.Drawing/System.Drawing/Graphics.cs
index 5e244d47d60..967fb33715a 100644
--- a/mcs/class/System.Drawing/System.Drawing/Graphics.cs
+++ b/mcs/class/System.Drawing/System.Drawing/Graphics.cs
@@ -48,6 +48,7 @@ namespace System.Drawing
 		private static float defDpiX = 0;
 		private static float defDpiY = 0;
 		private IntPtr deviceContextHdc;
+		private Metafile.MetafileHolder _metafileHolder;
 
 		public delegate bool EnumerateMetafileProc (EmfPlusRecordType recordType,
 							    int flags,
@@ -62,6 +63,13 @@ namespace System.Drawing
 			nativeObject = nativeGraphics;
 		}
 
+		internal Graphics(IntPtr nativeGraphics, Image image) : this(nativeGraphics)
+		{
+			if (image is Metafile mf) {
+				_metafileHolder = mf.AddMetafileHolder();
+			}
+		}
+
 		~Graphics ()
 		{
 			Dispose ();			
@@ -297,6 +305,14 @@ namespace System.Drawing
 				status = GDIPlus.GdipDeleteGraphics (nativeObject);
 				nativeObject = IntPtr.Zero;
 				GDIPlus.CheckStatus (status);
+
+				if (_metafileHolder != null)
+				{
+					var mh = _metafileHolder;
+					_metafileHolder = null;
+					mh.GraphicsDisposed();
+				}
+
 				disposed = true;				
 			}
 
@@ -1773,8 +1789,7 @@ namespace System.Drawing
 
 			Status status = GDIPlus.GdipGetImageGraphicsContext (image.nativeObject, out graphics);
 			GDIPlus.CheckStatus (status);
-			Graphics result = new Graphics (graphics);
-				
+			Graphics result = new Graphics (graphics, image);
 			if (GDIPlus.RunningOnUnix ()) {
 				Rectangle rect  = new Rectangle (0,0, image.Width, image.Height);
 				GDIPlus.GdipSetVisibleClip_linux (result.NativeObject, ref rect);