Better error handling for image loading errors on iOS/Android (#849)

* First run at removing async void image update methods

Consistent error logging and IsLoading on Android,iOS,UWP

Move error logging into image handlers for better messages

Add demo of custom ImageRenderer error handling

Update docs

Make the test smaller so the results don't get pushed offscreen

Fix namespace error

* Update error handling for fast image renderer

* Update 37625 test to use image we control

* Add java disposed check to avoid ObjectDisposedException in async operations

* Add disposed checks to legacy renderer; null check element before SetIsLoading

* Check disposed on GetDesiredSize for fast renderer
Use local disposed member where possible for disposed check

* Check for disposal after async handlers in iOS

* Add disposal checks after async methods in Windows

* Reset linker settings on project; reduce redundant casts in ImageViewExtensions

8 files changed, 173 insertions, 50 deletions

diff --git a/Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs b/Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs
index f60acb7e..d0cd2045 100644
--- a/Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs
+++ b/Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs
@@ -1,28 +1,33 @@
 using System;
 using System.Threading.Tasks;
 using Android.Graphics;
-using Java.IO;
 using AImageView = Android.Widget.ImageView;
 
 namespace Xamarin.Forms.Platform.Android
 {
 	internal static class ImageViewExtensions
 	{
-		public static async void UpdateBitmap(this AImageView imageView, Image newImage, Image previousImage = null)
+		// TODO hartez 2017/04/07 09:33:03 Review this again, not sure it's handling the transition from previousImage to 'null' newImage correctly
+		public static async Task UpdateBitmap(this AImageView imageView, Image newImage, Image previousImage = null)
 		{
+			if (imageView == null || imageView.IsDisposed())
+				return;
+
 			if (Device.IsInvokeRequired)
 				throw new InvalidOperationException("Image Bitmap must not be updated from background thread");
 
 			if (previousImage != null && Equals(previousImage.Source, newImage.Source))
 				return;
 
-			((IImageController)newImage).SetIsLoading(true);
+			var imageController = newImage as IImageController;
+
+			imageController?.SetIsLoading(true);
 
-			(imageView as IImageRendererController).SkipInvalidate();
+			(imageView as IImageRendererController)?.SkipInvalidate();
 
 			imageView.SetImageResource(global::Android.Resource.Color.Transparent);
 
-			ImageSource source = newImage.Source;
+			ImageSource source = newImage?.Source;
 			Bitmap bitmap = null;
 			IImageSourceHandler handler;
 
@@ -34,10 +39,7 @@ namespace Xamarin.Forms.Platform.Android
 				}
 				catch (TaskCanceledException)
 				{
-				}
-				catch (IOException ex)
-				{
-					Internals.Log.Warning("Xamarin.Forms.Platform.Android.ImageRenderer", "Error updating bitmap: {0}", ex);
+					imageController?.SetIsLoading(false);
 				}
 			}
 
@@ -47,14 +49,19 @@ namespace Xamarin.Forms.Platform.Android
 				return;
 			}
 
-			if (bitmap == null && source is FileImageSource)
-				imageView.SetImageResource(ResourceManager.GetDrawableByName(((FileImageSource)source).File));
-			else
-				imageView.SetImageBitmap(bitmap);
+			if (!imageView.IsDisposed())
+			{
+				if (bitmap == null && source is FileImageSource)
+					imageView.SetImageResource(ResourceManager.GetDrawableByName(((FileImageSource)source).File));
+				else
+				{
+					imageView.SetImageBitmap(bitmap);
+				}
+			}
 
 			bitmap?.Dispose();
 
-			((IImageController)newImage).SetIsLoading(false);
+			imageController?.SetIsLoading(false);
 			((IVisualElementController)newImage).NativeSizeChanged();
 		}
 	}
diff --git a/Xamarin.Forms.Platform.Android/Extensions/JavaObjectExtensions.cs b/Xamarin.Forms.Platform.Android/Extensions/JavaObjectExtensions.cs
new file mode 100644
index 00000000..007d8759
--- /dev/null
+++ b/Xamarin.Forms.Platform.Android/Extensions/JavaObjectExtensions.cs
@@ -0,0 +1,12 @@
+using System;
+
+namespace Xamarin.Forms.Platform.Android
+{
+	internal static class JavaObjectExtensions
+	{
+		public static bool IsDisposed(this Java.Lang.Object obj)
+		{
+			return obj.Handle == IntPtr.Zero;
+		}
+	}
+}
\ No newline at end of file
diff --git a/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs b/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
index 845ec449..5ca1bd20 100644
--- a/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs
@@ -1,8 +1,10 @@
 using System;
 using System.ComponentModel;
+using System.Threading.Tasks;
 using AImageView = Android.Widget.ImageView;
 using AView = Android.Views.View;
 using Android.Views;
+using Xamarin.Forms.Internals;
 
 namespace Xamarin.Forms.Platform.Android.FastRenderers
 {
@@ -17,30 +19,32 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 
 		protected override void Dispose(bool disposing)
 		{
-			base.Dispose(disposing);
-
 			if (_disposed)
 				return;
 
 			_disposed = true;
 
-			if (!disposing)
-				return;
-
-			if (_visualElementTracker != null)
+			if (disposing)
 			{
-				_visualElementTracker.Dispose();
-				_visualElementTracker = null;
+				if (_visualElementTracker != null)
+				{
+					_visualElementTracker.Dispose();
+					_visualElementTracker = null;
+				}
+
+				if (_visualElementRenderer != null)
+				{
+					_visualElementRenderer.Dispose();
+					_visualElementRenderer = null;
+				}
+
+				if (_element != null)
+				{
+					_element.PropertyChanged -= OnElementPropertyChanged;
+				}
 			}
 
-			if (_visualElementRenderer != null)
-			{
-				_visualElementRenderer.Dispose();
-				_visualElementRenderer = null;
-			}
-
-			if (_element != null)
-				_element.PropertyChanged -= OnElementPropertyChanged;
+			base.Dispose(disposing);
 		}
 
 		public override void Invalidate()
@@ -54,9 +58,9 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 			base.Invalidate();
 		}
 
-		protected virtual void OnElementChanged(ElementChangedEventArgs<Image> e)
+		protected virtual async void OnElementChanged(ElementChangedEventArgs<Image> e)
 		{
-			this.UpdateBitmap(e.NewElement, e.OldElement);
+			await TryUpdateBitmap(e.OldElement);
 			UpdateAspect();
 
 			ElementChanged?.Invoke(this, new VisualElementChangedEventArgs(e.OldElement, e.NewElement));
@@ -77,6 +81,11 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 
 		SizeRequest IVisualElementRenderer.GetDesiredSize(int widthConstraint, int heightConstraint)
 		{
+			if (_disposed)
+			{
+				return new SizeRequest();
+			}
+
 			Measure(widthConstraint, heightConstraint);
 			return new SizeRequest(new Size(MeasuredWidth, MeasuredHeight), MinimumSize());
 		}
@@ -114,7 +123,7 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 
 			_element?.SendViewInitialized(Control);
 		}
-
+		
 		void IVisualElementRenderer.SetLabelFor(int? id)
 		{
 			if (_defaultLabelFor == null)
@@ -144,18 +153,53 @@ namespace Xamarin.Forms.Platform.Android.FastRenderers
 		{
 		}
 
-		protected virtual void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
+		protected virtual async void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
 		{
 			if (e.PropertyName == Image.SourceProperty.PropertyName)
-				this.UpdateBitmap(_element);
+				await TryUpdateBitmap();
 			else if (e.PropertyName == Image.AspectProperty.PropertyName)
 				UpdateAspect();
 
 			ElementPropertyChanged?.Invoke(this, e);
 		}
 
+		protected virtual async Task TryUpdateBitmap(Image previous = null)
+		{
+			// By default we'll just catch and log any exceptions thrown by UpdateBitmap so they don't bring down
+			// the application; a custom renderer can override this method and handle exceptions from
+			// UpdateBitmap differently if it wants to
+
+			try
+			{
+				await UpdateBitmap(previous);
+			}
+			catch (Exception ex)
+			{
+				Log.Warning(nameof(ImageRenderer), "Error loading image: {0}", ex);
+			}
+			finally
+			{
+				((IImageController)_element)?.SetIsLoading(false);
+			}
+		}
+
+		protected async Task UpdateBitmap(Image previous = null)
+		{
+			if (_element == null || _disposed)
+			{
+				return;
+			}
+
+			await Control.UpdateBitmap(_element, previous);
+		}
+
 		void UpdateAspect()
 		{
+			if (_element == null || _disposed)
+			{
+				return;
+			}
+
 			ScaleType type = _element.Aspect.ToScaleType();
 			SetScaleType(type);
 		}
diff --git a/Xamarin.Forms.Platform.Android/Renderers/FileImageSourceHandler.cs b/Xamarin.Forms.Platform.Android/Renderers/FileImageSourceHandler.cs
index 93124fef..95d66fb7 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/FileImageSourceHandler.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/FileImageSourceHandler.cs
@@ -3,6 +3,7 @@ using System.Threading;
 using System.Threading.Tasks;
 using Android.Content;
 using Android.Graphics;
+using Xamarin.Forms.Internals;
 
 namespace Xamarin.Forms.Platform.Android
 {
@@ -17,10 +18,18 @@ namespace Xamarin.Forms.Platform.Android
 		public async Task<Bitmap> LoadImageAsync(ImageSource imagesource, Context context, CancellationToken cancelationToken = default(CancellationToken))
 		{
 			string file = ((FileImageSource)imagesource).File;
+			Bitmap bitmap;
 			if (File.Exists (file))
-				return !DecodeSynchronously ? (await BitmapFactory.DecodeFileAsync (file).ConfigureAwait (false)) : BitmapFactory.DecodeFile (file);
+				bitmap = !DecodeSynchronously ? (await BitmapFactory.DecodeFileAsync (file).ConfigureAwait (false)) : BitmapFactory.DecodeFile (file);
 			else
-				return !DecodeSynchronously ? (await context.Resources.GetBitmapAsync (file).ConfigureAwait (false)) : context.Resources.GetBitmap (file);
+				bitmap = !DecodeSynchronously ? (await context.Resources.GetBitmapAsync (file).ConfigureAwait (false)) : context.Resources.GetBitmap (file);
+
+			if (bitmap == null)
+			{
+				Log.Warning(nameof(FileImageSourceHandler), "Could not find image or image file was invalid: {0}", imagesource);
+			}
+
+			return bitmap;
 		}
 	}
 }
\ No newline at end of file
diff --git a/Xamarin.Forms.Platform.Android/Renderers/ImageLoaderSourceHandler.cs b/Xamarin.Forms.Platform.Android/Renderers/ImageLoaderSourceHandler.cs
index 8d0ae3d9..b22ca485 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/ImageLoaderSourceHandler.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/ImageLoaderSourceHandler.cs
@@ -3,6 +3,7 @@ using System.Threading;
 using System.Threading.Tasks;
 using Android.Content;
 using Android.Graphics;
+using Xamarin.Forms.Internals;
 
 namespace Xamarin.Forms.Platform.Android
 {
@@ -11,12 +12,19 @@ namespace Xamarin.Forms.Platform.Android
 		public async Task<Bitmap> LoadImageAsync(ImageSource imagesource, Context context, CancellationToken cancelationToken = default(CancellationToken))
 		{
 			var imageLoader = imagesource as UriImageSource;
-			if (imageLoader != null && imageLoader.Uri != null)
+			Bitmap bitmap = null;
+			if (imageLoader?.Uri != null)
 			{
 				using (Stream imageStream = await imageLoader.GetStreamAsync(cancelationToken).ConfigureAwait(false))
-					return await BitmapFactory.DecodeStreamAsync(imageStream).ConfigureAwait(false);
+					bitmap =  await BitmapFactory.DecodeStreamAsync(imageStream).ConfigureAwait(false);
 			}
-			return null;
+
+			if (bitmap == null)
+			{
+				Log.Warning(nameof(ImageLoaderSourceHandler), "Could not retrieve image or image data was invalid: {0}", imageLoader);
+			}
+
+			return bitmap;
 		}
 	}
 }
\ No newline at end of file
diff --git a/Xamarin.Forms.Platform.Android/Renderers/ImageRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/ImageRenderer.cs
index 57937d89..43dac1fe 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/ImageRenderer.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/ImageRenderer.cs
@@ -1,10 +1,10 @@
 using System;
 using System.ComponentModel;
+using System.Threading.Tasks;
 using Android.Graphics;
 using Android.Views;
 using AImageView = Android.Widget.ImageView;
 using Xamarin.Forms.Internals;
-using static Xamarin.Forms.Platform.Android.ImageViewExtensions;
 
 namespace Xamarin.Forms.Platform.Android
 {
@@ -20,7 +20,6 @@ namespace Xamarin.Forms.Platform.Android
 
 		public ImageRenderer()
 		{
-            System.Diagnostics.Debug.WriteLine(">>>>> Old Image Renderer");
 			AutoPackage = false;
 		}
 
@@ -39,7 +38,7 @@ namespace Xamarin.Forms.Platform.Android
 			return new FormsImageView(Context);
 		}
 
-		protected override void OnElementChanged(ElementChangedEventArgs<Image> e)
+		protected override async void OnElementChanged(ElementChangedEventArgs<Image> e)
 		{
 			base.OnElementChanged(e);
 
@@ -50,28 +49,63 @@ namespace Xamarin.Forms.Platform.Android
 			}
 
 			_motionEventHelper.UpdateElement(e.NewElement);
-			
-			Control.UpdateBitmap(e.NewElement, e.OldElement);
+
+			await TryUpdateBitmap(e.OldElement);
 
 			UpdateAspect();
 		}
 
-		protected override void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
+		protected override async void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
 		{
 			base.OnElementPropertyChanged(sender, e);
 
 			if (e.PropertyName == Image.SourceProperty.PropertyName)
-				Control.UpdateBitmap(Element);
+				await TryUpdateBitmap();
 			else if (e.PropertyName == Image.AspectProperty.PropertyName)
 				UpdateAspect();
 		}
 
 		void UpdateAspect()
 		{
+			if (Element == null || Control == null || Control.IsDisposed())
+			{
+				return;
+			}
+
 			AImageView.ScaleType type = Element.Aspect.ToScaleType();
 			Control.SetScaleType(type);
 		}
 
+		protected virtual async Task TryUpdateBitmap(Image previous = null)
+		{
+			// By default we'll just catch and log any exceptions thrown by UpdateBitmap so they don't bring down
+			// the application; a custom renderer can override this method and handle exceptions from
+			// UpdateBitmap differently if it wants to
+
+			try
+			{
+				await UpdateBitmap(previous);
+			}
+			catch (Exception ex)
+			{
+				Log.Warning(nameof(ImageRenderer), "Error loading image: {0}", ex);
+			}
+			finally
+			{
+				((IImageController)Element)?.SetIsLoading(false);
+			}
+		}
+
+		protected async Task UpdateBitmap(Image previous = null)
+		{
+			if (Element == null || Control == null || Control.IsDisposed())
+			{
+				return;
+			}
+
+			await Control.UpdateBitmap(Element, previous);
+		}
+
         public override bool OnTouchEvent(MotionEvent e)
         {
             if (base.OnTouchEvent(e))
diff --git a/Xamarin.Forms.Platform.Android/Renderers/StreamImagesourceHandler.cs b/Xamarin.Forms.Platform.Android/Renderers/StreamImagesourceHandler.cs
index 3128995d..a46e3abc 100644
--- a/Xamarin.Forms.Platform.Android/Renderers/StreamImagesourceHandler.cs
+++ b/Xamarin.Forms.Platform.Android/Renderers/StreamImagesourceHandler.cs
@@ -3,6 +3,7 @@ using System.Threading;
 using System.Threading.Tasks;
 using Android.Content;
 using Android.Graphics;
+using Xamarin.Forms.Internals;
 
 namespace Xamarin.Forms.Platform.Android
 {
@@ -11,12 +12,19 @@ namespace Xamarin.Forms.Platform.Android
 		public async Task<Bitmap> LoadImageAsync(ImageSource imagesource, Context context, CancellationToken cancelationToken = default(CancellationToken))
 		{
 			var streamsource = imagesource as StreamImageSource;
-			if (streamsource != null && streamsource.Stream != null)
+			Bitmap bitmap = null;
+			if (streamsource?.Stream != null)
 			{
 				using (Stream stream = await ((IStreamImageSource)streamsource).GetStreamAsync(cancelationToken).ConfigureAwait(false))
-					return await BitmapFactory.DecodeStreamAsync(stream).ConfigureAwait(false);
+					bitmap = await BitmapFactory.DecodeStreamAsync(stream).ConfigureAwait(false);
 			}
-			return null;
+
+			if (bitmap == null)
+			{
+				Log.Warning(nameof(ImageLoaderSourceHandler), "Image data was invalid: {0}", streamsource);
+			}
+
+			return bitmap;
 		}
 	}
 }
\ No newline at end of file
diff --git a/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj b/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj
index bf5344ff..5100b873 100644
--- a/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj
+++ b/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj
@@ -103,6 +103,7 @@
     <Compile Include="AndroidApplicationLifecycleState.cs" />
     <Compile Include="AndroidTitleBarVisibility.cs" />
     <Compile Include="AppCompat\FrameRenderer.cs" />
+    <Compile Include="Extensions\JavaObjectExtensions.cs" />
     <Compile Include="FastRenderers\AccessibilityProvider.cs" />
     <Compile Include="FastRenderers\ButtonRenderer.cs" />
     <Compile Include="AppCompat\FormsViewPager.cs" />