Skip to content

Commit 7a94edf

Browse files
committed
build: carry an SDK patch for the OverlayPortal semantics crash
1 parent b6ea9e8 commit 7a94edf

5 files changed

Lines changed: 248 additions & 2 deletions

File tree

AGENTS.md

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,14 @@ it, so a toolchain bump leaves the old SDK on PATH until the session is
3030
replaced, and a run against the wrong SDK announces nothing: it builds, it runs,
3131
its tests pass.
3232

33-
Three things enforce it, none of which relies on anybody remembering:
33+
Four things enforce it, none of which relies on anybody remembering:
3434

3535
| Where | What it refuses |
3636
|---|---|
3737
| `require_mise` in `tool/dev/_lib.sh` | no mise, no `mise.toml`, or a flutter that resolves outside mise's own installs — the last one is the dangerous case, because `mise exec` will happily forward to a system SDK |
3838
| `tool/check/tooling.sh` | a bare toolchain command in the docs or CI; and, for every script in `tool/`, one that does not parse, is not executable, has no shebang, or reaches `flutter` / `dart` / `mise exec` without going through `pinned` |
39-
| `tool/run.sh` | runs both before it starts anything (0.34 s) |
39+
| `tool/internal/apply_sdk_patches.sh` | an SDK that is missing a patch this repo owns, or a patch that no longer applies to the pinned version — see [SDK patches](#sdk-patches) |
40+
| `tool/run.sh` | runs them before it starts anything (0.34 s) |
4041

4142
```sh
4243
tool/dev/analyze.sh
@@ -61,6 +62,29 @@ tool/dev/analyze.sh
6162
the CI gates, `release/` versioning and notes, `gen/` asset and code
6263
generators, `internal/` pieces other scripts call and nobody runs by hand.
6364

65+
### SDK patches
66+
67+
**The pinned Flutter SDK is not stock.** `tool/patches/*.patch` is applied to it
68+
by `require_mise`, so every script here — and every CI job, which reaches the
69+
toolchain the same way — runs a patched framework. Each patch names its upstream
70+
issue in its own header; read that before touching one.
71+
72+
This is the one place the repo deliberately does what the rest of this section
73+
exists to prevent, so it is held to the same standard. The patch lives in the
74+
repo rather than in an install, it is applied at the single gate every toolchain
75+
call already passes, and it says so the first time it changes anything. A patch
76+
applied on one laptop and forgotten on a runner would be exactly the
77+
works-here-fails-there the pin was bought to stop.
78+
79+
Two things follow, and both matter:
80+
81+
- **A patch that no longer applies is a hard stop**, not a skip. That is almost
82+
always a Flutter bump. Somebody has to decide whether upstream fixed the bug —
83+
delete the patch — or whether it has to be re-cut against the new source.
84+
- **Every patch carries a test that fails without it.** The test, not a check
85+
script, is what proves the patch is really in; it is also what stays behind
86+
once the patch is deleted, to say the upstream fix is real.
87+
6488
## Running
6589

6690
```sh
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
// The regression guard for `tool/patches/0001-semantics-attach-stale-children.patch`.
2+
//
3+
// Nothing in DPIP is under test here — this is the framework, reproduced at the
4+
// smallest shape that reaches the bug. It lives in the suite because the patch
5+
// has to be kept honest by something that runs on every machine and in CI, and
6+
// a test that fails on an unpatched SDK does that without a check script: the
7+
// patch is applied by `require_mise`, so a run that skipped it fails here.
8+
//
9+
// When upstream fixes flutter#189902 and mise.toml moves to a version that
10+
// carries the fix, the patch stops applying (and says so, loudly) — this test
11+
// should then pass on its own. Keep it. It is the thing that says the fix is
12+
// really in.
13+
//
14+
// Shape credit: the reduction in the upstream issue. Each of the three oddities
15+
// below is load-bearing; drop any one and the assert does not fire.
16+
import 'package:flutter/gestures.dart';
17+
import 'package:flutter/material.dart';
18+
import 'package:flutter_test/flutter_test.dart';
19+
20+
/// Two tooltips, one of which is rigged to reach the bug.
21+
///
22+
/// Every [Tooltip] is an [OverlayPortal]`RawTooltip` is built on
23+
/// `OverlayPortal.overlayChildLayoutBuilder` — which is why an app that never
24+
/// names `OverlayPortal` (DPIP does not) still hits an OverlayPortal defect.
25+
class _TwoTooltips extends StatelessWidget {
26+
const _TwoTooltips();
27+
28+
/// A border, so the two rows do not collapse into one render object and the
29+
/// semantics subtree has an interior node to lose track of.
30+
Widget _decorate(Widget child) => DecoratedBox(
31+
decoration: BoxDecoration(border: Border.all()),
32+
child: child,
33+
);
34+
35+
Widget _tooltip(String label) => Tooltip(
36+
message: '$label tooltip',
37+
// `explicitChildNodes` is what stops the child folding into the tooltip's
38+
// own node, so there is a separate SemanticsNode available to be stolen.
39+
child: Semantics(explicitChildNodes: true, child: Text('$label text')),
40+
);
41+
42+
@override
43+
Widget build(BuildContext context) {
44+
Widget bad = _tooltip('bad');
45+
// The second Overlay is what lets one node end up parented under a subtree
46+
// that a later pass rebuilds from the bottom up — the "stealing".
47+
bad = Overlay.wrap(child: ExcludeSemantics(child: bad));
48+
// A tight box: it stops the enclosing node being rebuilt on the pass that
49+
// would otherwise refresh the stale `_children` list and hide the bug.
50+
bad = SizedBox(width: 200, height: 100, child: bad);
51+
52+
return Dialog(
53+
child: Column(
54+
spacing: 20,
55+
children: [_decorate(_tooltip('good')), _decorate(bad)],
56+
),
57+
);
58+
}
59+
}
60+
61+
void main() {
62+
testWidgets('a tooltip shown and dismissed twice keeps the semantics tree '
63+
'consistent (flutter#189902)', (tester) async {
64+
// Semantics is only built when something asks for it — a screen reader on
65+
// a device, this handle in a test. Without it `flushSemantics` does no work
66+
// and the bug cannot be reached.
67+
final handle = tester.ensureSemantics();
68+
69+
await tester.pumpWidget(
70+
const MaterialApp(home: Scaffold(body: _TwoTooltips())),
71+
);
72+
73+
final gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
74+
await gesture.addPointer(location: Offset.zero);
75+
addTearDown(gesture.removePointer);
76+
77+
final target = tester.getCenter(find.text('bad text'));
78+
const away = Offset(5, 5);
79+
80+
// Twice is the whole test. The first show/dismiss leaves a SemanticsNode
81+
// with `attached == true` and `parent == null`; the second dismiss walks
82+
// into it and trips `assert(!child.attached)` in `_replaceChildren`.
83+
for (var pass = 1; pass <= 2; pass++) {
84+
await gesture.moveTo(target);
85+
await tester.pump();
86+
await tester.pump(const Duration(seconds: 1));
87+
await tester.pumpAndSettle();
88+
89+
await gesture.moveTo(away);
90+
await tester.pump();
91+
await tester.pump(const Duration(seconds: 1));
92+
await tester.pumpAndSettle();
93+
}
94+
95+
// Disposed in the body, not `addTearDown`: the binding's own end-of-test
96+
// check for leaked handles runs before tear-downs do.
97+
handle.dispose();
98+
});
99+
}

tool/dev/_lib.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,11 @@ EOF
8888
fi
8989

9090
export DPIP_MISE_CHECKED=1
91+
92+
# The pinned SDK carries repo-owned patches (tool/patches). Applied here, at
93+
# the one gate every toolchain call already passes through, so no script and
94+
# no machine can be the one that forgot. Silent unless it changes something.
95+
"$root/tool/internal/apply_sdk_patches.sh"
9196
}
9297

9398
# `flutter`/`dart`/anything else, on the pinned toolchain.

tool/internal/apply_sdk_patches.sh

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
#!/usr/bin/env bash
2+
# Applies tool/patches/*.patch to the pinned Flutter SDK.
3+
#
4+
# Called from `require_mise` in tool/dev/_lib.sh, so every script that reaches
5+
# the toolchain has already run it. Nobody runs this by hand.
6+
#
7+
# Patching the SDK is the thing this repo is otherwise most careful *not* to
8+
# do — AGENTS.md → Toolchain exists because a build off the wrong SDK announces
9+
# nothing. A patched SDK is a wrong SDK by that definition, so the same rule
10+
# applies to the patch: it lives in the repo, it is applied by a script every
11+
# entry point calls, and it is never left to a machine to have remembered. The
12+
# alternative is one laptop where the bug is fixed and a CI runner where it is
13+
# not, which is exactly the failure the pin was bought to prevent.
14+
#
15+
# Each patch is one of three states, and only one of them is silent:
16+
#
17+
# applies cleanly → apply it, and say so
18+
# already applied → nothing to do, say nothing
19+
# neither → stop. The SDK moved out from under the patch, which
20+
# usually means a Flutter bump. Somebody has to decide
21+
# whether upstream fixed it; a skipped patch would let
22+
# the bug back in without a word.
23+
set -euo pipefail
24+
25+
root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
26+
patch_dir="$root/tool/patches"
27+
28+
[[ -d $patch_dir ]] || exit 0
29+
shopt -s nullglob
30+
patches=("$patch_dir"/*.patch)
31+
((${#patches[@]})) || exit 0
32+
33+
# `mise where`, not the resolved binary's parent: this wants the SDK root that
34+
# `packages/flutter/...` hangs off, and require_mise has already established
35+
# that mise owns it.
36+
sdk="$(cd "$root" && mise where flutter 2>/dev/null || true)"
37+
if [[ -z $sdk || ! -d $sdk ]]; then
38+
printf '\n Cannot locate the pinned Flutter SDK to patch it.\n' >&2
39+
printf ' Run `mise install` from %s.\n\n' "$root" >&2
40+
exit 1
41+
fi
42+
43+
for p in "${patches[@]}"; do
44+
name="$(basename "$p")"
45+
46+
# --forward alone is not enough to tell "applies" from "already applied":
47+
# both refuse, with different messages. A reverse dry-run answers it
48+
# directly — a patch that can be undone is a patch that is already in.
49+
if patch -p1 -d "$sdk" --forward --dry-run --silent <"$p" >/dev/null 2>&1; then
50+
patch -p1 -d "$sdk" --forward --silent <"$p"
51+
printf ' \033[33m●\033[0m patched SDK: %s\n' "$name" >&2
52+
elif patch -p1 -d "$sdk" --reverse --dry-run --silent <"$p" >/dev/null 2>&1; then
53+
: # already applied
54+
else
55+
cat >&2 <<PATCHFAIL
56+
57+
tool/patches/$name no longer applies to the pinned SDK.
58+
59+
SDK: $sdk
60+
61+
The patch is pinned to one Flutter version and the pin has moved. Read the
62+
header of the patch — it names the upstream issue. If that issue is fixed in
63+
this version, delete the patch; its regression test stays and should pass on
64+
its own. If it is not fixed, re-cut the patch against the new source.
65+
66+
PATCHFAIL
67+
exit 1
68+
fi
69+
done
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
Guard SemanticsNode.attach() against a stale child list.
2+
3+
flutter#189902 https://github.com/flutter/flutter/issues/189902 (open)
4+
cut against flutter 3.47.0
5+
6+
`detach()` documents that `_children` is allowed to be stale — it may hold
7+
nodes that have since been re-parented — and skips those:
8+
9+
if (child.parent == this) { child.detach(); }
10+
11+
`attach()` walks the same list with no such guard, so it re-attaches a node
12+
that now belongs to somebody else. That produces a node with `attached == true`
13+
and `parent == null`, and the next `_replaceChildren` trips
14+
15+
'package:flutter/src/semantics/semantics.dart': line 3013
16+
Failed assertion: '!child.attached': is not true.
17+
18+
Every `Tooltip` is an `OverlayPortal` (`RawTooltip` is built on
19+
`OverlayPortal.overlayChildLayoutBuilder`), so showing and dismissing one
20+
twice is enough to reach it — which is what DPIP hit. Related, all open and
21+
all traced to `fccfa978a97` "Reland: Refactor OverlayPortal semantics", in
22+
stable since 3.41.0: flutter#187198, flutter#182604.
23+
24+
The assert is debug-only, but the inconsistent parent/child pointers it
25+
catches are real in release too, where they silently mis-shape the tree a
26+
screen reader walks.
27+
28+
`test/shared/semantics_overlay_portal_test.dart` fails without this patch.
29+
That test — not a check script — is what keeps the patch honest: when upstream
30+
lands a fix and mise.toml moves off 3.47.0, this patch stops applying and
31+
tool/internal/apply_sdk_patches.sh fails loudly. Delete it then, and the test
32+
should stay green on its own.
33+
34+
--- a/packages/flutter/lib/src/semantics/semantics.dart 2026-09-01 13:49:30
35+
+++ b/packages/flutter/lib/src/semantics/semantics.dart 2026-09-01 13:49:30
36+
@@ -3263,7 +3263,12 @@
37+
}
38+
if (_children != null) {
39+
for (final SemanticsNode child in _children!) {
40+
- child.attach(owner);
41+
+ // The list of children may be stale and may contain nodes that have
42+
+ // been assigned to a different parent. Mirrors the guard `detach()`
43+
+ // already has. See https://github.com/flutter/flutter/issues/189902
44+
+ if (child.parent == this) {
45+
+ child.attach(owner);
46+
+ }
47+
}
48+
}
49+
}

0 commit comments

Comments
 (0)