Skip to content

Preventing NullReferenceException on WebAuthenticatorIntermediateActi… - #12347

Merged
mattleibow merged 8 commits into
dotnet:mainfrom
ederbond:main
Mar 8, 2023
Merged

mattleibow merged 8 commits into
dotnet:mainfrom
ederbond:main

Conversation

@ederbond

@ederbond ederbond commented Dec 29, 2022 •

Copy link
Copy Markdown
Contributor

Description of Change

Preventing random NullReferenceException on WebAuthenticatorIntermediateActivity.android.cs

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

ghost commented Dec 29, 2022

Copy link
Copy Markdown

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
Copy Markdown

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

@ederbond

Copy link
Copy Markdown
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

jfversluis commented Dec 30, 2022 •

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@ederbond

Copy link
Copy Markdown
Contributor Author

Any news about this @jfversluis and @mattleibow ?

@jfversluis jfversluis self-assigned this Jan 30, 2023
@jfversluis

Copy link
Copy Markdown
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

ederbond commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author

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

MartyIX commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@github-actions

github-actions Bot commented Mar 1, 2023

Copy link
Copy Markdown
Contributor

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

@mattleibow

Copy link
Copy Markdown
Member

/azp run

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

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@ederbond

ederbond commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

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

PureWeen commented Mar 9, 2023

Copy link
Copy Markdown
Member

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

ederbond commented Mar 9, 2023 •

Copy link
Copy Markdown
Contributor Author

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

Px7-941 commented Mar 17, 2023

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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