Fix: Remove attach target on deactivation of widget from overlay portal controller #164439

rkishan516 · 2025-03-02T05:34:10Z

Fix: Remove attach target on deactivation of widget from overlay portal controller
fixes: #164376

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].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

flutter-dashboard · 2025-03-02T05:34:13Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

Piinks

Hi @rkishan516 thank you for sending a PR!
It looks like this is failing many tests, and lacks tests itself. Can you take a look?
Thanks!

rkishan516 · 2025-03-17T06:14:02Z

@Piinks I have made required changes.

LongCatIsLooong · 2025-03-17T18:01:07Z

packages/flutter/lib/src/widgets/overlay.dart

    }
  }

  @override
-  void dispose() {
-    assert(widget.controller._attachTarget == this);
+  void deactivate() {


If the OverlayPortal is reparented, the _attachTarget has to be restored right? Also could you add a test for this?

I have pushed the required change.

It should probably happen in activate instead of didUpdateWidget.

Yaa, that will be correct. Let me do the change.

LongCatIsLooong

Detaching the controller from the state when it's deactivated makes sense to me.

However this would create a new corner case: when the state is deactivated, we don't want the developer to call show or hide on the associated controller because the state the controller attaches to may not rebuild. I think that can happen if you cache the OverlayPortal and gives it a global key.

LongCatIsLooong · 2025-03-19T22:09:50Z

packages/flutter/lib/src/widgets/overlay.dart

-    assert(widget.controller._attachTarget == this);
+  void deactivate() {
+    super.deactivate();
+    assert(widget.controller._attachTarget == null || widget.controller._attachTarget == this);


I think the assert here should be assert(widget.controller._attachTarget == this)?

Yes, that should be the case but when reattaching with global key, it was causing failures.
But i think since we are moving reattach in activate method, this can be removed.

rkishan516 · 2025-03-20T00:46:51Z

Detaching the controller from the state when it's deactivated makes sense to me.

However this would create a new corner case: when the state is deactivated, we don't want the developer to call show or hide on the associated controller because the state the controller attaches to may not rebuild. I think that can happen if you cache the OverlayPortal and gives it a global key.

I will write a test for this and check.

rkishan516 · 2025-03-20T02:23:47Z

Detaching the controller from the state when it's deactivated makes sense to me.
However this would create a new corner case: when the state is deactivated, we don't want the developer to call show or hide on the associated controller because the state the controller attaches to may not rebuild. I think that can happen if you cache the OverlayPortal and gives it a global key.

I will write a test for this and check.

I have added the test, we don't need to call show method again after reactivation.

LongCatIsLooong · 2025-03-20T16:47:39Z

It seems there's a leak in one of the new tests. Maybe one of the OverlayEntries were not disposed at the end of the test?

LongCatIsLooong · 2025-03-20T17:05:07Z

I think setting _attatchTarget to null when the Element is deactivated could change the behavior in some corner cases, and what we want to fix is just the assert in _setupController. Can fix the asserts by:

In the _setupController assert, if the attached state is not Element.debugIsActive, then we could allow the re-attach,
assert in State.activate that _attachTarget == this

rkishan516 · 2025-03-21T00:58:20Z

I think setting _attatchTarget to null when the Element is deactivated could change the behavior in some corner cases, and what we want to fix is just the assert in _setupController. Can fix the asserts by:

In the _setupController assert, if the attached state is not Element.debugIsActive, then we could allow the re-attach,

assert in State.activate that _attachTarget == this

Element.debugIsActive
Returns true if the Element is active.
This getter always returns false in profile and release builds.

I think this will not solve the issue because in release it will always reattach to newer element.

rkishan516 · 2025-03-21T02:30:52Z

It seems there's a leak in one of the new tests. Maybe one of the OverlayEntries were not disposed at the end of the test?

Even after disposing overlay entries, its showing leak.

LongCatIsLooong · 2025-03-21T03:53:12Z

in release

The issue won't happen in release builds. The crash is caused by an assert.

LongCatIsLooong · 2025-03-21T03:54:49Z

It seems there's a leak in one of the new tests. Maybe one of the OverlayEntries were not disposed at the end of the test?

Even after disposing overlay entries, its showing leak.

The relevant error messages were:

     Actual: <Instance of 'Leaks'>
     Which: contains leaks:
            # The text is generated by leak_tracker.
            # For leak troubleshooting tips open:
            # https://github.com/flutter/flutter/blob/main/docs/contributing/testing/Leak-tracking.md
            notDisposed:
              total: 4
              objects:
                ValueNotifier<_OverlayEntryWidgetState?>:
                  test: attachTarget is restored after reparenting
                  identityHashCode: 933371839
                OverlayEntry:
                  test: attachTarget is restored after reparenting
                  identityHashCode: 947575411
                ValueNotifier<_OverlayEntryWidgetState?>:
                  test: attachTarget is restored after reparenting
                  identityHashCode: 51179505
                OverlayEntry:
                  test: attachTarget is restored after reparenting
                  identityHashCode: 662836894

rkishan516 · 2025-03-21T03:56:39Z

in release

The issue won't happen in release builds. The crash is caused by an assert.

Okay, then I will do the change in _setupController.

LongCatIsLooong · 2025-03-21T03:59:36Z

packages/flutter/test/widgets/overlay_portal_test.dart

+    late OverlayEntry overlayEntry1, overlayEntry2;
+    addTearDown(() {
+      overlayEntry1.dispose();
+      return overlayEntry2.dispose();


nit: remove the return

LongCatIsLooong · 2025-03-21T04:01:32Z

packages/flutter/test/widgets/overlay_portal_test.dart

+    );
+
+    // Move to second overlay
+    setState(() {


I suspect when the StatefulBuilder rebuilds, the builder callback will be called a second time, creating new OverlayEntries so in the test 4 OverlayEntries were created but only 2 were disposed.

Yaa, I need to give GlobalKey to Overlay widgets such that they also maintain themself until test is completed.

Even after adding global key, tests are failing

…al controller

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 2, 2025

Piinks reviewed Mar 11, 2025

View reviewed changes

rkishan516 force-pushed the portal-controller-deactivate branch from ccc208a to a1b0390 Compare March 12, 2025 03:32

rkishan516 requested a review from Piinks March 12, 2025 03:37

Piinks requested a review from LongCatIsLooong March 17, 2025 14:56

LongCatIsLooong reviewed Mar 17, 2025

View reviewed changes

rkishan516 force-pushed the portal-controller-deactivate branch 2 times, most recently from 4c7cdd2 to 1781190 Compare March 18, 2025 01:23

LongCatIsLooong reviewed Mar 19, 2025

View reviewed changes

rkishan516 force-pushed the portal-controller-deactivate branch 4 times, most recently from db30016 to 58584ed Compare March 20, 2025 02:21

rkishan516 force-pushed the portal-controller-deactivate branch 2 times, most recently from 23c1126 to df3f365 Compare March 21, 2025 01:58

rkishan516 force-pushed the portal-controller-deactivate branch from df3f365 to e3304ec Compare March 21, 2025 02:38

LongCatIsLooong reviewed Mar 21, 2025

View reviewed changes

rkishan516 force-pushed the portal-controller-deactivate branch from e3304ec to d6c5f3d Compare March 21, 2025 04:08

Fix: Remove attach target on deactivation of widget from overlay port…

cef835f

…al controller

rkishan516 force-pushed the portal-controller-deactivate branch from d6c5f3d to cef835f Compare March 21, 2025 04:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Remove attach target on deactivation of widget from overlay portal controller #164439

Fix: Remove attach target on deactivation of widget from overlay portal controller #164439

rkishan516 commented Mar 2, 2025 •

edited

Loading

flutter-dashboard bot commented Mar 2, 2025

Piinks left a comment

rkishan516 commented Mar 17, 2025

LongCatIsLooong Mar 17, 2025

rkishan516 Mar 18, 2025

rkishan516 Mar 18, 2025

LongCatIsLooong Mar 19, 2025

rkishan516 Mar 20, 2025

LongCatIsLooong left a comment

LongCatIsLooong Mar 19, 2025

rkishan516 Mar 20, 2025

rkishan516 commented Mar 20, 2025

rkishan516 commented Mar 20, 2025

LongCatIsLooong commented Mar 20, 2025

LongCatIsLooong commented Mar 20, 2025

rkishan516 commented Mar 21, 2025

rkishan516 commented Mar 21, 2025

LongCatIsLooong commented Mar 21, 2025

LongCatIsLooong commented Mar 21, 2025

rkishan516 commented Mar 21, 2025

LongCatIsLooong Mar 21, 2025

LongCatIsLooong Mar 21, 2025

rkishan516 Mar 21, 2025

rkishan516 Mar 22, 2025

Fix: Remove attach target on deactivation of widget from overlay portal controller #164439

Are you sure you want to change the base?

Fix: Remove attach target on deactivation of widget from overlay portal controller #164439

Conversation

rkishan516 commented Mar 2, 2025 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Mar 2, 2025

Piinks left a comment

Choose a reason for hiding this comment

rkishan516 commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkishan516 commented Mar 20, 2025

rkishan516 commented Mar 20, 2025

LongCatIsLooong commented Mar 20, 2025

LongCatIsLooong commented Mar 20, 2025

rkishan516 commented Mar 21, 2025

rkishan516 commented Mar 21, 2025

LongCatIsLooong commented Mar 21, 2025

LongCatIsLooong commented Mar 21, 2025

rkishan516 commented Mar 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkishan516 commented Mar 2, 2025 •

edited

Loading