Fix NullReferenceException when displaying Toast or SnackBar

[maxkoshevoi]

Description of Change

Added retry logic for Android for getting renderer.

Bugs Fixed

• Fixes [Bug] NullReferenceException when displaying Toast or SnackBar #959

API Changes

None

Behavioral Changes

None

PR Checklist

• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Changes adhere to coding standard

• Updated documentation

[maxkoshevoi]

@VladislavAntonyuk, @AndreiMisiukevich, @pictos Could you please take a look at this? Is there a better fix for this issue?

[VladislavAntonyuk]

It looks good in terms of code stability, but we do not show the Snackbar.
I suggest to throw custom exception that this page cannot be the "parent" for the Snackbar.
Thoughts?

[maxkoshevoi]

this page cannot be the "parent" for the Snackbar

@VladislavAntonyuk The thing is that the page can be "parent" for the Snackbar. This code fails only if we try to show Snackbar at the same time when we change current page. Both pages are fully capable of displaying Snackbar, but when we are in the middle of transitioning between them, GetRenderer() returns null. (see issue description for more info)

And throwing exception here would mean that every call to DisplayToastAsync and DisplaySnackBarAsync needs to be wrapped in try-catch, cause it can fail in rare cases.

[VladislavAntonyuk]

As described in the issue, you run the Snackbar on page appearing.
Looks like that sometimes the Snackbar Snow method is called before the renderer set the control.
So with your code you won't see the Snackbar, while you actually expect it.
What about other platforms? Can you reproduce it for UWP or iOS?
I would suggest to create some kind of retry policy: if the getRenderer returns null, we can wait for a few seconds and try again.
But the root cause of the issue from my point of view is that OnAppearing calls Snackbar before we actually can get the view

[pictos]

@maxkoshevoi Is this just occur with Shell?

[maxkoshevoi]

Can you reproduce it for UWP or iOS?

@VladislavAntonyuk Didn't try to reproduce it on other platforms. My app can be built only for Android as of now. I don't have time to create a new project and test it on UWP (don't have Mac to test it on iOS) right now.

I would suggest to create some kind of retry policy

Updated PR with retry logic instead of just returning. Tested and it works with 50 ms retry.
Forgot that we can use await in platform specific logic too 😄

But the root cause of the issue from my point of view is that OnAppearing calls Snackbar before we actually can get the view

Well, in my use case, OnAppearing triggers a network request, and if it fails (which happens really quickly if device isn't connected to the internet) toast is displayed. Seems like a valid use of a toast. Also if I stay on that page, toast will appear just fine. The issue occurs if I immediately switch back to previous page.
In another case this issue happens when page is trying to display toast and is being closed at the same time, so it's not about appearing too early.

Is this just occur with Shell?

@pictos I'm not sure about that. What I can say is that it definitely happens in Shell app

1. When switching between tabs
2. When Poping page of the stack

[pictos]

@maxkoshevoi because of that I said to wait until we see the issue and discuss it first.

Since this only happens on Shell (did you test on TabbedPage?) and in a very specific moment, navigation I would that's a Shell problem and not our lib.
Shell has some navigations corners that lead into edge cases just like this one, and I don't see value in trying to fix it here, because, maybe, this is a workaround and not a fix.

Also, let's move this discussion to the issue and please can you provide a repro?

[pictos]

@maxkoshevoi can you solve these conflicts?

[maxkoshevoi]

@pictos @brminnick Why is this retargeted to develop? It's a bugfix that doesn't change an API.

[pictos]

@maxkoshevoi I believe that happens because of the nullable PR, at that time we moved all PRs to target develop

[AndreiMisiukevich]

?

// COMMENT EXPLAINING THE PURPOSE OF THIS METHOD
static async Task<IVisualElementRenderer?> GetRendererWithRetries(Page page, int retryCount = 5)
{
	var renderer = Platform.GetRenderer(page);
	if (renderer != null || retryCount <= 0)
		return renderer;

	await Task.Delay(50);
	return await GetRendererWithRetries(page, retryCount - 1);
}

As for Show method.
Can we preserve Return-Type to be void?

Maybe we can put the code after GetRendererWithRetries call to ContinueWith?

internal void Show(Page page, SnackBarOptions arguments) {
    GetRendererWithRetries(sender).ContinueWith(....);
}

Any thoughts?

[maxkoshevoi]

COMMENT EXPLAINING THE PURPOSE OF THIS METHOD

Good point. I'll add it

Can we preserve Return-Type to be void?

@AndreiMisiukevich Can I ask why do you want to preserve it as void? This is internal API, so users won't notice it.
Anyway, ContinueWith won't help you here, since you still need to wait until task completes (otherwise method will return while task is still running).

There are two options to preserve void return type and return when method executed to completion:

1. Replace await with .Result
2. Replace Task.Delay with Thread.Sleep

[TheCodeTraveler]

Can we preserve Return-Type to be void?

@AndreiMisiukevich Can you explain why you need to preserve the void return type?

@maxkoshevoi No, do not ever use Thread.Sleep or .Result, and you shouldn't use .ContinueWith except in rare circumstances.

If we do need to preserve the void return type, we'll use the .SafeFireAndForget() extension method. However, this would mean that Show is now a fire-and-forget method.

I recommend using async Task... and propagating the Task upwards through the API, but I'll wait to see Andrei's reason for needing to keep the void return type.

[maxkoshevoi]

@maxkoshevoi No, do not ever use Thread.Sleep or .Result

Yeah, it's bad to use them since they block the thread. But that's the only way to preserve void return type.
Just provided some options to do that =)

If we do need to preserve the void return type, we'll use the .SafeFireAndForget() extension method.

@brminnick It won't help here since

• It doesn't return result of the task (and we need result of GetRendererWithRetries)
• It doesn't wait until task completes, so it breaks existing execution flow

I recommend using async Task... and propagating the Task upwards through the API

Totally agree. I also don't see a reason not to do that

[AndreiMisiukevich]

Can we preserve Return-Type to be void?

@AndreiMisiukevich Can you explain why you need to preserve the void return type?

I thought it is related to public API

[AndreiMisiukevich]

Well, let's add a comment explaining what this method does, and then we will be able to get it merged, I believe. If there are no other objections.

And if you wish you can consider the "recursion-style" implementation of this method that I proposed above (up to you).

Thank you!

[maxkoshevoi]

@AndreiMisiukevich Added comment and changed implementation to recursive one.

[AndreiMisiukevich]

@pictos can you please take a look again?

[pictos]

just this little change

[pictos]

@maxkoshevoi thanks for the changes, I'll take a look today night or tomorrow during the day.

[maxkoshevoi]

@pictos Just a friendly reminder that PR is ready for merge

[pictos]

@maxkoshevoi we have a TestPage folder inside the sample app, do you think that is possible to add a scenario there that proves that this PR fixes the issue? That isn't required to get this merged

[maxkoshevoi]

@pictos It should be possible, but complicated. We would need to create mini-Shell environment with two pages inside our sample app

[pictos]

In that case let ignore it for now. Thanks again for this PR