Repository navigation
[Android] Fix ScrollToAsync UI-thread livelock caused by the MapRequestScrollTo retry loop - #39213
Conversation
…#39190) <!-- 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! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Description of Change <!-- Enter description of the fix in this section --> Regenerate only `issue-triage.lock.yml` using gh-aw v0.86.2 against deployed main `8c974ad7037c6e171e951883c752bbcea1514cc3`, following dotnet#39065. Shared `pat_pool.md` frontmatter changed before merge, leaving the triage lock stale. Actual production runs [37503999448](https://github.com/dotnet/maui/actions/runs/37503999448) and [37503998307](https://github.com/dotnet/maui/actions/runs/37503998307) failed before inference/publication with `E009 CONFIG_HASH_MISMATCH`. The compiler changes only `frontmatter_hash` in the first metadata line: - Previous: `d2c371bf924eb254532ec722f31d817ebdb42fd4e5565b4952a40e09e465e0d1` - Regenerated: `eb116514f337eb247c8767becceaefdd9cfb69da65044098b5cdc510e7c2d153` The new value matches the actual activation runtime recomputation in run 37503999448. All bytes after that metadata line are unchanged: authorization, GPT-only inference, permission boundaries, publication, hash checks, action/container pins and job conditions are preserved. Add a deployment note requiring dependent lock recompilation after shared import frontmatter changes. No workflow source, shared import, script or C# changes. Configuration validation: - `git merge-base --is-ancestor 8c974ad HEAD` — passed before editing; initial HEAD was exactly that deployed squash merge. - `gh aw compile issue-triage --strict --no-check-update` — passed; 1 workflow succeeded, 0 warnings, compiler v0.86.2. - `gh aw hash-frontmatter .github/workflows/issue-triage.md` — passed; output equals regenerated metadata and actual runtime hash above. - `test "$(gh aw hash-frontmatter .github/workflows/issue-triage.md)" = "$(head -n 1 .github/workflows/issue-triage.lock.yml | sed 's/^# gh-aw-metadata: //' | jq -r .frontmatter_hash)"` — passed. - `cmp <(git show HEAD:.github/workflows/issue-triage.lock.yml | tail -n +2) <(tail -n +2 .github/workflows/issue-triage.lock.yml)` — passed before committing; compiled lock excluding metadata is byte-identical to deployed main. - `actionlint -ignore 'unexpected key "queue" for "concurrency" section' .github/workflows/issue-triage.lock.yml` — passed with actionlint 1.7.12; only the existing narrow compiler-generated queue exception. - `git diff --check` — passed. No pipeline/Pester/snapshot/synthetic fixture tests or product builds were added or run. No production commands, dispatches or issue-label mutations were performed for this correction. Post-merge production outcome audit remains separate; local validation does not claim hosted end-to-end success. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Follow-up to dotnet#39065. No issue is closed: the reproduced defect is the activation failure in the linked actual production runs, not the underlying product reports used as smoke-test targets. <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
…dotnet#39188) <!-- 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! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Description of Change <!-- Enter description of the fix in this section --> Fix a confirmed startup-deadline regression in the CI-fix Azure DevOps dispatcher introduced by dotnet#39147, and correct a separate, pre-existing producer prompt contradiction that blocks mandatory safe-output registration. **Production evidence:** [run 37477952193](https://github.com/dotnet/maui/actions/runs/37477952193) spent approximately 533 seconds between starting the 420-second deadline and completing trusted checkout; [run 37485076087](https://github.com/dotnet/maui/actions/runs/37485076087) spent approximately 494 seconds. Both then failed with `The variable cannot be validated because the value 0 is not a valid value for the DispatcherBudgetSeconds variable.` **Root cause:** The script parameter `[ValidateRange(1, 540)][int]$DispatcherBudgetSeconds` and top-level `$script:DispatcherBudgetSeconds` are the same validated PowerShell variable. Clamping valid caller input `420` against an already-expired absolute deadline assigns `0` to that variable, triggering validation before explicit budget-exhaustion reporting can run. Extracted helper tests did not exercise this entrypoint initialization. **Correction:** - Store the clamped runtime allowance in the distinct `EffectiveDispatcherBudgetSeconds` variable and update remaining-budget helpers and their extracted-test state consistently. Caller budget `0` remains invalid. - Fail startup explicitly with `[dispatcher-budget-exhausted]` when preparation has consumed the deadline, before PR discovery, authentication, HTTP, or sleep. Write a failed preparation/deadline job summary and state `No queue POST was attempted.` This also fails rather than claiming successful no-work reconciliation when the deadline is expired. - Limit trusted checkout to the standalone `/.github/scripts/Queue-CiFixAzdoValidation.ps1` using non-cone sparse checkout. The existing SHA-pinned checkout v7.0.1 supports exact patterns and automatically uses `blob:none` for sparse fetches. Preserve `github.sha`, `fetch-depth: 1`, and `persist-credentials: false`; never fetch or execute PR code. - Document preparation behavior and limitations. Keep the absolute pre-checkout 420-second deadline and ten-minute hard job timeout unchanged; do not restart or extend either allowance. **Validation:** Baseline dispatcher suite: 73 passed. Before the script correction, the six new actual-entrypoint cases produced four expected expired-deadline failures with the original validation exception and two passing caller-validation/future-deadline cases. After correction, all **79 tests passed**, including existing helper/queue regressions. Expired cases assert the preparation summary, explicit prefix, no-POST message, and zero HTTP/sleep calls; a native PowerShell process asserts nonzero exit. The valid future deadline still produces the three verified offline pipeline payloads. Exact final commands and results: ```powershell pwsh -NoProfile -Command '$env:PSModulePath = "/Users/shneuvil/.copilot/session-state/237fbb01-6532-4170-9f5e-6af14fa3a5e0/files/powershell-modules" + [IO.Path]::PathSeparator + $env:PSModulePath; Import-Module Pester -ErrorAction Stop; Invoke-Pester -Path .github/scripts/Queue-CiFixAzdoValidation.Tests.ps1 -Output Normal -CI' # Passed: 79; failed/skipped: 0. Reused existing Pester; no dependency installation. pwsh -NoProfile -Command '$files = @(".github/scripts/Queue-CiFixAzdoValidation.ps1", ".github/scripts/Queue-CiFixAzdoValidation.Tests.ps1"); foreach ($file in $files) { $tokens = $null; $errors = $null; [void][System.Management.Automation.Language.Parser]::ParseFile((Join-Path $PWD $file), [ref]$tokens, [ref]$errors); if ($errors.Count) { $errors | ForEach-Object { Write-Error $_ }; exit 1 }; Write-Output "Parse passed: $file" }' # Passed: both PowerShell files parsed without errors. ``` ```bash actionlint .github/workflows/ci-fix-azdo-validation.yml # Passed. git diff --check # Passed. ``` **Limits:** The sparse-checkout assertion is a static configuration/trust regression, not a hosted checkout timing measurement. Production checkout performance remains unproven; checkout or PowerShell startup exceeding the ten-minute hard guard can still cancel the job before a summary is written. No live Azure queue requests, manual workflow dispatches/reruns, `/azp` requests, or credential/permission/model-policy changes were performed. There is still no successful eligible-PR Azure canary evidence here; this PR does not establish Azure permissions or exact-head check linkage for any of the three pipelines. **Related producer blocker (pre-existing, not introduced by dotnet#39147):** [main producer run 37464881821](https://github.com/dotnet/maui/actions/runs/37464881821) and [net11 producer run 37465231994](https://github.com/dotnet/maui/actions/runs/37465231994) concluded success but emitted `missing_tool` / `report_incomplete`. The net11 diagnostic explicitly says mandatory expectation registration requires PowerShell but the run instructions exclude it. The main diagnostic likewise reports the prompt-specific `no-pwsh` restriction, alongside separate unavailable cited Azure timeline evidence. This is an upstream producer blocker: a compliant PR cannot reach dispatcher validation if required registration is prohibited by its own prompt. Both producers already allow `pwsh` in `tools.bash`, and Hard Rule 11 requires `Register-CiFixSafeOutputExpectation.ps1` or atomic validation/registration via `Test-CiFixTransport.ps1`. Their bottom Environment constraints sections nevertheless said `no pwsh`. The mirrored correction changes only that prose: explicitly identify `pwsh` availability for those deterministic helpers, retain `no gh` / `no python` and `curl` + `jq` API guidance, and reiterate existing fail-closed registration/transport requirements. No tool allowlists, network, permissions, credentials, trust, model policy, or output gates change. Added two assertions in the existing registration test suite protecting both producer twins. Both failed on the old contradictory guidance and pass after correction. Regenerated both locks with retained **gh-aw v0.86.2**, without manual edits or compiler upgrade; **only `body_hash` changes**. Frontmatter and all generated lock runtime content remain identical. Additional exact validation commands and results: ```powershell pwsh -NoProfile -Command '$env:PSModulePath = "/Users/shneuvil/.copilot/session-state/237fbb01-6532-4170-9f5e-6af14fa3a5e0/files/powershell-modules" + [IO.Path]::PathSeparator + $env:PSModulePath; Import-Module Pester -ErrorAction Stop; $config = New-PesterConfiguration; $config.Run.Path = @(".github/scripts/Register-CiFixSafeOutputExpectation.Tests.ps1", ".github/scripts/Test-CiFixTransport.Tests.ps1", ".github/scripts/Queue-CiFixAzdoValidation.Tests.ps1"); $config.Run.Exit = $true; $config.Output.Verbosity = "Normal"; $config.TestResult.Enabled = $true; $config.TestResult.OutputPath = "/Users/shneuvil/.copilot/session-state/1e0cc90a-a521-4055-9e7a-9565cdaf5504/files/producer-and-dispatcher-pester-results.xml"; Invoke-Pester -Configuration $config' # Passed after lock regeneration: 116 tests (6 registration, 31 transport, 79 dispatcher); 0 failed/skipped. pwsh -NoProfile -Command '$file = Join-Path $PWD ".github/scripts/Register-CiFixSafeOutputExpectation.Tests.ps1"; $tokens = $null; $errors = $null; [void][System.Management.Automation.Language.Parser]::ParseFile($file, [ref]$tokens, [ref]$errors); if ($errors.Count) { $errors | ForEach-Object { Write-Error $_ }; exit 1 }; Write-Output "PowerShell parse passed"' # Passed. ``` ```bash /Users/shneuvil/.copilot/session-state/237fbb01-6532-4170-9f5e-6af14fa3a5e0/files/tools/gh-aw-0.86.2/darwin-arm64 compile ci-status-fix ci-status-fix-net11 --strict --no-emit --no-check-update # Passed: 2 workflows, 0 warnings. /Users/shneuvil/.copilot/session-state/237fbb01-6532-4170-9f5e-6af14fa3a5e0/files/tools/gh-aw-0.86.2/darwin-arm64 compile ci-status-fix ci-status-fix-net11 --strict --no-check-update # Passed: 2 workflows, 0 warnings; emitted body-hash-only changes. actionlint .github/workflows/ci-status-fix.lock.yml .github/workflows/ci-status-fix-net11.lock.yml # Nonzero both before and after regeneration: identical six baseline diagnostics. # Each twin: concurrency.queue not recognized, ShellCheck SC2129 style, github.aw expression not recognized. # No new lint findings; baseline/after outputs compared byte-identical. ``` **Producer limits:** These are static prompt/configuration assertions and offline helper tests, not a hosted agent behavior evaluation. Removing the contradiction does not guarantee that an agent will register correctly, find a viable product fix, create a PR, or attach Azure checks. Separate cited-timeline failures and other producer blockers are not fixed here. No live workflow or network validation tests were run. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Follow-up to dotnet#39147 for the production startup regression evidenced by the two linked dispatcher runs, plus the separate pre-existing producer helper-guidance contradiction evidenced by the two producer runs. No separate tracking issue is closed by this PR. <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. --> --------- Co-authored-by: PureWeen <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e0cc90a-a521-4055-9e7a-9565cdaf5504
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 39213Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 39213" |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey there @@philipag! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
Hey there @philipag! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The timeout recursively re-arms instead of completing requests for views that never lay out.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Fixes an Android ScrollToAsync UI-thread livelock caused by repeatedly polling layout state.
Changes:
- Defers only until the first layout.
- Uses a one-shot global-layout listener with a four-second timeout.
- Removes
IsLayoutRequestedfrom the serving condition.
| File | Description |
|---|---|
ScrollViewHandler.Android.cs |
Reworks deferred scroll scheduling and fallback behavior. |
This comment has been minimized.
This comment has been minimized.
|
/azp run |
github-actions
Bot
added
the
s/agent-review-in-progress
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
|
Note 🔍
|
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions and add test?
This comment has been minimized.
This comment has been minimized.
MauiBot
added
s/agent-review-incomplete
github-actions
Bot
added
the
s/agent-review-in-progress
|
Note 🔍
|
This comment has been minimized.
This comment has been minimized.
MauiBot
removed
the
s/agent-review-in-progress
github-actions
Bot
added
the
s/agent-review-in-progress
087efcf to
d0dbb69
Compare
d0dbb69 to
ad2951c
Compare
|
Thanks - both points from the review are addressed, and the two inline threads are resolved. The AI suggestions
Head is One thing a maintainer needs to do:
So nothing in this PR can move that check; re-running the shared workflow, or the CI-fix queue draining, clears it. @kubaflo - both asks in your review are addressed above (AI findings answered in their threads, tests added), so a re-review when convenient would settle the review state; I cannot request one myself (the reviewers endpoint answers 404 for an outside collaborator). |
There was a problem hiding this comment.
🟡 Changes recommended
The new predicate can lose scroll requests during legitimate pending layouts, and waiter cleanup is missing during handler disconnection.
3 open findings
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| // A view that has been laid out at least once has geometry worth scrolling against: the | ||
| // platform clamps the offsets to the last completed layout. IsLayoutRequested must not gate | ||
| // this decision - see WaitToScrollOnLayout for why waiting on it never ends. | ||
| if (!handler.PlatformView.IsLaidOut) |
| { | ||
| var platformView = handler.PlatformView; | ||
| var observer = platformView.ViewTreeObserver; | ||
| var waiter = new LayoutWaiter(platformView, handler, request); |
| var servedAt = DateTime.UtcNow; | ||
| var deadline = servedAt.AddSeconds(6); |
This comment has been minimized.
This comment has been minimized.
|
/azp run |
github-actions
Bot
added
the
s/agent-review-in-progress
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
|
Note 🔍
|
There was a problem hiding this comment.
🟡 Changes recommended
Detached views bypass the effective timeout, and stale waiters can target a reused handler.
4 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| observer.AddOnGlobalLayoutListener(waiter); | ||
| } | ||
|
|
||
| platformView.PostDelayed(waiter.OnTimeout, LayoutWaitTimeoutMillis); |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
| // A view that has been laid out at least once has geometry worth scrolling against: the | ||
| // platform clamps the offsets to the last completed layout. IsLayoutRequested must not gate | ||
| // this decision - see WaitToScrollOnLayout for why waiting on it never ends. | ||
| if (!handler.PlatformView.IsLaidOut) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Logic and Correctness — Removing IsLayoutRequested from the serve predicate can drop valid add-then-scroll requests: when IsLaidOut is true but a content-growth layout is pending, ServeScrollTo clamps against stale pre-layout extent and no replay occurs after the new layout, so the requested offset is under-shot (repro shape in #10283).
| observer.AddOnGlobalLayoutListener(waiter); | ||
| } | ||
|
|
||
| platformView.PostDelayed(waiter.OnTimeout, LayoutWaitTimeoutMillis); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Async and Threading Safety — platformView.PostDelayed(waiter.OnTimeout, ...) is not a reliable wall-clock timeout for never-attached/detached views; the runnable can remain queued until attach and never fire, leaving ScrollToAsync incomplete indefinitely on that lifecycle path.
| observer.RemoveOnGlobalLayoutListener(this); | ||
| } | ||
|
|
||
| if (_handler.IsConnected()) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Handler Mapper and Property Patterns — Deferred request ownership is not cleaned up on disconnect: Fire() drops the serve when !_handler.IsConnected() and there is no handler-level pending-request completion path, so a disconnect/detach before layout/timeout can orphan the corresponding ScrollToAsync task.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@philipag — new AI review results are available based on commit
ad2951c.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: b926f057
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
📱 ScrollViewHandlerTests (ScrollToRequestDeferredBeforeFirstLayoutIsServedByThatLayout, ScrollToRequestForNeverLaidOutViewTerminatesOnceWithoutRetrying) Category=ScrollView |
✅ FAIL — 514s | ✅ PASS — 390s |
🔴 Without fix — 📱 ScrollViewHandlerTests (ScrollToRequestDeferredBeforeFirstLayoutIsServedByThatLayout, ScrollToRequestForNeverLaidOutViewTerminatesOnceWithoutRetrying): FAIL ✅ · 514s
(no coded error found; showing last 1200 chars)
"totalMemoryBytes": 8322998272
},
"target": {
"kind": "emulator",
"name": "sdk_gphone_x86_64",
"identifier": "emulator-5554",
"operatingSystem": "Android",
"operatingSystemVersion": "11",
"apiLevel": 30,
"architecture": "x86_64",
"supportedArchitectures": [
"x86_64",
"x86",
"arm64-v8a",
"armeabi-v7a",
"armeabi"
],
"manufacturer": "Google",
"model": "sdk_gphone_x86_64",
"productName": "sdk_gphone_x86_64",
"buildFingerprint": "google/sdk_gphone_x86_64/generic_x86_64_arm64:11/RSR1.201211.001/7027799:user/release-keys",
"cpuModel": "0",
"cpuMaxFrequencyHertz": 2000,
"totalMemoryBytes": 2077544448
}
}
}
<<XHARNESS_RESULT_END>>
info: Attempting to remove apk 'com.microsoft.maui.core.devicetests'..
info: Successfully uninstalled com.microsoft.maui.core.devicetests
XHarness exit code: 1 (TESTS_FAILED)
Passed: 0
Failed: 2
Skipped: 0
Total: 2
Tests completed with exit code: 1
🟢 With fix — 📱 ScrollViewHandlerTests (ScrollToRequestDeferredBeforeFirstLayoutIsServedByThatLayout, ScrollToRequestForNeverLaidOutViewTerminatesOnceWithoutRetrying): PASS ✅ · 390s
(no coded error found; showing last 1200 chars)
795000,
"totalMemoryBytes": 8322998272
},
"target": {
"kind": "emulator",
"name": "sdk_gphone_x86_64",
"identifier": "emulator-5554",
"operatingSystem": "Android",
"operatingSystemVersion": "11",
"apiLevel": 30,
"architecture": "x86_64",
"supportedArchitectures": [
"x86_64",
"x86",
"arm64-v8a",
"armeabi-v7a",
"armeabi"
],
"manufacturer": "Google",
"model": "sdk_gphone_x86_64",
"productName": "sdk_gphone_x86_64",
"buildFingerprint": "google/sdk_gphone_x86_64/generic_x86_64_arm64:11/RSR1.201211.001/7027799:user/release-keys",
"cpuModel": "0",
"cpuMaxFrequencyHertz": 2000,
"totalMemoryBytes": 2077544448
}
}
}
<<XHARNESS_RESULT_END>>
info: Attempting to remove apk 'com.microsoft.maui.core.devicetests'..
info: Successfully uninstalled com.microsoft.maui.core.devicetests
XHarness exit code: 0
Passed: 2
Failed: 0
Skipped: 0
Total: 2
Tests completed successfully
📁 Fix files reverted (1 files)
src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs
📋 Pre-Flight — Context & Validation
Pre-Flight Context
PR snapshot
- PR: #39213 —
[Android] Fix ScrollToAsync UI-thread livelock caused by the MapRequestScrollTo retry loop - State: Open, non-draft; GitHub reports mergeability as blocked.
- Submitted revision:
ad2951c24a68be84ae7bf959afb3c06ec25aa23f - Base revision:
b926f05713b0d1843f8f3bef28b025c6242f1d3c(main) - Scope: 2 files, 325 additions, 6 deletions.
- The immutable diff supplied in the request matches the two files GitHub reports. This context does not assess later worktree state.
Reported problem and relevant existing behavior
The PR reports an Android-specific ScrollView.ScrollToAsync livelock. At the base revision, ScrollViewHandler.MapRequestScrollTo defers while either PlatformView.IsLaidOut is false or PlatformView.IsLayoutRequested is true. Deferral uses PlatformView.Post to invoke the same mapper again.
The reported failure mode is that Android can leave IsLayoutRequested true without scheduling another traversal when RequestLayout() occurs during layout. The reposted mapper then continually sees the same flag and reposts itself, allegedly causing an incomplete ScrollToAsync task, sustained UI-looper traffic, allocations, and CPU/battery use.
Surrounding source supports the completion dependency:
ScrollView.ScrollToAsynccreates aTaskCompletionSourceand returns its task.- The task completes when
IScrollView.ScrollFinished()reachesSendScrollFinished(). - Android’s
MauiScrollView.ScrollTocalls the completion callback immediately after an instant native scroll, or when its one-secondValueAnimatorends for an animated scroll. - The handler calls
VirtualView.ScrollFinished()only while connected. A request that is deferred indefinitely or dropped before that callback can therefore leave the public task incomplete.
The external reproduction archive, raw logs, Android framework internals, and performance measurements cited in the PR description were not independently inspected. Their contents and numerical claims remain unverified context.
Changed files and mechanism
-
src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs- Removes
IsLayoutRequestedfrom the initial defer predicate. A view that has completed any layout is now served immediately against its current geometry. - Extracts the existing conversion, platform scroll, and completion callback into
ServeScrollTo. - For a view not yet laid out, adds
WaitToScrollOnLayout:- Registers a
ViewTreeObserver.IOnGlobalLayoutListenerwhen the observer is alive. - Schedules a four-second fallback with
platformView.PostDelayed. - Calls
RequestLayout()only whenIsLayoutRequestedis false.
- Registers a
- Adds
LayoutWaiter, which usesInterlocked.Exchangeas a one-shot guard shared by the layout and timeout paths. - On firing, removes itself from the current live
ViewTreeObserverand callsServeScrollToonly if the handler remains connected. - The timeout path goes directly to
ServeScrollTo; it does not re-enter the layout predicate.
- Removes
-
src/Core/tests/DeviceTests/Handlers/ScrollView/ScrollViewHandlerTests.Android.cs- Adds a four-second test-side bound corresponding to the private production constant.
- Adds
ScrollToRequestDeferredBeforeFirstLayoutIsServedByThatLayout:- Starts with an attached but
GONE, never-laid-out view. - Makes it visible and induces a layout-requested latch from a global-layout callback.
- Expects the deferred request to be served once before the timeout.
- Expects a later request in
IsLaidOut && IsLayoutRequestedto be served immediately. - Waits beyond the fallback period to check that the original request is not served twice.
- Starts with an attached but
- Adds
ScrollToRequestForNeverLaidOutViewTerminatesOnceWithoutRetrying:- Uses an attached
GONEview that remains unlaid-out. - Expects no immediate completion, one completion around the fallback bound, and no second completion after another timeout period.
- Uses an attached
- Adds a counting
IScrollViewstub usingVolatile.ReadandInterlocked.Increment.
Platform-sensitive paths
The implementation and tests are Android-only. Important Android-specific behavior includes:
View.IsLaidOutandView.IsLayoutRequestedlifecycle semantics.ViewTreeObserver.OnGlobalLayout, which is tree-wide; the waiter filters events by checking whether its own view is laid out.View.PostDelayedbehavior while attached versus detached.RequestLayout()behavior during a traversal.- Native offset clamping against current child extent.
- Handler disconnection, platform-view detachment/reattachment, and possible handler reuse.
- Instant and animated scrolling have different completion paths in
MauiScrollView; the added tests invokeScrollToRequest(..., instant: true)only.
For comparison, the base iOS and Windows handlers explicitly store pending scroll requests as handler state and complete/remove them during DisconnectHandler. The submitted Android waiter is local to each request and is not owned or canceled by DisconnectHandler.
Related issue context
The PR links symptom-adjacent reports, but the available records do not establish that they share this precise root cause:
- #15387: open;
ScrollToAsynccalled from navigation/appearing does not return, labeled for Android and iOS. The issue body available through GitHub is limited. It references closed #16619. - #10283: open; adding children and scrolling in the same method fails. The report says Android completes without scrolling, while Windows remains incomplete until manual scrolling.
- #19835: open; nested Android
ScrollViewrequest is a silent no-op. - #14594: closed as a duplicate; Android scroll-to-element/coordinate no-op.
Current Controls source also has separate logic that parks element-target requests until element geometry exists. How that newer Controls-level behavior interacts with each historical issue is unknown.
Validation requirements and supplied evidence
- Required platform: Android.
- Provided Gate result: Gate ✅ PASSED — tests FAIL without fix, PASS with fix.
- The Gate was not re-run.
- The PR author reports Pixel 8 Pro/API 36 device-test and reproduction measurements, including red/green results and surrounding ScrollView suites. Those figures were not independently verified here.
- GitHub’s aggregate commit-status endpoint currently reports
pendingwith no legacy status entries. GitHub exposed a large check-run set, but it was not exhaustively inspected; overall CI health is therefore unknown, not absent.
Later Android validation should distinguish at least:
- First-layout event completion from timeout completion.
- Attached-but-never-laid-out and never-attached views.
- No repeated posting, duplicate serving, or retained listener after each outcome.
- Disconnect, detach/reattach, and handler/platform-view reuse while waiting.
- Content growth followed immediately by
ScrollToAsync, including final offset after the pending layout. - Instant and animated requests.
- Vertical, horizontal, and bidirectional orientations where relevant.
- Multiple or superseding requests and correct task completion ownership.
Remaining uncertainties
These are unresolved context items, not conclusions:
- An unresolved review thread argues that dropping
IsLayoutRequestedmay serve against stale geometry during a legitimate pending layout, allowing Android to clamp to the old extent and lose an add-then-scroll request such as #10283. - Another thread notes that
LayoutWaiter.Firedoes not complete the request when the handler has disconnected. Because the request has already left Controls’ pending path, the public task may remain incomplete; lifecycle ownership and cleanup need confirmation. - A further thread claims
View.PostDelayedon a detached or never-attached view may not reach the main looper until attachment, defeating the wall-clock timeout. The added timeout test covers an attachedGONEview, not a never-attached view. - The first test records its event-path timing after the request and layout trigger. An unresolved comment argues that elapsed setup time could allow the fallback to fire while the measured interval still appears shorter than four seconds.
- Removing the listener through
_view.ViewTreeObserverassumes the observer used for removal corresponds appropriately to the observer used for registration across detach/reattach. - When layout wins, the delayed callback remains scheduled until its four-second deadline; the one-shot guard prevents a second serve, but temporary object retention and callback cleanup were not characterized.
- The four-second timeout value and user-visible latency are policy choices with no cited shared constant.
- Serving a never-laid-out view completes the task but may apply no offset because the platform has zero scrollable geometry. The PR author explicitly reports that callers needing the offset after a later
GONE → VISIBLEtransition would need another request. - The PR currently has unresolved automated review threads and an earlier human changes-requested review asking that automated suggestions be checked and tests added. Tests were subsequently added, but no later human disposition was observed.
- This is context gathering only, not an exhaustive code review or an adjudication of the submitted approach.
🔬 Code Review — Deep Analysis
Expert PR Evaluation
Verdict: NEEDS_CHANGES
Confidence: low
Independent Assessment
The submitted Android change replaces recursive looper polling before first layout with a one-shot global-layout listener plus a four-second fallback, while serving requests immediately after any completed layout. The one-shot guard prevents the event and timeout paths from serving the same request twice, but the submitted implementation does not preserve request ownership across all layout and handler lifecycle states.
Blocking Findings
-
❌ Stale geometry can under-serve requests during a legitimate pending layout —
src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs:175Dropping
IsLayoutRequestedfrom the defer predicate means an already-laid-out view with pending content growth scrolls against its previous extent. Android can clamp the request before the new extent exists, and the submitted code has no post-layout replay. This leaves the add-content-then-scroll mechanism unresolved. -
❌ The fallback is not a wall-clock timeout for an unattached or detached view —
src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs:236View.PostDelayeduses the view callback queue when the view is not attached. The runnable may not reach the main looper until attachment, so a view that never attaches can still leaveScrollToAsyncincomplete indefinitely. -
❌ Disconnect can orphan a deferred request —
src/Core/src/Handlers/ScrollView/ScrollViewHandler.Android.cs:290LayoutWaiter.Firesuppresses serving afterIsConnected()becomes false, but the handler does not own, cancel, or complete the pending request during disconnect. A disconnect before layout or timeout therefore has no completion handoff for the public task.
Prior Review Reconciliation
The expert reviewer checked top-level reviews, inline review comments, and PR issue comments. Existing concerns corresponding to all three findings remain applicable to submitted revision ad2951c24a68be84ae7bf959afb3c06ec25aa23f.
Blast Radius and Failure Modes
- The mapper runs for every Android
ScrollToAsyncrequest; the change is platform-specific but affects normal, pre-layout, and lifecycle-transition paths. - No new process-wide or static state is introduced.
- Listener one-shot state prevents duplicate serving, but does not solve request ownership after disconnect.
- Multiple requests create independent waiters with no supersession or cancellation policy.
- Added tests exercise attached views and instant scrolling; they do not establish never-attached timeout behavior, disconnect completion, animated completion, or correct final offset after pending content growth.
Evidence Status
- Gate: ✅ PASSED — the supplied tests fail without the submitted fix and pass with it.
- Required-check state was not established as clean; the expert observed failing and in-progress check runs through anonymous submitted-SHA evidence after authenticated required-check retrieval was unavailable.
- Inline findings were persisted to
inline-findings.jsonbefore refinement consideration.
The three concrete mechanisms meet the evidence-first threshold for one consolidated pr-plus-reviewer refinement.
🛠️ Try-Fix — Analysis & Comparison
Alternative generation: not requested
Evidence-first mode intentionally omits routine Try-Fix attempts. This is not a failed
attempt, a completed alternative search, or evidence that alternatives have no value.
The expert review may still produce one evidence-backed PR refinement.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Trusted evidence-first context consumed |
| Code Review | NEEDS_CHANGES (low) | 3 blocking errors, 0 warnings |
| Gate | ✅ PASSED | Android; tests fail without the submitted fix and pass with it |
| Try-Fix | not requested | Evidence-first mode intentionally omitted routine alternatives |
| Refinement | ❌ FAILED | One implemented candidate; isolated validation was invalid and its focused build found a compile error |
| Report | ✅ COMPLETE |
Code Review and Candidate Comparison
| Rank | Candidate | Evidence | Blocking status |
|---|---|---|---|
| 1 | pr |
Gate passed for the submitted Android fix | Expert review found stale-geometry, unattached-timeout, and disconnect-completion defects |
| 2 | pr-plus-reviewer |
Implemented one consolidated refinement | Candidate-local SDK provisioning failed, the subsequent raw-worktree SDK fallback is inadmissible, and the focused candidate build failed with CS0234 at ScrollViewHandler.Android.cs:262 |
pr wins the bounded comparison because it is the only candidate with valid passing regression evidence. That does not make it merge-ready: the expert findings independently veto approval. The implemented refinement attempted to wait for legitimate pending layouts, schedule the timeout on the main looper, and make disconnect own completion, but a candidate that does not compile and was not validated in isolation must rank below the Gate-passing submitted fix.
Summary
The submitted fix removes the UI-thread repost loop and the supplied Gate demonstrates the targeted regression, but it leaves three blocking lifecycle/layout mechanisms unresolved. Routine Try-Fix was not requested. The sole evidence-backed refinement was implemented once and validated once as required; provisioning broke isolation and the build exposed a compile error, so it cannot replace or validate the PR.
Root Cause
The original Android mapper treats IsLayoutRequested as proof that another traversal will arrive and recursively reposts itself while that flag remains true. Android can leave the flag latched without scheduling a traversal, producing an unbounded looper retry and preventing ScrollToAsync completion.
Fix Quality
The raw PR replaces polling with a one-shot layout listener and fallback, which addresses the observed livelock and passed the supplied Gate. However, serving immediately whenever IsLaidOut is true can clamp against stale extent during legitimate pending content growth; View.PostDelayed does not provide a wall-clock bound before attachment; and a disconnect can remove the only path that completes a deferred request. These are concrete blocking findings, so the submitted fix requires further changes even though it remains the comparison winner.
📱 UI Tests — ViewBaseTests
Detected UI test categories: ViewBaseTests
✅ Deep UI tests — 118 passed, 0 failed, 1 skipped across 1 category on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
ViewBaseTests |
118/119 (1 skipped) ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
added
s/agent-changes-requested
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you check the ai's suggestions?
This comment has been minimized.
This comment has been minimized.
Tests Failure Analysis
🧪 CI Analysis — click to expand📊 maui-pr
🧪 maui-pr-devicetests
🧪 maui-pr-uitests
🧭 Follow-up — actions and refreshNext action: Complete UI/device evidence; inspect Install binlog and unresolved assertions.
|
The bug
On Android,
ScrollView.ScrollToAsynccan enter a state where the returnedTasknever completes and the handler spins the UI thread forever — thousands of looper messages per second, continuous JNI-bridge allocations, GC churn, battery drain — until the process is killed. The trigger is the handler's own wait condition!IsLaidOut || IsLayoutRequestedbecoming permanently true, which an ordinary app can cause without touching any MAUI internals.Cause
MapRequestScrollTodefers by re-posting itself to the same looper it is running on:This treats
IsLayoutRequested == trueas "a traversal is scheduled, so the condition will eventually clear". Android does not guarantee that:ViewRootImplsilently swallowsrequestLayout()while it is handling a layout-inside-layout request (mHandlingLayoutInLayoutRequest) — routine when a layout callback resizes or scrolls the view itself (parallax, keyboard-adaptive containers, auto-scroll lists). The flag then stays set with no traversal ever coming, so every armed retry re-posts forever: one permanent message-loop item plus oneRunnableImplementor/closure allocation perScrollToAsynccall.A flag-polled retry cannot detect or escape this state, because the state is defined by the flag no longer meaning what the loop assumes it means.
Repro
Self-contained MAUI app: ScrollBug-repro.zip (sources + raw logcat, no build outputs; build with
cd repro && dotnet build). It uses the stockScrollViewand no handler overrides; one test-page hook subscribes to the view'sViewTreeObserver.GlobalLayoutand callsRequestLayout()from inside the callback — precisely the swallowed-request pattern above. The app then verifies the latch (40 ms sampling ofIsLaidOut/IsLayoutRequested, reportsSTUCK pct=100 laidOutPct=100), fires 20 ×ScrollToAsync(0, y, false), and counts its own UI-thread message flow through aLooperprinter.Pixel 8 Pro emulator (API 36), stock
Microsoft.Maui.Controls 11.0.0-rc.1.26451.6:The same app with the scroll calls removed reports 1 msg/s — a healthy
ScrollViewnever produces this signature. Raw logcat for every run is in the zip.The fix
Keep the deferral for the case it exists for (before the first layout), but stop gating it on a flag that can lie forever:
ScrollToclamps against the last completed layout, so serving is never less correct than waiting — and in the latched state it is the only option that terminates.IsLayoutRequesteddrops out of the branch condition.ViewTreeObserver.OnGlobalLayoutListener, afterRequestLayout()arms the traversal when none is pending. Event-driven, so nothing polls or self-re-posts.Taskrather than parking it forever.The served path is untouched — same pixel conversion, same
ScrollFinished()semantics — so requests that work today behave identically.Results
Same app, same verified-frozen state, A/B by swapping only the
RequestScrollTomapping:It gets worse off-screen: vsync work disappears and the retry chains run at raw looper throughput. No platform mitigation applies — the loop holds no wakelock, alarm or job for Doze/App Standby to gate, and battery optimization is enabled for the repro package and changes nothing. That combination is why this shows up as battery drain on an idle screen with no console output and no wake-lock in the usage stats.
Related issues
Same wait branch, reported as hangs / silent no-ops:
Those reports show the hang / no-op face of this branch. When the view is in the latched state described above, each deferred request instead becomes a permanent item on the message loop — the CPU/GC/battery half of this bug. Those issues also discuss the completion callback parking inside
MauiScrollView, which this PR deliberately does not touch: it fixes the scheduling half, so the latched state terminates instead of livelocking.Review focus
IsLaidOutalone the right predicate?PostDelayed) acceptable, or would you prefer it sourced from an existing constant?Two device tests are now in this PR (
src/Core/tests/DeviceTests/Handlers/ScrollView/ScrollViewHandlerTests.Android.cs), covering both ways the wait can end:ScrollToRequestDeferredBeforeFirstLayoutIsServedByThatLayout- laid out later, served by that layout pass.ScrollToRequestForNeverLaidOutViewTerminatesOnceWithoutRetrying- GONE on a live window, served once by the bounded fallback, then nothing.Against
mainboth fail withActual: 0after their full observation window, i.e. the request is never served; against the handler in this PR both pass, and the surrounding suite is unchanged (Core 86/84 passed/2 skipped, Controls 13/13). Measured logs, the message census before and after, and the harness used are in the replies to the two review threads.Analysis, repro harness and measurements in this PR were produced with AI assistance (qwen3.8-flash-next); every number above was measured on the emulator and is reproducible from the attached zip.