Skip to content

Fix null-check crash in semantics geometry-dirty enqueue path - #189942

Open
mbcorona wants to merge 3 commits into
flutter:masterfrom
mbcorona:fix-semantics-geometry-null-owner-189908
Open

Fix null-check crash in semantics geometry-dirty enqueue path#189942
mbcorona wants to merge 3 commits into
flutter:masterfrom
mbcorona:fix-semantics-geometry-null-owner-189908

Conversation

@mbcorona

Copy link
Copy Markdown
Member

Description

Fixes a fatal _TypeError ("Null check operator used on a null value") thrown during PipelineOwner.flushSemantics.

In _RenderObjectSemantics (packages/flutter/lib/src/rendering/object.dart), two spots enqueue a render object for a geometry update via owner!._nodesNeedingSemanticsGeometryUpdate.add(...). When a contributing render object is momentarily detached (a parent flips include -> exclude -> include across frames while the object's semantics geometry is still dirty), owner is null and the owner! deref throws.

This mirrors the already-fixed sibling crash #184036 (fixed by #184226), which guarded the _buildSemanticsSubtree site but left these two enqueue sites unguarded.

Changes

  • updateChildren: guard the enqueue with owner?. instead of owner!.
  • markNeedsUpdate: same guard.

A detached node has no owner and is not in the pipeline, so skipping the enqueue is a correct no-op; it is re-scheduled when the node re-attaches. scheduleInitialSemantics is intentionally left unchanged, since it asserts the object is attached and its owner is non-null by contract.

Added a regression test in packages/flutter/test/rendering/semantics_layout_sized_test.dart that drives the include -> exclude -> include geometry-dirty flip and asserts the flush completes without throwing. The exact detached-owner race cannot be staged deterministically in a single-threaded test (the framework's attach/detach invariants prevent setting it up directly), so the test exercises the same reachable enqueue path; a note in the test explains this.

Fixes#189908

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboardflutter-dashboardBot added the CICD Run CI/CD label Jul 23, 2026
@github-actionsgithub-actionsBot added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Jul 23, 2026

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces force-unwraps of the render object's owner with null-safe navigation when enqueuing semantics geometry updates, preventing potential null pointer crashes. A test is also added to verify this behavior. The feedback suggests removing the expect(..., returnsNormally) wrapper from the new test to avoid a common testing anti-pattern and ensure cleaner stack traces upon failure.

Comment on lines +145 to +163
expect(() {
layout(parent, phase: EnginePhase.flushSemantics);

// Exclude `child` from the semantics tree while its geometry is dirty,
// leaving its geometry marked dirty (blocked).
middle1.markNeedsLayout();
middle2.markNeedsLayout();
child.markNeedsLayout();
final child2 = RenderBlockSemanticsBoundary();
parent.add(child2);
pumpFrame(phase: EnginePhase.flushSemantics);

// Re-include `child` while its geometry is still dirty. This drives the
// geometry-update enqueue in `updateChildren`.
middle1.markNeedsLayout();
child.markNeedsLayout();
parent.remove(child2);
pumpFrame(phase: EnginePhase.flushSemantics);
}, returnsNormally);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Flutter/Dart tests, wrapping the entire test body in expect(() { ... }, returnsNormally) is generally considered an anti-pattern. If any exception is thrown, the test will automatically fail, and doing so without the expect wrapper provides a cleaner, more direct stack trace. Consider removing the expect and returnsNormally wrapper.

layout(parent, phase:EnginePhase.flushSemantics);
// Exclude `child` from the semantics tree while its geometry is dirty,// leaving its geometry marked dirty (blocked).
middle1.markNeedsLayout();
middle2.markNeedsLayout();
child.markNeedsLayout();
final child2 =RenderBlockSemanticsBoundary();
parent.add(child2);
pumpFrame(phase:EnginePhase.flushSemantics);
// Re-include `child` while its geometry is still dirty. This drives the// geometry-update enqueue in `updateChildren`.
middle1.markNeedsLayout();
child.markNeedsLayout();
parent.remove(child2);
pumpFrame(phase:EnginePhase.flushSemantics);

@justinmc
justinmc self-requested a review July 28, 2026 22:31
@hannah-hyj
hannah-hyj self-requested a review July 29, 2026 21:47
@mbcoronambcorona self-assigned this Jul 31, 2026
@chunhtai
chunhtai self-requested a review August 19, 2026 22:01

@chunhtaichunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, once the gemini comment is addressed

@mbcorona

Copy link
Copy Markdown
MemberAuthor

Thanks for the review @chunhtai , Gemini's suggestion has been addressed.

@chunhtaichunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@chunhtaichunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2026

@justinmcjustinmc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change looks good but see my comment on the test.

Comment on lines 126 to +127

test('does not crash enqueuing geometry update when child owner is null', () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test passes on master, could you make it fail without your change?

@chunhtaichunhtai removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2026
@mbcorona

Copy link
Copy Markdown
MemberAuthor

HI @justinmc

You're right, as written it passes on master so it isn't proving anythingthe, the tricky part is that updateChildren only runs for nodes the flush loop picks up, and that loop already skips anything where owner != this. So the node being flushed is always attached. And since attach and detach walk the whole subtree together, any real child of an attached node is attached too. That means a normal child never has a null owner while its parent is being flushed. The null owner only seems to happen as a brief cross frame race in production, with an accessibility service running.
To hit the crash I need a node with a null owner sitting under an attached node that's being flushed, but the geometry step right after needs that same node to have a real ancestry path back up, which only happens if it's attached. Can't have both at once in a settled tree.

At this point it's basically a defensive null check, same shape as the if (renderObject.owner != null) guard we already have for the build path lower in this file. I'm inclined to drop the test and mark this one test exempt, but I didn't want to just do that without checking. Would that work for you, or is there a repro angle you'd want me to try?

@chunhtai

Copy link
Copy Markdown
Contributor

This shouldn't be test exempted. If we can't figure out the exact cause, we shouldn't merge this.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: accessibilityAccessibility, e.g. VoiceOver or TalkBack. (aka a11y)CICDRun CI/CDframeworkflutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[a11y] Null-check crash (_TypeError) in _RenderObjectSemantics.updateChildren — unguarded owner! at rendering/object.dart:5875

4 participants

@mbcorona@chunhtai@justinmc@VivaLaRoy