Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preventing NullReferenceException on WebAuthenticatorIntermediateActi… #12347

Merged
merged 8 commits into from
Mar 8, 2023
Merged

Conversation

ederbond
Copy link
Contributor

@ederbond ederbond commented Dec 29, 2022

Description of Change

Preventing random NullReferenceException on WebAuthenticatorIntermediateActivity.android.cs

@ghost ghost added the community ✨ Community Contribution label Dec 29, 2022
@ghost
Copy link

ghost commented Dec 29, 2022

Hey there @ederbond! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@LAXStudios
Copy link

Hello,
does this also apply to NetworkOnMainThreadException, wich appears randomly on Authentication for Rest APIs?
Greetings

@ederbond
Copy link
Contributor Author

Hi @LAXStudios, I'm not sure. I just got a NullReferenceException that was happening randomly when I try do authenticate with Identity Server.

@jfversluis jfversluis added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Dec 30, 2022
@jfversluis
Copy link
Member

jfversluis commented Dec 30, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ederbond
Copy link
Contributor Author

Any news about this @jfversluis and @mattleibow ?

@jfversluis jfversluis self-assigned this Jan 30, 2023
@jfversluis
Copy link
Member

@ederbond did you actually verify this works? I can imagine that maybe this line might now break because this value is going to be null?

https://github.com/dotnet/maui/pull/12347/files#diff-9dd7f7fa057499c8357247f9f40fe5abc4890c315fc8451563c48b840c9af101R46

Still, this wouldn't be worse than what is happening now. Just wondering if we have a full solution here.

@ederbond
Copy link
Contributor Author

ederbond commented Feb 1, 2023

Hi @jfversluis I didn't tested it cause I don't know how to compile a modifield version of .NET MAUI locally and use it on my own Maui Project. But this is a random error that happen time to time when I try to authenticate using OAuth. The stacktrace that Visual Studio 2022 shows me, make it cristal clear that there was a NullReference exception on that line.

BTW I couldn't navigate the line you asked me a question on your previous reply:
This link doesn't righlight the line you're refering to on your previous reply:
https://github.com/dotnet/maui/pull/12347/files#diff-9dd7f7fa057499c8357247f9f40fe5abc4890c315fc8451563c48b840c9af101R46

@MartyIX
Copy link
Contributor

MartyIX commented Feb 1, 2023

I don't know how to compile a modifield version of .NET MAUI locally and use it on my own Maui Project.

This might be helpful https://github.com/dotnet/maui/blob/main/.github/DEVELOPMENT.md#compile-using-a-local-bindotnet, see:

dotnet cake --sln="<download_directory>\MauiApp2\MauiApp2.sln" --target=VS

It does not affect your system if you follow the "Compile using a local bin\dotnet" section. If you are on Windows, then #12762 (comment) is important too.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@mattleibow
Copy link
Member

/azp run

@mattleibow mattleibow enabled auto-merge (squash) March 7, 2023 17:21
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ederbond
Copy link
Contributor Author

ederbond commented Mar 8, 2023

Hi, @mattleibow is it going to be merged? Time to time I bump into an app crash because of this. But since it's a random error it's hard to reproduce, I'm quite confident this PR will fix the issue.

@mattleibow mattleibow merged commit a3884c8 into dotnet:main Mar 8, 2023
@mattleibow mattleibow added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Mar 8, 2023
@PureWeen PureWeen added the backport/NO This change should not be backported. It may break customers. label Mar 9, 2023
@PureWeen
Copy link
Member

PureWeen commented Mar 9, 2023

We're currently not planning on backporting this because we don't have a repro and we haven't tested this fix. We feel there is most likely still an issue inside the OnResume call that will occur. Once we can test this fix we will reconsider backporting.

@ederbond
Copy link
Contributor Author

ederbond commented Mar 9, 2023

While that my MAUI app will still be randomly crashing most of the time when I ask it to logoff.
I'm using
<PackageReference Include="IdentityModel.OidcClient" Version="5.2.1" />
to authenticate against my Identity Server.

Sometimes when I call IdentityModel.OidcClient.LogoutAsync() it crashes with a NullReferenceException on that line that I have changed on this PR. But since it doesn't happen very oftem (random) it's hard to create a repro test.

@Px7-941
Copy link

Px7-941 commented Mar 17, 2023

Unfortunately, this pull request does not solve the whole problem. I was able to debug the behavior a bit closer.
When I set the Opera browser as default, no custom tabs are used. The default browser is started.

https://github.com/dotnet/maui/blob/main/src/Essentials/src/WebAuthenticator/WebAuthenticator.android.cs#L78

This probably causes that the activity is not started and so the intent extras are null.

https://github.com/dotnet/maui/blob/main/src/Essentials/src/WebAuthenticator/WebAuthenticatorIntermediateActivity.android.cs

The null check is not sufficient as suspected.
if(extras == null) { return; }
104b87b

I was able to fix it with a null check.

https://github.com/dotnet/maui/blob/main/src/Essentials/src/WebAuthenticator/WebAuthenticatorIntermediateActivity.android.cs#L46

StartActivity(actualIntent ?? Intent);

Maybe someone could test this with the Mi Browser.

@vhugogarcia
Copy link
Contributor

I would like to test the workaround @Px7-941 shared. However, I added the Activity class: WebAuthenticatorIntermediateActivity.cs in the /Platforms/Android folder using same Maui namespace to see if get overwritten without luck.

do you have any idea how to overwrite it in any .NEt MAUI app, so I can test using the Mi Browser?

Thanks in advance

@jstedfast
Copy link
Member

Does anyone have a sample test case to illustrate the original problem or the problem that @PureWeen alludes to potentially existing in issue #13978 ? And is it the same issue that @Px7-941 is hitting?

@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info backport/NO This change should not be backported. It may break customers. backport/suggested The PR author or issue review has suggested that the change should be backported. community ✨ Community Contribution fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants