Fixed Android's StreamImageSourceService.LoadDrawableAsync() by jstedfast · Pull Request #14109 · dotnet/maui

jstedfast · 2023-03-21T20:48:35Z

Description of Change

The original issue is that images were being loaded/set on the Image control in a non-UI thread causing an unhandled exception stating, "Only the original thread that created a view hierarchy can touch its view".

Android's StreamImageSourceService.LoadDrawableAsync() incorrectly had a ConfigureAwait(false) on an awaited call to get the image data stream which meant that the subsequent LoadImageFromStream() call would not happen on the original thread.

As added precaution, modified MauiCustomViewTarget.java to post view.setImageDrawable() calls to the main (UI) thread as well.

Issues Fixed

Fixes #14052
Fixes #15397

Redth · 2023-03-22T02:09:45Z

@jonathanpeppers @mattleibow I seem to recall some issues with using post on the android side previously, does that sound familiar at all?

jonathanpeppers

I think the main thing missing here is some kind of test.

Can we add a test to this file?

https://github.com/dotnet/maui/blob/main/src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs

jonathanpeppers · 2023-03-22T02:19:00Z

    protected void onResourceCleared(@Nullable Drawable placeholder) {
-		this.view.setImageDrawable(placeholder);
+		post(() -> this.view.setImageDrawable(placeholder));


Found the docs on this callback:

https://bumptech.github.io/glide/javadocs/490/com/bumptech/glide/request/target/CustomViewTarget.html

It doesn't say what thread this is on, but it must be the main thread because view.setImageDrawable would throw?

Reviewing some examples on GitHub:

https://github.com/CzBiX/v2ex-android/blob/f6a79a91b84ae8030930231d965845798c4ce0a8/app/src/main/java/com/czbix/v2ex/parser/AsyncImageGetter.java#L196-L198

https://github.com/signalapp/Signal-Android/blob/2cef06cd6ef2ed70fdab3fcb525df9909f18aa5a/app/src/main/java/org/thoughtcrime/securesms/util/AvatarUtil.java#L71-L74

I don't think we need this post(), did it fix something when you added it?

It did not fix anything specific, no. I think that, technically, only the ConfigureAwait(false) removal is needed. The post() calls were suggested changes that might help make MAUI more robust against this type of problem if customers did things on other threads.

jonathanpeppers · 2023-03-22T02:20:12Z

+        post(() -> {
+            // set the image
+            this.view.setImageDrawable(resource);


This change to post() might be OK, did this fix something?

Removing the ConfigureAwait(false) from StreamImageSourceService.Android.cs seemed to have worked by itself. The post() changes didn't fix anything on their own (i.e. I tested adding back the ConfigureAwait(false) and ended up with errors again).

Maybe we don't need this anymore then? Is there any fears that there are related issues?

jstedfast · 2023-03-22T11:58:54Z

I'll work on adding a test today

Redth · 2023-03-22T19:23:51Z

@jstedfast this looks potentially related: #14128

Could you maybe validate your changes against that repro too?

jstedfast · 2023-03-23T14:53:07Z

@Redth yea, that definitely looks related. I'll test it to verify.

jstedfast · 2023-03-27T17:50:47Z

@Redth I have confirmed that issue #14128 is a duplicate and is fixed by this change.

jstedfast · 2023-03-31T15:10:01Z

I've been unable to get the DeviceTests to build or run. Would anyone else be willing to write the test(s) for this?

mattleibow

How come the tests are not catching this? Are we missing something in the core device tests that we should be adding this?

mattleibow

How come the tests are not catching this? Are we missing something in the core device tests that we should be adding this?

jstedfast · 2023-05-04T17:46:06Z

Any chance we can get this merged?

/cc @Redth @jonathanpeppers

jonathanpeppers · 2023-05-04T19:28:10Z

Can we add a test? #14109 (review)

jstedfast · 2023-05-04T20:10:51Z

@jonathanpeppers How would you write a test for this?

jonathanpeppers · 2023-05-04T20:15:46Z

We should be able to add a test here that loads an image from an EmbeddedResource as a Stream:

https://github.com/dotnet/maui/blob/main/src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs

Previously, any Image using a Stream would break? Correct?

mattleibow · 2023-05-05T04:59:49Z

Does the test fail without your changes? Was the issue also related to setting the property from a background thread, or if the stream went to the background thread? Maybe the task should be Source = ImageSource.FromStream(() => Task.Run(() => stream))?

I do know we have images in our sample that I am sure load, but we do basic things.

jstedfast · 2023-05-05T16:29:00Z

@mattleibow The customer sample did not set the stream in a background thread - they set it in the main thread.

jstedfast · 2023-05-05T17:43:31Z

Argh, not sure why it's getting a null stream for the embedded image when run on the bots but not locally. Doesn't seem to make sense :(

jstedfast · 2023-05-05T21:15:05Z

Still getting failures. It looks like the failures are only on iOS.

Is there something specifically different about iOS that embedded resources would be named differently or somehow not embedded, perhaps?

jonathanpeppers · 2023-05-05T21:37:50Z

@mattleibow do you know?

System.ArgumentNullException : ArgumentNull_Generic Arg_ParamName_Name, ctx

Stack trace
   at ObjCRuntime.NativeObjectExtensions.GetNonNullHandle(INativeObject self, String argumentName)
   at CoreAnimation.CALayer.RenderInContext(CGContext ctx)
   at Microsoft.Maui.DeviceTests.AssertionExtensions.ToBitmap(UIView view, IMauiContext mauiContext)
   at Microsoft.Maui.DeviceTests.AssertionExtensions.AssertContainsColor(UIView view, UIColor expectedColor, IMauiContext mauiContext, Nullable`1 tolerance)
   at Microsoft.Maui.DeviceTests.ImageTests.&lt;&gt;c__DisplayClass2_0.&lt;&lt;ImageSetFromStreamRenders&gt;b__0&gt;d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.Maui.Dispatching.DispatcherExtensions.&lt;&gt;c__DisplayClass3_0.&lt;&lt;DispatchAsync&gt;b__0&gt;d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.Maui.Dispatching.DispatcherExtensions.&lt;&gt;c__DisplayClass2_0`1.&lt;&lt;DispatchAsync&gt;b__0&gt;d[[System.Boolean, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.Maui.DeviceTests.ImageTests.ImageSetFromStreamRenders()
--- End of stack trace from previous location ---

The stream isn't null or this stacktrace would be a lot different.

Transition back to the calling thread context (which should theoretically be the main thread). Fixes issue #14052

jonathanpeppers

LGTM, assuming the new test verifies the fix.

janseris · 2023-06-30T11:16:44Z

When is this fix released in a non-preview MAUI version?

PureWeen · 2023-08-04T20:54:36Z

If we backport this, we should regenerate the android file

PureWeen · 2023-08-09T15:30:25Z

/backport to net7.0

github-actions · 2023-08-09T15:30:41Z

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/5811072947

rmarinho requested review from Redth, jonathanpeppers and mattleibow March 21, 2023 22:08

jonathanpeppers reviewed Mar 22, 2023

View reviewed changes

Eilon added the area-image Image loading, sources, caching label Mar 22, 2023

jstedfast force-pushed the dev/jestedfa/android-image-load-wrong-thread branch 3 times, most recently from d26c380 to b526ccb Compare March 27, 2023 17:11

jstedfast requested a review from jonathanpeppers April 18, 2023 18:47

mattleibow reviewed Apr 28, 2023

View reviewed changes

jstedfast force-pushed the dev/jestedfa/android-image-load-wrong-thread branch from 91d41c3 to 59cfd4c Compare May 4, 2023 14:49

jonathanpeppers reviewed May 5, 2023

View reviewed changes

Comment thread src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs Outdated

jonathanpeppers reviewed May 5, 2023

View reviewed changes

Comment thread src/Controls/tests/DeviceTests/Elements/Image/ImageTests.cs Outdated

Comment thread src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj Outdated

PureWeen reviewed May 8, 2023

View reviewed changes

Comment thread src/Core/src/ImageSources/StreamImageSourceService/StreamImageSourceService.Android.cs

jstedfast added 4 commits May 22, 2023 10:32

Fixed Android's StreamImageSourceService.LoadDrawableAsync()

1290d00

Transition back to the calling thread context (which should theoretically be the main thread). Fixes issue #14052

Try harder in the Android backend to load images in the main (UI) thread

8574886

Reverted the onResourceCleared() use of post()

e1eb4f9

Added test case

83580c6

jstedfast force-pushed the dev/jestedfa/android-image-load-wrong-thread branch from 3046dc3 to 83580c6 Compare May 22, 2023 14:32

jstedfast added 2 commits May 22, 2023 10:48

Move the test to be Android-specific to avoid iOS failures.

09aa288

fixed the build

9bb8005

jstedfast requested review from PureWeen, jonathanpeppers and mattleibow May 23, 2023 16:22

jstedfast enabled auto-merge May 23, 2023 16:26

jonathanpeppers approved these changes May 23, 2023

View reviewed changes

Comment thread src/Core/src/ImageSources/StreamImageSourceService/StreamImageSourceService.Android.cs

jstedfast merged commit ecd03a3 into main May 23, 2023

jstedfast deleted the dev/jestedfa/android-image-load-wrong-thread branch May 23, 2023 16:59

jonathanpeppers mentioned this pull request Jun 12, 2023

ImageSource.FromStream Leads to Crash #15397

Closed

UkeHa mentioned this pull request Jul 24, 2023

White or Black screen navigation on Android #13698

Closed

samhouts added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Aug 4, 2023

github-actions Bot mentioned this pull request Aug 9, 2023

[net7.0] Fixed Android's StreamImageSourceService.LoadDrawableAsync() #16640

Merged

PureWeen added the backport/approved After some discussion or review, this PR or change was approved to be backported. label Aug 9, 2023

ne0rrmatrix mentioned this pull request Sep 19, 2023

Threading issues on android not backported to dotnet 7.x jerry08/Woka#5

Closed

jstedfast mentioned this pull request Oct 9, 2023

[Android] Image.Source = ImageSource.FromResource() & ImageSource.FromStream() are not reliably integrating back with the MainThread #17857

Closed

github-actions Bot locked and limited conversation to collaborators Dec 12, 2023

samhouts added the platform/android label Feb 1, 2024

Eilon added area-controls-image Image control and removed area-image Image loading, sources, caching labels May 10, 2024

samhouts added the fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! label Aug 2, 2024

Conversation

jstedfast commented Mar 21, 2023 • edited by samhouts Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Issues Fixed

Uh oh!

Redth commented Mar 22, 2023

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jstedfast Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

jstedfast Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

mattleibow Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

jstedfast commented Mar 22, 2023

Uh oh!

Redth commented Mar 22, 2023

Uh oh!

jstedfast commented Mar 23, 2023

Uh oh!

jstedfast commented Mar 27, 2023

Uh oh!

jstedfast commented Mar 31, 2023

Uh oh!

mattleibow left a comment

Choose a reason for hiding this comment

Uh oh!

mattleibow left a comment

Choose a reason for hiding this comment

Uh oh!

jstedfast commented May 4, 2023

Uh oh!

jonathanpeppers commented May 4, 2023

Uh oh!

jstedfast commented May 4, 2023

Uh oh!

jonathanpeppers commented May 4, 2023

Uh oh!

mattleibow commented May 5, 2023

Uh oh!

Uh oh!

jstedfast commented May 5, 2023

Uh oh!

jstedfast commented May 5, 2023

Uh oh!

Uh oh!

Uh oh!

jstedfast commented May 5, 2023

Uh oh!

jonathanpeppers commented May 5, 2023

Uh oh!

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janseris commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PureWeen commented Aug 4, 2023

Uh oh!

PureWeen commented Aug 9, 2023

Uh oh!

github-actions Bot commented Aug 9, 2023

Uh oh!

Reviewers

Assignees

Labels

jstedfast commented Mar 21, 2023 •

edited by samhouts

Loading

janseris commented Jun 30, 2023 •

edited

Loading