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

Fix WebView2 navigation with non-ascii URIs #6164

Merged
merged 4 commits into from
Nov 12, 2021
Merged

Conversation

krschau
Copy link
Contributor

@krschau krschau commented Oct 26, 2021

This change fixes an issue where navigating to a URI that contains non-ascii characters would cause the WebView2 to navigate an infinite number of times.

The Xaml WebView2’s Source can be set by us, or by the CoreWebView2 itself. When a Source change comes from the core (like it does on NavigationStarting, for example), we have to set the Source property on our side, but we want to know that this change came from the core and we shouldn’t tell the core to navigate to it again (which would cause an infinite loop). We stop ourselves by setting a flag around the call, and saving the Uri we know we’re already navigating to. If the Uris match, we don’t call into the core.

The issue was caused because we would save a percent-encoded string and compare it to the new URI directly. Instead, we should compare it to the new URI's RawUri, which is also percent-encoded.

When testing manually, selecting the NonAsciiUriTest will give the user a URI and various non-ascii characters that can be copied and pasted for additional testing.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 26, 2021
@krschau krschau added area-WebView and removed needs-triage Issue needs to be triaged by the area owners labels Oct 26, 2021
Copy link
Contributor

@DmitriyKomin DmitriyKomin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@krschau
Copy link
Contributor Author

krschau commented Oct 26, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krschau
Copy link
Contributor Author

krschau commented Oct 27, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

@krschau the WebView2Tests.BasicKeyboardTest failed 6/10 times we retried it, any ideas to improve reliability of this?

@StephenLPeters StephenLPeters added the team-Rendering Issue for the Rendering team label Oct 29, 2021
@krschau
Copy link
Contributor Author

krschau commented Nov 8, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krschau
Copy link
Contributor Author

krschau commented Nov 9, 2021

@StephenLPeters I think WebView2Tests.BasicKeyboardTest is timing out, so I shortened the test (the part I removed is duplicated elsewhere anyway).

@krschau
Copy link
Contributor Author

krschau commented Nov 9, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krschau
Copy link
Contributor Author

krschau commented Nov 10, 2021

I'm also disabling the CopyPasteTest. I'm seeing weird behavior that's worth its own investigation.

@krschau
Copy link
Contributor Author

krschau commented Nov 10, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…ame by just assuming that file can't be skipped
@krschau
Copy link
Contributor Author

krschau commented Nov 11, 2021

The check to see if a PR build must be run was getting caught up by the name of the new html file. Because of the non-ascii characters, it is represented as contained within double quotes, and those are invalid characters for [System.IO.Path]::GetExtension(). At @kmahone's suggestion, I'm putting a try/catch in, and invalid paths we'll just consider unskippable.

@krschau
Copy link
Contributor Author

krschau commented Nov 11, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krschau krschau merged commit 68611fa into main Nov 12, 2021
@krschau krschau deleted the user/krschau/non-ascii branch November 12, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-WebView team-Rendering Issue for the Rendering team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants