[Windows] Fix password Entry crash when setting text on empty field#33891
[Windows] Fix password Entry crash when setting text on empty field#33891kubaflo merged 4 commits intodotnet:inflight/currentfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows-specific crash in password Entry fields when text is programmatically set on an empty field. The crash occurred because the obfuscation logic calculated a negative insert position when the field was empty, causing ArgumentOutOfRangeException in the String.Insert() method.
Changes:
- Added
Math.Max(0, ...)to clamp the insert position to prevent negative values in the password obfuscation logic - Added a device test to verify that setting text on an empty password Entry does not crash
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Core/src/Platform/Windows/MauiPasswordTextBox.cs | Fixed the insert position calculation to prevent negative indices by using Math.Max(0, ...) |
| src/Controls/tests/DeviceTests/Elements/Entry/EntryTests.Windows.cs | Added test to verify password Entry doesn't crash when text is set on empty field |
|
|
||
| // Setting Text on an empty password entry triggers TextChanging event which previously threw ArgumentOutOfRangeException |
There was a problem hiding this comment.
The comment is not properly indented. It should be indented with two tabs to align with the code below it.
| // Setting Text on an empty password entry triggers TextChanging event which previously threw ArgumentOutOfRangeException | |
| // Setting Text on an empty password entry triggers TextChanging event which previously threw ArgumentOutOfRangeException |
📋 PR Finalization ReviewTitle: ✅ GoodCurrent: Description: ✅ Good✅ Excellent description:
Code Review: ✅ Passed✅ Looks Good1. Simple, surgical fix
2. Proper test coverage
3. Defensive programming approach
🟡 Minor Suggestions (Optional)1. Test comment indentation (line 128)
2. Test assertion could be stronger
Verdict: ✅ This PR is ready for merge. The fix is minimal, correct, well-documented, and properly tested. |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please implement the ai's suggestions?
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33891Or
iex "& { $(irm https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33891" |
The “Remove branch not guarded” concern was reviewed and validated. The delete path is only triggered through user-driven input where indices remain within valid bounds, and no crash was reproducible during testing. Unlike the insert scenario, it is not affected by programmatic edge cases, so no change is required. |
Checked the AI suggestions and addressed the applicable ones. |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
🚦 Gate - Test Before and After Fix📊 Expand Full Gate —
|
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
📱 EntryTests (PasswordEntryPlatformSetDoesNotCrash) Category=Entry |
✅ FAIL — 104s | ❌ FAIL — 18s |
🔴 Without fix — 📱 EntryTests (PasswordEntryPlatformSetDoesNotCrash): FAIL ✅ · 104s
Determining projects to restore...
Restored D:\a\1\s\src\Controls\src\Xaml\Controls.Xaml.csproj (in 1.29 min).
Restored D:\a\1\s\src\Controls\src\Xaml.Design\Controls.Xaml.Design.csproj (in 22 ms).
Restored D:\a\1\s\src\Controls\tests\DeviceTests\Controls.DeviceTests.csproj (in 1.3 min).
Restored D:\a\1\s\src\Controls\src\Core.Design\Controls.Core.Design.csproj (in 18 ms).
Restored D:\a\1\s\src\Controls\src\BindingSourceGen\Controls.BindingSourceGen.csproj (in 71 ms).
Restored D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj (in 211 ms).
Restored D:\a\1\s\src\Core\maps\src\Maps.csproj (in 309 ms).
Restored D:\a\1\s\src\Controls\Maps\src\Controls.Maps.csproj (in 462 ms).
Restored D:\a\1\s\src\TestUtils\src\DeviceTests\TestUtils.DeviceTests.csproj (in 130 ms).
Restored D:\a\1\s\src\TestUtils\src\DeviceTests.Runners\TestUtils.DeviceTests.Runners.csproj (in 373 ms).
Restored D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj (in 3.65 sec).
Restored D:\a\1\s\src\Graphics\src\Graphics.Win2D\Graphics.Win2D.csproj (in 22 ms).
Restored D:\a\1\s\src\Essentials\src\Essentials.csproj (in 75 ms).
Restored D:\a\1\s\src\Core\tests\DeviceTests.Shared\Core.DeviceTests.Shared.csproj (in 100 ms).
Restored D:\a\1\s\src\Core\src\Core.csproj (in 133 ms).
Restored D:\a\1\s\src\TestUtils\src\DeviceTests.Runners.SourceGen\TestUtils.DeviceTests.Runners.SourceGen.csproj (in 4.74 sec).
C:\Users\VssAdministrator\.nuget\packages\microsoft.windowsappsdk.base\1.8.250831001\buildTransitive\Microsoft.WindowsAppSDK.SelfContained.targets(75,9): error : WindowsAppSDKSelfContained requires a supported Windows architecture. [D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj::TargetFramework=net10.0-windows10.0.19041.0]
Build FAILED.
C:\Users\VssAdministrator\.nuget\packages\microsoft.windowsappsdk.base\1.8.250831001\buildTransitive\Microsoft.WindowsAppSDK.SelfContained.targets(75,9): error : WindowsAppSDKSelfContained requires a supported Windows architecture. [D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj::TargetFramework=net10.0-windows10.0.19041.0]
0 Warning(s)
1 Error(s)
Time Elapsed 00:00:08.52
🟢 With fix — 📱 EntryTests (PasswordEntryPlatformSetDoesNotCrash): FAIL ❌ · 18s
Determining projects to restore...
All projects are up-to-date for restore.
C:\Users\VssAdministrator\.nuget\packages\microsoft.windowsappsdk.base\1.8.250831001\buildTransitive\Microsoft.WindowsAppSDK.SelfContained.targets(75,9): error : WindowsAppSDKSelfContained requires a supported Windows architecture. [D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj::TargetFramework=net10.0-windows10.0.19041.0]
Build FAILED.
C:\Users\VssAdministrator\.nuget\packages\microsoft.windowsappsdk.base\1.8.250831001\buildTransitive\Microsoft.WindowsAppSDK.SelfContained.targets(75,9): error : WindowsAppSDKSelfContained requires a supported Windows architecture. [D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj::TargetFramework=net10.0-windows10.0.19041.0]
0 Warning(s)
1 Error(s)
Time Elapsed 00:00:08.13
⚠️ Issues found
- ❌ EntryTests (PasswordEntryPlatformSetDoesNotCrash) FAILED with fix (should pass)
📁 Fix files reverted (2 files)
eng/pipelines/ci-copilot.ymlsrc/Core/src/Platform/Windows/MauiPasswordTextBox.cs
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33891 | Math.Max(0, start - lengthDifference) to clamp Insert index |
⏳ PENDING (Gate env blocked) | MauiPasswordTextBox.cs |
Logically correct; env build error blocked validation |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Early-return if (realText.Length == 0) return passwordText — short-circuit before index arithmetic |
MauiPasswordTextBox.cs |
Build env: WindowsAppSDKSelfContained requires a supported Windows architecture |
|
| 2 | try-fix (claude-sonnet-4.6) | Math.Clamp(start - lengthDifference, 0, realText.Length) on Insert branch + Math.Clamp(start, 0, ...) on Remove branch |
MauiPasswordTextBox.cs |
Same build env blocker | |
| 3 | try-fix (gpt-5.3-codex) | Caller-side normalization of SelectionStart before calling DetermineTextFromPassword |
MauiPasswordTextBox.cs |
Same build env blocker | |
| 4 | try-fix (gpt-5.4) | Character-level merge rewrite — prefix/suffix merge eliminating Insert/Remove index arithmetic entirely | MauiPasswordTextBox.cs |
Same build env blocker | |
| 5 | try-fix (claude-opus-4.6, cross-poll) | Reentrancy guard: if (_internalChangeFlag) return + if (lengthDifference > 0 && SelectionStart < lengthDifference) return in OnNativeTextChanging |
MauiPasswordTextBox.cs |
Same build env blocker | |
| PR | PR #33891 | Math.Max(0, start - lengthDifference) to clamp Insert index |
MauiPasswordTextBox.cs |
Logically correct; same env blocks validation |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Reentrancy guard _internalChangeFlag to skip DetermineTextFromPassword for programmatic changes (→ Attempt 5) |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | Yes | Range-based buffer update from change event data (same pattern as Attempt 4 rewrite) |
| gpt-5.4 | 2 | Yes | Set Password directly in TextBoxExtensions.UpdateText instead of Text to bypass SelectionStart path |
Exhausted: Yes — all empirical testing blocked by WindowsAppSDKSelfContained requires a supported Windows architecture build environment limitation. Solution space thoroughly explored across 5 distinct fix strategies.
Selected Fix: PR's fix (Math.Max(0, start - lengthDifference)) — all 5 try-fix alternatives were blocked by the same build environment limitation. No empirical comparison was possible. The PR's fix is the simplest and most surgical. Among alternatives: Attempt 2 (Math.Clamp) is slightly more comprehensive (bounds-checks the upper end too), but unverified empirically. The PR's one-line fix directly addresses the root cause without changing method structure.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #33334 confirmed, Windows-only crash in DetermineTextFromPassword |
| Gate | ❌ FAILED | Windows — build env error (not code): WindowsAppSDKSelfContained requires a supported Windows architecture. "Without fix" failed as expected (✅, 104s), but "with fix" also failed (❌, 18s — same build error, test never executed). |
| Try-Fix | ✅ COMPLETE | 5 attempts, 0 passing — all |
| Report | ✅ COMPLETE |
Summary
PR #33891 fixes a Windows-only ArgumentOutOfRangeException crash in MauiPasswordTextBox.DetermineTextFromPassword with a minimal one-line change (Math.Max(0, start - lengthDifference)). The fix is logically correct. The Gate failed due to a build environment limitation (WindowsAppSDKSelfContained requires a supported Windows architecture in Graphics.csproj) that is unrelated to the PR code — the same error appeared in all 5 try-fix attempts. Empirical validation was not possible in this environment.
Root Cause
When a password Entry is empty (realText = "") and text is programmatically set, TextChanging fires with SelectionStart = 0. DetermineTextFromPassword("", 0, "test") computes lengthDifference = 4 and calls "".Insert(0 - 4, "●●●●") → Insert(-4, ...). In unchecked arithmetic, -4 wraps to a large uint value (4294967295), causing ArgumentOutOfRangeException. The production crash trace (issue #33334) confirms startIndex ('4294967295') must be less than or equal to '0'.
Fix Quality
The Insert branch fix is logically correct and minimal. Math.Max(0, start - lengthDifference) prevents the negative index for the reported crash scenario.
Remaining issues:
-
⚠️ Gate failure is environmental (non-blocking): The build error inGraphics.csprojis a pre-existing CI infrastructure issue unrelated to this PR. This cannot be fixed within the PR. -
⚠️ Remove branch not guarded (potential crash — same root cause class): Theelse if (lengthDifference < 0)branch callsrealText.Remove(start, -lengthDifference)without bounds validation. The PR author argues this path is only triggered through user-driven input where indices remain valid. However, ifstart > realText.Lengthorstart + (-lengthDifference) > realText.Lengthcan occur (e.g., concurrent updates, paste operations with unusual SelectionStart), this will throwArgumentOutOfRangeExceptionwith the same crash pattern. The PR author should either add defensive bounds checking or explain concretely why this path is safe. -
ℹ️ Indentation of test comment (trivial): Line 136 in
EntryTests.Windows.cs— the comment indentation appears to match the surrounding code (3 tabs). The original copilot suggestion (2 tabs) appears to have been incorrect. This is not a concern. -
✅ Test assertion is adequate:
Assert.Equal("test", passwordTextBox.Text)verifies the text was correctly set. This was addressed by the PR author.
Selected Fix
Selected Fix: PR — it is logically sound for the reported scenario. No try-fix candidate empirically outperformed it (all blocked). Before merging, the Remove branch defensive coding should be discussed or addressed.
…33891) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause When a password `Entry` field is empty and text is programmatically set, the obfuscation logic calculates a negative insert position `(0 - 4 = -4)`. String. `Insert()` doesn't accept negative indices, causing an `ArgumentOutOfRangeException` crash in windows. ### Description of Change Added `Math.Max(0, ...)` to clamp the insert position to a minimum of 0, preventing negative **values.** ### Issues Fixed Fixes #33334 ### Platforms Tested - [x] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/43993544-7700-4bff-a04c-d34b999ac962" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/e7f12e20-1cd8-45e7-b07d-bfe7ebac6248" /> |
…otnet#33891) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause When a password `Entry` field is empty and text is programmatically set, the obfuscation logic calculates a negative insert position `(0 - 4 = -4)`. String. `Insert()` doesn't accept negative indices, causing an `ArgumentOutOfRangeException` crash in windows. ### Description of Change Added `Math.Max(0, ...)` to clamp the insert position to a minimum of 0, preventing negative **values.** ### Issues Fixed Fixes dotnet#33334 ### Platforms Tested - [x] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/43993544-7700-4bff-a04c-d34b999ac962" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/e7f12e20-1cd8-45e7-b07d-bfe7ebac6248" /> |
…33891) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause When a password `Entry` field is empty and text is programmatically set, the obfuscation logic calculates a negative insert position `(0 - 4 = -4)`. String. `Insert()` doesn't accept negative indices, causing an `ArgumentOutOfRangeException` crash in windows. ### Description of Change Added `Math.Max(0, ...)` to clamp the insert position to a minimum of 0, preventing negative **values.** ### Issues Fixed Fixes #33334 ### Platforms Tested - [x] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/43993544-7700-4bff-a04c-d34b999ac962" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/e7f12e20-1cd8-45e7-b07d-bfe7ebac6248" /> |
…33891) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause When a password `Entry` field is empty and text is programmatically set, the obfuscation logic calculates a negative insert position `(0 - 4 = -4)`. String. `Insert()` doesn't accept negative indices, causing an `ArgumentOutOfRangeException` crash in windows. ### Description of Change Added `Math.Max(0, ...)` to clamp the insert position to a minimum of 0, preventing negative **values.** ### Issues Fixed Fixes #33334 ### Platforms Tested - [x] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/43993544-7700-4bff-a04c-d34b999ac962" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/e7f12e20-1cd8-45e7-b07d-bfe7ebac6248" /> |
…33891) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause When a password `Entry` field is empty and text is programmatically set, the obfuscation logic calculates a negative insert position `(0 - 4 = -4)`. String. `Insert()` doesn't accept negative indices, causing an `ArgumentOutOfRangeException` crash in windows. ### Description of Change Added `Math.Max(0, ...)` to clamp the insert position to a minimum of 0, preventing negative **values.** ### Issues Fixed Fixes #33334 ### Platforms Tested - [x] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/43993544-7700-4bff-a04c-d34b999ac962" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/e7f12e20-1cd8-45e7-b07d-bfe7ebac6248" /> |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root Cause
When a password
Entryfield is empty and text is programmatically set, the obfuscation logic calculates a negative insert position(0 - 4 = -4). String.Insert()doesn't accept negative indices, causing anArgumentOutOfRangeExceptioncrash in windows.Description of Change
Added
Math.Max(0, ...)to clamp the insert position to a minimum of 0, preventing negative values.Issues Fixed
Fixes #33334
Platforms Tested
Screenshots