Repository navigation
[web] Keep the semantics tree usable when an update lists a child that was never sent - #195837
hannah-hyj wants to merge 1 commit into
Conversation
…t was never sent Defense in depth for flutter#193235. When a SemanticsUpdate contains a parent whose child list references an id that has never been sent to the engine, `SemanticsObject.updateChildren()` dereferenced `owner._semanticsTree[childId]!` and threw `Unexpected null value`. Because `_finalizeTree()` was only called on the success path, the owner stayed in `SemanticsUpdatePhase.updating` with the pending attachment and detachment lists populated, so every subsequent update failed as well and the tree stayed broken for the rest of the session. Disabling and re-enabling semantics did not help either: `reset()` walked the (now inconsistent) child lists through `_finalizeTree()` and threw from there. This change makes the engine tolerate such an update instead of poisoning itself: * `SemanticsObject._lookUpChild` replaces the bare `!` look-ups in `updateChildren` (render order and hit-test order) and `recomputeChildrenAdjustment`. In debug mode it asserts with a message that names the parent and the missing child; in release mode the missing child is skipped and the rest of the update is applied. * The two depth-first visitors already asserted on a missing child; they now also skip it so that the detachment walk in `_finalizeTree()` cannot throw on a stale child list. * `EngineSemanticsOwner.updateSemantics` runs `_finalizeTree()` in a `finally` block, and clears each node's dirty fields in a `finally` block, so the owner always returns to the idle phase with empty attachment and detachment lists even if the update throws. * `EngineSemanticsOwner.reset()` no longer detaches every node and walks their child lists; it disposes each object directly, runs the one-time post-update callbacks, and clears all owner state in a `finally` block so that disabling semantics is always a clean slate. The framework-side trigger observed in production (a parent that stops merging its descendants without sending them) is removed separately in flutter#195829. This change makes sure that any remaining or future producer of such an update degrades to a missing node in release builds and a descriptive assertion in debug builds, rather than a permanently broken accessibility tree. Tests: three new tests in `semantics_test.dart` under "update that lists a child that was never sent". On master, all three fail, and the failure also cascades into unrelated later tests in the same file ("semantics owner update phases" reports the owner stuck in `updating`; "SemanticRole Sets id..." reports "Semantics node map was inconsistent"), which is exactly the poisoning this change prevents.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request robustifies the web engine's semantics update and reset processes to prevent the engine from getting stuck in an inconsistent state when the framework sends incomplete updates (e.g., listing a child that was never sent). It introduces safe child lookup with debug assertions, wraps the update phase in try-finally blocks to guarantee tree finalization, and refactors reset() to safely dispose of objects and clear state. Feedback on the changes highlights a potential risk in executing _oneTimePostUpdateCallbacks during reset() after disposing of the semantics objects, and suggests copying _semanticsTree.values to a list before iterating to avoid a ConcurrentModificationError during disposal.
| for (final SemanticsObject object in _semanticsTree.values) { | ||
| object.dispose(); | ||
| } | ||
| _phase = SemanticsUpdatePhase.postUpdate; | ||
| for (final ui.VoidCallback callback in _oneTimePostUpdateCallbacks) { | ||
| callback(); | ||
| } |
There was a problem hiding this comment.
Executing _oneTimePostUpdateCallbacks during reset() is risky and likely incorrect. These callbacks (such as requesting focus or sending accessibility announcements) are registered during an update and expect a valid, active semantics tree. Since all SemanticsObjects have just been disposed, running these callbacks can lead to exceptions (e.g., trying to access or operate on disposed elements) or unwanted side effects (e.g., announcing messages when semantics is being disabled). In the original implementation, these callbacks were simply cleared without being executed.\n\nAdditionally, iterating directly over _semanticsTree.values while calling dispose() carries a risk of ConcurrentModificationError if any disposal logic or side effects modify the map. Copying the values to a list using .toList() before iterating is a safer, defensive programming practice.
for (final SemanticsObject object in _semanticsTree.values.toList()) {\n object.dispose();\n }
flutter-zl
left a comment
flutter-zl
left a comment
There was a problem hiding this comment.
Thanks! The finalize-in-finally and the reset() rewrite look good.
I built release demos with this PR's semantics.dart patched into the web SDK. They use raw updateSemantics calls, like the repro on #193235:
- Before: https://flutter-demo-93-before.web.app
- After: https://flutter-demo-93-after.web.app
An empty root listing a missing child, step 1, is fixed. A root that already has children and adds a missing id, step 2, still throws Null check operator used on a null value in release.
After _lookUpChild skips the missing id, childrenInRenderOrder.length < childCount, but updateChildren still iterates to childCount in while (newIndex < childCount) and for (int i = childCount - 1; ...). Using childrenInRenderOrder.length for minLength, the trivial-case check, and both loops should fix it.
The new tests don't reach this because the debug assert in _lookUpChild fires first. Collecting missing ids and asserting after _finalizeTree() would let the tests cover the release path, and it would also stop a debug throw from leaving later nodes in the same update with dirty fields.
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
fix #193235.
When a SemanticsUpdate contains a parent whose child list references an id that has never been sent to the engine,
SemanticsObject.updateChildren()dereferencedowner._semanticsTree[childId]!and threwUnexpected null value. Because_finalizeTree()was only called on the success path, the owner stayed inSemanticsUpdatePhase.updatingwith the pending attachment and detachment lists populated, so every subsequent update failed as well and the tree stayed broken for the rest of the session. Disabling and re-enabling semantics did not help either:reset()walked the (now inconsistent) child lists through_finalizeTree()and threw from there.This change makes the engine tolerate such an update instead of poisoning itself:
SemanticsObject._lookUpChildreplaces the bare!look-ups inupdateChildren(render order and hit-test order) andrecomputeChildrenAdjustment. In debug mode it asserts with a message that names the parent and the missing child; in release mode the missing child is skipped and the rest of the update is applied._finalizeTree()cannot throw on a stale child list.EngineSemanticsOwner.updateSemanticsruns_finalizeTree()in afinallyblock, and clears each node's dirty fields in afinallyblock, so the owner always returns to the idle phase with empty attachment and detachment lists even if the update throws.EngineSemanticsOwner.reset()no longer detaches every node and walks their child lists; it disposes each object directly, runs the one-time post-update callbacks, and clears all owner state in afinallyblock so that disabling semantics is always a clean slate.The framework-side trigger observed in production (a parent that stops merging its descendants without sending them) is removed separately in #195829. This change makes sure that any remaining or future producer of such an update degrades to a missing node in release builds and a descriptive assertion in debug builds, rather than a permanently broken accessibility tree.
Tests: three new tests in
semantics_test.dartunder "update that lists a child that was never sent". On master, all three fail, and the failure also cascades into unrelated later tests in the same file ("semantics owner update phases" reports the owner stuck inupdating; "SemanticRole Sets id..." reports "Semantics node map was inconsistent"), which is exactly the poisoning this change prevents.Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.