rotorgames / Rg.Plugins.Popup

Xamarin Forms popup plugin
MIT License
1.15k stars 337 forks source link

System.NullReferenceException When trying to pop a popup #704

Open NoamMani opened 2 years ago

NoamMani commented 2 years ago

Hi,

🐛 Bug Report

On iOS 15, sometimes we got this exception when we try to pop a specific popup:

`Exception: System.NullReferenceException: Object reference not set to an instance of an object. at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f87294 + 0x00460> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f897e8 + 0x0003f> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass20_0.b0 () <0x105f90804 + 0x0007b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass28_0.b0 () <0x105f82440 + 0x0018b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass23_0.b0 () <0x105f81c90 + 0x001e7> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass28_0.b0 () <0x105f82440 + 0x0018b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0

Note we call "RemovePageAsync" and we got an exception from the "AddAsync" method.

Expected behavior

Pop the specific popup.

Configuration

Version: 2.0.0.13

Platform:

Thanks.

AlonRom commented 2 years ago

@rotorgames @KirillAshikhmin can you please help with that?

LuckyDucko commented 2 years ago

Sorry for the late reply, my focus has mainly been on upgrading to MAUI

Could you provide a project example of this? I am currently running the latest on iOS 15 fine.

NoamMani commented 2 years ago

We are unable to reproduce the issue through an example project. In addition, we don't get the exception all the time in our project.

Here are some facts that may help to understand our case:

LuckyDucko commented 2 years ago

hey @NoamMani

this looks like you are solving a similar issue to what i use in my awaitablePopups too.

in specific this page

There must be some functionality that is accidentally trigger this push async twice in your codebase.

AlonRom commented 2 years ago

@LuckyDucko we are using await as well, and this is the only popup we are showing when the app is launched. what can cause this exception? it happens to many iOS users

LuckyDucko commented 2 years ago

Screen Shot 2021-10-21 at 08 47 17

Does this ever cause an exception to be thrown? something similar to System.InvalidOperationException with the message “Collection was modified; enumeration operation may not execute.”

As you are modifying the navstack, indirectly, that the foreach is iterating over?

Could you change it to a for loop and see what happens?

NoamMani commented 2 years ago

No, It doesn't throw the exception you mentioned: System.InvalidOperationException with the message “Collection was modified; enumeration operation may not execute.”

we get the exception I mentioned above: Exception: System.NullReferenceException: Object reference not set to an instance of an object. at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f87294 + 0x00460> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f897e8 + 0x0003f> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass20_0.b__0 () <0x105f90804 + 0x0007b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass28_0.b__0 () <0x105f82440 + 0x0018b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass23_0.b__0 () <0x105f81c90 + 0x001e7> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass28_0.b__0 () <0x105f82440 + 0x0018b> in <c764561bc21b420689a747c209523bf2#9d0a4ec250688d50b764561a6d88ab00>:0

What is most suspicious is that we call "RemovePageAsync" and we get an exception from the "AddAsync" method.

LuckyDucko commented 2 years ago

It is unusual. My only thought is that, the AddAsync was prevented somehow from being completed fully, which held it for long enough that the remove caused the addasync to break, since you removed the page being added.

However, it's just guess work until we have a reproducible. Good to have this record floating about however

AlonRom commented 2 years ago

@LuckyDucko please look at this method:

  public Task PushAsync(PopupPage page, bool animate = true)
    {
        lock (_locker)
        {
            if (_popupStack.Contains(page))
                throw new RGPageInvalidException("The page has been pushed already. Pop or remove the page before to push it again");

            Pushing?.Invoke(this, new PopupNavigationEventArgs(page, animate));

            _popupStack.Add(page);

            var task = InvokeThreadSafe(async () =>
            {
                animate = CanBeAnimated(animate);

                if (animate)
                {
                    page.PreparingAnimation();
                    await AddAsync(page);
                    await page.AppearingAnimation();
                }
                else
                {
                    await AddAsync(page);
                }

                page.AppearingTransactionTask = null;

                Pushed?.Invoke(this, new PopupNavigationEventArgs(page, animate));
            });

            page.AppearingTransactionTask = task;

            return task;
        }
    }

How do you handle a case (which is our case) when this line has been executed successfully:

_popupStack.Add(page);

but the AddAsync has an exception: await AddAsync(page);

the page remains in the stack, and from that point, every pop fails with an exception. Can you help us resolve this?

cc @NoamMani

LuckyDucko commented 2 years ago

Ah and its caught in InvokeThreadSafe which is

private static Task InvokeThreadSafe(Func<Task> action)
        {
            var tcs = new TaskCompletionSource<bool>();

            Device.BeginInvokeOnMainThread(async () =>
            {
                try
                {
                    await action.Invoke();
                    tcs.SetResult(true);
                }
                catch (Exception e)
                {
                    tcs.SetException(e);
                }
            });

            return tcs.Task;
        }

So the pushasync should have an exception attached to it as well. Are you able to show me the exception that comes from awaiting the task of PushAsync? (if im understanding correctly whats going on)

AlonRom commented 2 years ago

@LuckyDucko sure, here the first exception when trying to display a popup:

DialogService.cs(211) - InternalShowPopupAsync: Show popup: MainMapLoaderViewModel 12:34:24 MAPS

MainMapViewModel.cs(151) - InitializeAsync: Exception showing map loader popup: System.NullReferenceException: Object reference not set to an instance of an object. at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f92024 + 0x00460> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f94578 + 0x0003f> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass20_0.b0 () <0x105f9b594 + 0x0007b> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass28_0.b0 () <0x105f8d1d0 + 0x0018b> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at PetActivityMobile.Core.Services.DialogService.InternalShowPopupAsync (Infra.Core.Base.InfraViewModelBase viewModel, System.Object parameter, System.Boolean root) <0x1049d7594 + 0x0024b> in <822344bc1bfb4bb2b95041cf4c9c58c5#7477ef96607d155376acafc3749f9392>:0 at PetActivityMobile.Core.ViewModels.Map.MainMapViewModel.InitializeAsync (System.Object navigationData) <0x1043936a0 + 0x002bf> in <822344bc1bfb4bb2b95041cf4c9c58c5#7477ef96607d155376acafc3749f9392>:0 .

And after that we get an exception on every pop:

DialogService.cs(65) - PopPopupAsync: Exception: System.NullReferenceException: Object reference not set to an instance of an object. at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f92024 + 0x00460> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x105f94578 + 0x0003f> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass20_0.b0 () <0x105f9b594 + 0x0007b> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass28_0.b0 () <0x105f8d1d0 + 0x0018b> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass23_0.b0 () <0x105f8ca20 + 0x001e7> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>cDisplayClass28_0.b0 () <0x105f8d1d0 + 0x0018b> in <c764561bc21b420689a747c209523bf2#7477ef96607d155376acafc3749f9392>:0 at PetActivityMobile.Core.Services.DialogService.PopPopupAsync[TViewModel] () <0x1043c8870 + 0x004bb> in <822344bc1bfb4bb2b95041cf4c9c58c5#7477ef96607d155376acafc3749f9392>:0

LuckyDucko commented 2 years ago

PetActivityMobile.Core.ViewModels.Map.MainMapViewModel.InitializeAsync

Can you confirm this function works correctly as not a popup? Sorry without being able to debug, this is difficult to solve.

AlonRom commented 2 years ago

@LuckyDucko of course, the function works:

We even added a try-catch just in case we have an inner exception: try { await DialogService.ShowPopupAsync(new MainMapLoaderViewModel(), null); } catch (Exception ex) { Logger.Error(LogTags.Maps, $"Exception showing map loader popup: {ex}."); }

but probably because this occurs anyway: _popupStack.Add(page); from now on every pop fails.

Appreciate your assistance here, this is very urgent for us. thanks

LuckyDucko commented 2 years ago

@AlonRom would it be okay if i'm invited into the private repo and i can investigate directly? i'd happily sign anything if there is any concerns about privacy of the code. My email is tysonhooker@live.com.

Otherwise, my best suggestion is removing rg as a nuget, and instead downloading the rg repo, and linking it in manually as a project reference.

Once that has happened, you can add breakpoints the rg code as it runs, and determine the exact point of failure.

AlonRom commented 2 years ago

@LuckyDucko sorry, I'm not allowed to share the private repo. anyway, it won't help because it's something that we can't reproduce in debug. Another thing that may provide some clue, it occurs only at the startup of the app. Could we have some raise condition there?

LuckyDucko commented 2 years ago

I am just taking a guess here looking at the traces.

Can you seperate things out so it works as follows.

-create viewmodel. -create page. -attach viewmodel to page.

Then, can you try and make it work using a Xamarin Forms Modal.

And then make it work using a Popup?

The situation with debug makes me suspicious that this is not an issue with the popup repo itself. Does it work in release mode on the exact same device?

AlonRom commented 2 years ago

we are not able to reproduce in release mode, unfortunately. what you wrote is exactly what we are doing:

_logger?.Info(LogTags.DialogsTag, $"Show popup: {viewModel.GetType().Name}");

        PopupPage page = CreatePopupPage(viewModel.GetType());
        page.BindingContext = viewModel;

        var navigationPage = Application.Current.MainPage as NavigationPage;
        if (!root && navigationPage != null)
        {
            await navigationPage.Navigation.PushPopupAsync(page);
        }
        else
        {
            Application.Current.MainPage = new NavigationPage(page);
        }
LuckyDucko commented 2 years ago

What scenario exactly are you able to reproduce this issue? just iOS 15 devices that are using the full app store release you have?

AlonRom commented 2 years ago

@LuckyDucko exactly

LuckyDucko commented 2 years ago

So then, it would seem that some of the changes, possibly concerning privacy, are what might be forcing the crash on your devices.

See if all your certificates allow for everything you require, as well as entitlements? I am running rg on my own project on iPhone with iOS 15 and it works correctly, including using it in an awaitable context similar to your own.

On startup, you seem to be launching a map as your very first page. Can you try and load just a plain popup with a button that return a bool? so that way, it can have a VM using your DialogService, as well as an awaitable task, just not the map at load time.

AlonRom commented 2 years ago

@LuckyDucko what do you mean by privacy? Not sure about the next step, it works as expected most cases, but clearly there in an inner exception and my map is main screen so I can't replace it.

LuckyDucko commented 2 years ago

@AlonRom the latest iOS had feature sets the emphasised privacy, so i wonder if one of those settings are what is causing the issues?

Well, my understanding was that a map is on a popup? But after your comment i'm not so sure

AlonRom commented 2 years ago

@LuckyDucko it's not a popup, it's the main page.

LuckyDucko commented 2 years ago

So, what popup is causing the error to be thrown?

AlonRom commented 2 years ago

a loader I'm displaying in the InitializeAsync:

public override async Task InitializeAsync(object navigationData) { try { Logger.Info(LogTags.Maps, "MainMap InitializeAsync start."); await DialogService.ShowPopupAsync(new MainMapLoaderViewModel(), null); } catch (Exception ex) { Logger.Error(LogTags.Maps, $"Exception showing map loader popup: {ex}."); }

AlonRom commented 2 years ago

@LuckyDucko I THINK I FOUND THE ISSUE! But I still need your help... We took the Rg Package and added logs inside AddAsync to understand what is the Null Exception:

DialogService.cs(35) - LogPopupNavigationInfo: : PopupPlatformIos.cs(59) - AddAsync - page: PetActivityMobile.Core.CustomControls.Popup.Views.MainMapLoaderView, page.parent: Xamarin.Forms.NavigationPage

DialogService.cs(35) - LogPopupNavigationInfo: : PopupPlatformIos.cs(65) - AddAsync - UIApplication.SharedApplication.KeyWindow is Null

DialogService.cs(35) - LogPopupNavigationInfo: : PopupPlatformIos.cs(72) - AddAsync - UIApplication.SharedApplication.KeyWindow.WindowLevel

DialogService.cs(40) - LogPopupNavigationError: : PopupPlatformIos.cs(124) - AddAsync - Exception System.NullReferenceException: Object reference not set to an instance of an object. at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x106775a1c + 0x00788> in <6845a47b2e33479398dc61567c6f8142#4e6f5acf8cf7edcb8744e41fb036ee40>:0

It looks like "UIApplication.SharedApplication.KeyWindow" is null in some cases when the app loads up, here is the code that cause the Exception: if (UIApplication.SharedApplication.KeyWindow.WindowLevel == UIWindowLevel.Normal) UIApplication.SharedApplication.KeyWindow.WindowLevel = -1;

Can you please suggest a solution for this as soon as possible, we are releasing the app in a few days.

thanks!

LuckyDucko commented 2 years ago

@AlonRom this was a recentish change i believe, by @dimonovdd

after reading this https://stackoverflow.com/questions/54333021/unable-to-access-uiapplication-sharedapplication-keywindow-during-the-launch-of

It seems your issue is that you are throwing a popup before the app has properly started, and this is causing issues

AlonRom commented 2 years ago

@LuckyDucko I don't have any control over this, this is an internal exception in this package. Can you please suggest a solution?

LuckyDucko commented 2 years ago

@AlonRom to be clear, the reason an exception is being thrown is because you are attempting to add a popup too early, and should most likely need to wait, again it is impossible to give any sort of advice as i only have one side of the code.

However, in the link i sent, they mentioned two fixes.

If you are launching a popup as soon as the app is starting, perhaps this code should be called from the OnStart() method available in xamarin forms.

if this fails to help, in the comments they noted that they fixed it by putting

UIWindow window = new UIWindow(UIScreen.MainScreen.Bounds); 
UINavigationController navController = new UINavigationController(); 
window.RootViewController = navController; 
window.MakeKeyAndVisible();

in the .iOS FinishedLaunching Method

AlonRom commented 2 years ago

@LuckyDucko how it can be too early? it's inside the InitilizeAsync method of a ViewModel. I really don't understand where I need to wait, I'm initializing my main page and it's ViewModel properly.

LuckyDucko commented 2 years ago

@AlonRom By too early, i mean in the app lifecycle.

"The key window is the currently visible window that is also the one that receives user input." https://docs.microsoft.com/en-us/dotnet/api/uikit.uiapplication.keywindow?view=xamarin-ios-sdk-12

For a popup to be shown, it must be shown on the keywindow. However, there seems to be an issue where the keywindow is null.

For the keywindow to be null, i don't believe the app has finished the process of starting up by the time you are attempting to access it.

Can you please try the FinishedLaunching Method of this? it would be in the AppDelegate.cs file, where you would have also added the init function for Rg as well.

AlonRom commented 2 years ago

@LuckyDucko my FinishLaunching is pretty big:

` public override bool FinishedLaunching(UIApplication app, NSDictionary launchOptions) { try { Rg.Plugins.Popup.Popup.Init();

if ENABLE_TEST_CLOUD

            Xamarin.Calabash.Start();

endif

            Forms.Init();

            IOCHelper.InitIOC();
            Xamarin.FormsGoogleMaps.Init(AppConsts.iOSGoogleMapsKey);
            Xamarin.FormsGoogleMapsBindings.Init();

            var coreApp = new Core.App();

            // only at the end of the IOC registration
            InitHttpHeaders.Init();

            // Sharpnado init needs to be after Xamarin.Forms.Forms.Init() and before LoadApplication(new App())

            LoadApplication(coreApp);`

where exactly do you suggest adding this code: UIWindow window = new UIWindow(UIScreen.MainScreen.Bounds); UINavigationController navController = new UINavigationController(); window.RootViewController = navController; window.MakeKeyAndVisible();

Won't it cause a duplicate window with the one that will be created when the FinishedLaunching will finish? I'm a bit concerned about adding this just before release.

LuckyDucko commented 2 years ago

@AlonRom The stackoverflow Q doesnt specify, however i believe that the default initialisation only occurs if it is null, so the duplicate shouldnt be an issue..However

var coreApp = new Core.App();

try putting it before this line.

Or, in Core.App() initialisation, move any reference to DialogService into the OnStart() override

AlonRom commented 2 years ago

@LuckyDucko I added more logs and I do finish executing FinishLaunching before I display the popup. So I'm not sure that's the issue. however, because we have 2 roots possible in the app we are replacing the root at the start app:

return await MainThread.InvokeOnMainThreadAsync(async () => NavigationPage newMainPage = new NavigationPage(newRootPage); Page firstPage = mainPage.Navigation.NavigationStack.First(); await DisposePage(firstPage); firstPage.Parent = null; _lastViewModel = newMainPage.Navigation.NavigationStack.LastOrDefault()?.BindingContext as InfraViewModelBase; _lastView = _lastViewModel?.GetType(); _logger.Info(InfraLogTags.NavigationTag, $"Set newMainPage: {newRootPage?.GetType().Name} to Application.Current.MainPage");

Application.Current.MainPage = newMainPage; return newRootPage; }

Can this cause the KeyWindow to be null after it was ok on startup?

dimonovdd commented 2 years ago

Hi. Can you provide a sample of this issue?

AlonRom commented 2 years ago

@dimonovdd I can't share the project but I can provide any relevant information. I think everything related to this issue is in this ticket already,

dimonovdd commented 2 years ago

@AlonRom Can you make a small sample? Even if someone fixes this bug, how can we test a fix?

AlonRom commented 2 years ago

@dimonovdd @LuckyDucko I can't reproduce it in debug and I have no idea what causing this issue, so I can't prepare a sample when I don't know what to put there.

All I know is it's a null exception from the internal code of this package so the fix must be in the package.

2 things can help for now:

  1. Can something cause the KeyWindow to be null after it was ok on startup?

  2. Is there an API in the rg package which cleans this stack? _popupStack.Add(page); this way I can handle this from outside the package. I won't get the loader to show but at least the stack won't be corrupted like it now.

dimonovdd commented 2 years ago

Try calling a pop-up after completing the “OnAppearing" method on your first navigation page

AlonRom commented 2 years ago

@dimonovdd “OnAppearing" is a view method, I have no logic in the view I'm calling the popup in the ViewModel in the InitializeAsync

dimonovdd commented 2 years ago

Do you want to check it out? Or do you want someone to fix this problem for you?

I think your problem is that you are calling a popup before the first page is appeared.

AlonRom commented 2 years ago

@dimonovdd yes, I prefer a fix in the package since it's an internal exception. I think there should be protection. all I'm doing is calling a popup after the page was created in the InitializeAsync method, using OnAppearing and not InitializeAsync is not the way in my opinion.

If I wait as you suggested then I see a blank screen for a short while instead of a popup loader, that's another reason why it's not the solution. the loader should be displayed right away.

dimonovdd commented 2 years ago

Unfortunately this is a limitation that you should accept

LuckyDucko commented 2 years ago

@AlonRom

Are you using the loader as the cover for you switching the root and setting it up and whatnot?

Either case, while we dont have an inbuilt way, you could instead provide a custom navigation via private static IPopupNavigation? _customNavigation; Which i believe should give you access to the stack that would help you sort it out? With such an unusual use case, i think its the best move forward.

@dimonovdd cheers for looking this over, i apologise for late reply, i had a fairly hectic weekend

AlonRom commented 2 years ago

Hi @LuckyDucko I added more logs and we found out why and when it happens but still no solution. it happens currently only on iOS 15+, when a silent push was sent to this device and then the user opens up the app. For some reason, the KeyWindow is null in this case.

@dimonovdd I also tried creating the Window at the beginning of the finishedLaunching but it didn't help as well :/

do you have any suggestions now with this new info?

thanks

dimonovdd commented 2 years ago

@AlonRom Have you tried to do what I suggested?

AlonRom commented 2 years ago

@dimonovdd I tried all suggestions. I added logs for the KeyWindow all around in the navigation service. it's null in this case

dimonovdd commented 2 years ago

@AlonRom

Try calling a pop-up after completing the “OnAppearing" method on your first navigation page

NoamMani commented 2 years ago

@dimonovdd

We added logs to check the key window state inside "OnAppearing" method:

MicrosoftTeams-image (8)

The key window was still null after "OnAppearing" was complete: image

dimonovdd commented 2 years ago

@NoamMani It's very strange, how do you get the Key Window?