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: wide string concatenation #40892

Merged
merged 3 commits into from Jan 8, 2024
Merged

fix: wide string concatenation #40892

merged 3 commits into from Jan 8, 2024

Conversation

clavin
Copy link
Member

@clavin clavin commented Jan 5, 2024

Description of Change

In a recent Chromium roll (#40045) we make some changes that were caused by this following revision: Remove Windows-specific wstring variants of StringPrintf() etc..

While the changes we made appear to be correct, we're observing some unicode conversion errors in practice, where the strings we modified are ending up with U+FFFE (the "replacement character") in them, indicating a string encoding conversion issue.

I'm still digging into the specifics of what exactly is going wrong and verifying that this PR is indeed correct, but this PR brings us in line with the changes Chromium made to address these situations: using base::StrCat (and a few base::NumberToWString). (See: multiple revisions attached to Chromium bug 1371963, each starting with Remove usage of wstring StringPrintf():.)

Release Notes

Notes: Fixed default protocol handler behavior on Windows.

@clavin clavin added semver/patch backwards-compatible bug fixes target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 5, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 5, 2024
shell/browser/browser_win.cc Outdated Show resolved Hide resolved
shell/browser/browser_win.cc Outdated Show resolved Hide resolved
shell/browser/browser_win.cc Outdated Show resolved Hide resolved
@clavin
Copy link
Member Author

clavin commented Jan 5, 2024

Looking into the test failures, they look like they might be relevant.

Edit: I was missing a space in one of the strings that was being generated. I double checked all the format strings to make sure they correspond to the new concatenated format, and it looks right to me.

@clavin
Copy link
Member Author

clavin commented Jan 5, 2024

The WoA CI fail seems unrelated, caused by Windows AV flagging the electron binary on our WoA runner machine. Other Windows platforms are succeeding.

@clavin
Copy link
Member Author

clavin commented Jan 5, 2024

From my testing, this fixes the usage of app.setAsDefaultProtocolClient when the electron binary's path has characters that get incorrectly re-encoded.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 6, 2024
@VerteDinde VerteDinde merged commit 37630a6 into main Jan 8, 2024
16 checks passed
@VerteDinde VerteDinde deleted the clavin/fix-wstring-concat branch January 8, 2024 06:02
Copy link

release-clerk bot commented Jan 8, 2024

Release Notes Persisted

Fixed default protocol handler behavior on Windows.

@trop
Copy link
Contributor

trop bot commented Jan 8, 2024

I have automatically backported this PR to "29-x-y", please check out #40908

@trop trop bot added in-flight/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 8, 2024
@trop
Copy link
Contributor

trop bot commented Jan 8, 2024

I have automatically backported this PR to "28-x-y", please check out #40909

@trop trop bot added in-flight/28-x-y merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. and removed target/28-x-y PR should also be added to the "28-x-y" branch. in-flight/28-x-y labels Jan 8, 2024
@trop trop bot removed the in-flight/29-x-y label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants