Fix owner-mismatch crash for OverlayPortal inside a deferred-mount parent (e.g. Table)#185793
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses a crash occurring when an OverlayPortal is used within a Table by adding a check to ensure the layout surrogate is attached before calling redepthChild in _RenderDeferredLayoutBox. A regression test has been added to verify the fix. The review feedback suggests enhancing this test by asserting the layout size of the overlay child to ensure the layout phase completes successfully.
141c2b0 to
600e26a
Compare
0e60bff to
62f231b
Compare
…rent `_RenderDeferredLayoutBox.redepthChildren` unconditionally called `_layoutSurrogate.redepthChild(this)`. When a parent like `Table` defers adopting its render-object children until every row has been mounted, the deferred-layout box is adopted by the theater (and gains a pipeline `owner`) while the layout surrogate is still un-attached, so the `child.owner == owner` assertion fires. The opposite direction in `_RenderLayoutSurrogateProxyBox.redepthChildren` already guards on `child.attached`. Mirror that guard here. The depth invariant is restored when the surrogate is finally attached, because the surrogate's own `redepthChildren` re-depths this box. Adds a regression test placing an `OverlayPortal` (with `controller.show()` called before mount, mirroring `Slider`) inside a `TableRow` and verifying neither the assertion nor an exception fires and the overlay child is present. Fixes flutter#174133 Fixes flutter#180337
62f231b to
2f52e17
Compare
justinmc
left a comment
There was a problem hiding this comment.
Thanks for the PR! LGTM but @LongCatIsLooong should take a look.
Someone wrote a test for this before, do you think it's useful in addition to your test, or leave it out? https://github.com/flutter/flutter/pull/180342/changes
|
Thanks! I looked at #180342 again. I think the existing test here is the better one to keep because it targets the root OverlayPortal/TableRow owner-ordering issue directly, while still mirroring the Slider path by calling controller.show() before mount. It also now asserts that the overlay child completes layout, so it covers the part that the older Slider regression was trying to protect. The Slider test is useful as a user-facing reproduction, but adding both feels mostly duplicative and would pull in Material/Slider coverage for a framework-level OverlayPortal fix. So my preference is to leave it out unless @LongCatIsLooong would rather have that higher-level coverage too. |
LongCatIsLooong
left a comment
There was a problem hiding this comment.
That's an elegant fix , thank you!
|
auto label is removed for flutter/flutter/185793, Failed to enqueue flutter/flutter/185793 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected.. |
…rent (e.g. Table) (flutter#185793) ## Issue Fixes flutter#174133 (and the duplicate flutter#180337). Placing a widget that hosts an `OverlayPortal` — most visibly `Slider`, but anything that calls `OverlayPortalController.show()` before its `OverlayPortal` is mounted — inside a `Table`/`TableRow` crashes during mount with: ``` 'package:flutter/src/rendering/object.dart': Failed assertion: line 2138 pos 12: 'child.owner == owner': is not true. flutter#2 RenderObject.redepthChild flutter#3 _RenderDeferredLayoutBox.redepthChildren overlay.dart:2576 flutter#4 RenderObject.redepthChild flutter#5 RenderObject.adoptChild flutter#6 _RenderTheater._addDeferredChild overlay.dart:1312 flutter#7 _OverlayEntryLocation._addChild overlay.dart:2185 flutter#8 _OverlayPortalElement.insertRenderObjectChild overlay.dart:2447 ... flutter#24 _TableElement.mount.<anonymous closure>... table.dart:303 ``` This is a regression introduced between Flutter 3.32 and 3.35. ## Root cause `Table` defers calling `adoptChild` on its `RenderObject` children until *every* row has been mounted (`_TableElement.insertRenderObjectChild` is a no-op while `_doingMountOrUpdate`; the children are wired in later by `_updateRenderObjectChildren`). Meanwhile `_OverlayPortalElement.mount` runs `super.mount` (which "attaches" the layout surrogate to its parent — but Table swallows it) and then immediately mounts the overlay child, which causes the deferred-layout box to be adopted by the `_RenderTheater`. The theater is attached, so the deferred box receives an owner — *while the layout surrogate has none yet*. When the theater calls `redepthChild(deferredBox)`, the deferred box's `redepthChildren` unconditionally calls `_layoutSurrogate.redepthChild(this)`, which trips the `child.owner == owner` assertion across that owner boundary. ## Fix `_RenderDeferredLayoutBox.redepthChildren` now skips the cross-redepth when the surrogate is not yet attached, mirroring the existing guard in `_RenderLayoutSurrogateProxyBox.redepthChildren`: ```dart if (_layoutSurrogate.attached) { _layoutSurrogate.redepthChild(this); } ``` The depth invariant is restored when the surrogate is finally adopted by its parent: `_RenderLayoutSurrogateProxyBox.redepthChildren` already calls `redepthChild(_deferredLayoutChild)` once that child becomes attached. ## Tests Adds a regression test in `test/widgets/table_test.dart` placing an `OverlayPortal` whose controller is `.show()`'d before mount (mirroring `Slider`) inside a `TableRow`, asserting that no exception is thrown and the overlay child is present. Without this fix the new test reproduces the assertion. With this fix `flutter test test/widgets/table_test.dart`, `test/widgets/overlay_portal_test.dart`, `test/widgets/overlay_test.dart`, `test/widgets/raw_tooltip_test.dart`, `test/widgets/overlay_layout_builder_test.dart`, and `test/material/slider_test.dart` all pass. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making. - [x] All existing and new tests are passing. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo --------- Co-authored-by: Victor Sanni <[email protected]>
Issue
Fixes #174133 (and the duplicate #180337). Placing a widget that hosts an
OverlayPortal— most visiblySlider, but anything that callsOverlayPortalController.show()before itsOverlayPortalis mounted — inside aTable/TableRowcrashes during mount with:This is a regression introduced between Flutter 3.32 and 3.35.
Root cause
Tabledefers callingadoptChildon itsRenderObjectchildren until every row has been mounted (_TableElement.insertRenderObjectChildis a no-op while_doingMountOrUpdate; the children are wired in later by_updateRenderObjectChildren). Meanwhile_OverlayPortalElement.mountrunssuper.mount(which "attaches" the layout surrogate to its parent — but Table swallows it) and then immediately mounts the overlay child, which causes the deferred-layout box to be adopted by the_RenderTheater. The theater is attached, so the deferred box receives an owner — while the layout surrogate has none yet.When the theater calls
redepthChild(deferredBox), the deferred box'sredepthChildrenunconditionally calls_layoutSurrogate.redepthChild(this), which trips thechild.owner == ownerassertion across that owner boundary.Fix
_RenderDeferredLayoutBox.redepthChildrennow skips the cross-redepth when the surrogate is not yet attached, mirroring the existing guard in_RenderLayoutSurrogateProxyBox.redepthChildren:The depth invariant is restored when the surrogate is finally adopted by its parent:
_RenderLayoutSurrogateProxyBox.redepthChildrenalready callsredepthChild(_deferredLayoutChild)once that child becomes attached.Tests
Adds a regression test in
test/widgets/table_test.dartplacing anOverlayPortalwhose controller is.show()'d before mount (mirroringSlider) inside aTableRow, asserting that no exception is thrown and the overlay child is present.Without this fix the new test reproduces the assertion. With this fix
flutter test test/widgets/table_test.dart,test/widgets/overlay_portal_test.dart,test/widgets/overlay_test.dart,test/widgets/raw_tooltip_test.dart,test/widgets/overlay_layout_builder_test.dart, andtest/material/slider_test.dartall pass.Pre-launch Checklist