Rebuild a semantics fragment when its sibling conflict changes - #190431

Declaring entry as late final and registering addTearDown before it is initialized can lead to a LateInitializationError if pumpWidget or any preceding step fails. This masks the actual test failure and makes debugging harder.

Instead, define the widgets first, initialize entry directly, register its teardown, and then pass it to pumpWidget.

      // A second anchor is required: the node that goes stale belongs to the
      // sibling, not to the portal being toggled.
      final Widget sibling = OverlayPortal.overlayChildLayoutBuilder(
        controller: siblingController,
        overlayChildBuilder: (BuildContext context, OverlayChildLayoutInfo info) =>
            const SizedBox.shrink(),
        child: Semantics(explicitChildNodes: true, child: const Text('sibling')),
      );

      Widget portal = OverlayPortal.overlayChildLayoutBuilder(
        controller: controller,
        overlayChildBuilder: (BuildContext context, OverlayChildLayoutInfo info) =>
            const Align(alignment: Alignment.topLeft, child: Text('overlay child')),
        child: Semantics(explicitChildNodes: true, child: const Text('anchor')),
      );
      portal = Overlay.wrap(child: ExcludeSemantics(child: portal));
      portal = SizedBox(width: 200, height: 100, child: portal);

      final OverlayEntry entry = OverlayEntry(
        builder: (BuildContext context) =>
            Semantics(container: true, child: Column(children: <Widget>[sibling, portal])),
      );
      addTearDown(() {
        entry.remove();
        entry.dispose();
      });

      await tester.pumpWidget(
        Directionality(
          textDirection: TextDirection.ltr,
          child: Overlay(
            initialEntries: <OverlayEntry>[entry],
          ),
        ),
      );

To prevent test pollution and properly dispose of SemanticsTester even if an assertion fails, register its disposal using addTearDown immediately after creation.

This manual call to semantics.dispose() can be removed since it is now handled automatically by addTearDown registered at the start of the test.

This doesn't feel right, in this case the oldParent should updateWith a new list of children that exclude the child. do you know why this is not the case?

Good question, and the short answer is that it does exactly that in the common case, just not in the one that breaks.

I instrumented the repro to log every updateWith and every steal. Trimmed to the interesting part, with the portal shown, hidden, then shown again:

UPDATEWITH: node#3 children=[2]
STEAL:      node#2 taken from node#1 by node#3   (node#1.attached=true,  node#1._children=[2])
UPDATEWITH: node#1 children=[3, 4]          <- node#1 rebuilds, list no longer has node#2

UPDATEWITH: node#1 children=[2]
STEAL:      node#2 taken from node#3 by node#1   (node#3.attached=false, node#3._children=[2])
SHORTCIRCUIT: RenderSemanticsAnnotations built=true, cached=node#3, its _children=[2] -> no updateWith
UPDATEWITH: node#1 children=[3, 6]          <- node#3 goes back in still holding [2]

The first steal is the case you describe. node#1 loses node#2 and is rebuilt in the same pass, so it calls updateWith with a list that excludes it and everything stays consistent.

The second steal is the problem. The victim is node#3, which by then is detached and cached. Its _RenderObjectSemantics still has built == true, so when the portal is shown again _buildSemantics short circuits at object.dart:6225 and never reaches _produceSemanticsNode, _buildSemanticsSubtree or updateWith. node#3 is handed straight back to node#1 as a child while still holding _children = [2], and attaching it attaches node#2, which now belongs to node#1.

Nothing marks that _RenderObjectSemantics as needing a build when its semantics child is taken. markNeedsBuild is only driven by config, geometry and parentData, and there is no back reference from a SemanticsNode to the _RenderObjectSemantics that cached it.

I did try adding that back reference and calling markNeedsBuild at the moment of the steal, which I think is what you are pointing at. It does not work, because the steal always happens inside flushSemantics and the framework refuses to be dirtied from there in three places:

• markNeedsBuild() leaves the flush with a dirty node, so debugCheckForBuilds trips assert(node.built)
• RenderObject.markNeedsSemanticsUpdate() asserts !attached || !owner!._debugDoingSemantics
• PipelineOwner.flushSemantics asserts _nodesNeedingSemanticsUpdate.isEmpty on the way out

So the repair has to be synchronous, which is why this fixes the list rather than scheduling a rebuild.

If you would rather see the cached fragment rebuild instead, I am happy to try, but it looks like that means relaxing one of those three invariants or invalidating before the flush starts, perhaps in the OverlayPortal teardown path. Happy to go whichever way you prefer.

Nothing marks that _RenderObjectSemantics as needing a build when its semantics child is taken. markNeedsBuild is only driven by config, geometry and parentData

I think this is the problem, the victim renderobject (node3) must have knew its children got taken away (because it is not painting it), and it should have markNeedsSemanticsUpdate when that happen

You were right, and I was wrong about it not being possible. I have rewritten the PR to do what you asked.

shouldFormSemanticsNode reads two things:

return parentData!.explicitChildNodes || _hasSiblingConflict;

The parent data half is invalidated. _didUpdateParentData calls markNeedsBuild with the comment "Parent data changes may result in node formation changes". The sibling conflict half is not. markSiblingConfigurationConflict assigns the flag and returns, so a fragment can stop forming a node, keep built == true and keep its cachedSemanticsNode, and never find out that its children were taken while it was out of the tree.

So the render object does know, exactly as you said. It just does not act on it.

The reason my earlier answer said this was impossible is that I was trying to invalidate at the moment of the theft, which is in the middle of the build phase, where all three of those asserts fire. markSiblingConfigurationConflict is reached from updateChildren during the parent data phase, before the build loop, so marking there gets rebuilt in the same flush and none of them fire. My mistake.

One wrinkle worth mentioning, because it cost me a round. The obvious version, an early return in markSiblingConfigurationConflict when the value is unchanged, does not work. _marksConflictsInMergeGroup resets every fragment to false before recomputing, and that reset is load bearing: configToMergeUp returns null when shouldFormSemanticsNode is true, so leaving the previous pass's flag in place while recomputing makes fragments get skipped and everything merges up. It cost me six semantics tests before I understood why. The version in the PR snapshots the flags before the pass and compares after, leaving the existing sequence untouched.

The two changes from the previous revision are gone. Repairing the stale child list and guarding attach both worked, but they were treating the symptom, and this is the cause. I am happy to add the attach guard back as hardening if you want it, since six of the eight places that walk _children still assume the list is clean, but that is a separate concern from this bug and I would rather not muddy the PR with it.

test/semantics, test/rendering, test/widgets and test/material all pass with this. The regression test is unchanged.

do we need a separate loop for this? can we record existing Conflicting in the loop on line 6549

Done. The snapshot is taken in the existing loop now, right before the reset. Nothing in that loop touches another fragment's _hasSiblingConflict, the markSiblingConfigurationConflict(true) calls all happen after it, so reading it there gives the same value the separate pass gave.

I also folded the comparison into the loop that marks the conflicts rather than walking mergeGroup a third time. Anything in hasSiblingConflict that was not already in the snapshot is newly conflicting, and whatever is left in the snapshot after removing those stopped conflicting. Both of those need the mark, since losing a conflict changes shouldFormSemanticsNode the same way gaining one does.

this is not used in the test, and should be removed

Removed. It was doing even less than it looked like, testWidgets has semanticsEnabled on by default and SemanticsTester only wraps tester.ensureSemantics(), so it was not enabling anything either.

Worth saying out loud that this leaves the test with no semantics expectations of its own, so the only thing that can fail it is the assertion in _replaceChildren. Happy to add some if you would rather it pinned the resulting tree down instead of relying on that.

remove shouldn't be here, it will be inserted and removed by the overlay widget when it init and dispose. It is not the responsibility of owner's of the overlayentry to call remove

Removed, and I took entry.dispose() and the late final with it rather than just the remove().

OverlayState does not override dispose, and nothing sets entry._overlay back to null except OverlayEntry.remove(), so leaving the dispose() behind on its own trips 'An OverlayEntry must first be removed from the Overlay before dispose is called.' at overlay.dart:278. Dropping the whole teardown is the clean version, the entry goes away with the Overlay.

Correction to what I said here. The entry does not go away with the Overlay, nothing ever disposes it, and the leak tracking shard caught it. Windows framework_tests_widgets_leak_tracking is red on this PR because of that. _WrappingOverlayState itself calls remove() and then dispose(), so whoever creates the entry really does have to do both.

I switched the test to Overlay.wrap, which owns its entry, so there is nothing left to clean up and still no remove() in the test. It passes with LEAK_TRACKING=true and still fails without the fix.