Disallow dispose during listener callback #114530

chunhtai · 2022-11-02T16:51:39Z

I think this is the correct way to handle the lifecycle of change listenable, but this may be a hard breaking change that requires non trivial migration.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer

Overall it seems reasonable to me to disallow dispose during callback execution.

goderbauer · 2022-11-02T16:58:33Z

packages/flutter/lib/src/foundation/change_notifier.dart

    assert(() {
      _debugDisposed = true;
      return true;
    }());
    if (kFlutterMemoryAllocationsEnabled && _creationDispatched) {
      MemoryAllocations.instance.dispatchObjectDisposed(object: this);
    }
+    print('dispose is called $_notificationCallStackDepth $_reentrantlyRemovedListeners, _debugDisposed $_debugDisposed');


remove this?

oops, thanks for catching it

goderbauer · 2022-11-02T16:59:23Z

packages/flutter/test/foundation/change_notifier_test.dart

@@ -49,6 +49,23 @@ class Counter with ChangeNotifier {
 }

 void main() {
+  testWidgets('ValueNotifier can not dispose in callback', (WidgetTester tester) async {


Should we test this for one level deeper on the ChangeNotifier as well?

goderbauer · 2022-11-02T17:02:01Z

packages/flutter/lib/src/foundation/change_notifier.dart

@@ -321,14 +321,17 @@ class ChangeNotifier implements Listenable {
  @mustCallSuper
  void dispose() {
    assert(ChangeNotifier.debugAssertNotDisposed(this));
+    assert(_notificationCallStackDepth == 0 && _reentrantlyRemovedListeners == 0, 'dispose can not be called during listener callbacks');


Given the error message, should we just assert on the _notificationCallStackDepth? That's the only thing that indicates callback execution, no?

goderbauer · 2022-11-02T17:03:18Z

packages/flutter/lib/src/foundation/change_notifier.dart

    _listeners = _emptyListeners;
+    _reentrantlyRemovedListeners = 0;


If we assert that it is 0 above, this would be a no-op, no?

with this, it won't crash in release build

Maybe leave that as a comment here for future readers?

surely it would still cause a crash? the notifyListeners method is going to call _listeners[i] with values of i bigger than _emptyListeners.length if it's in the middle of dispatching listeners.

goderbauer · 2022-11-02T17:04:32Z

The analyzer is unhappy.

goderbauer

LGTM

goderbauer · 2022-11-02T21:14:52Z

packages/flutter/lib/src/foundation/change_notifier.dart

    _listeners = _emptyListeners;
+    _reentrantlyRemovedListeners = 0;


Maybe leave that as a comment here for future readers?

auto-submit · 2022-11-02T21:39:16Z

auto label is removed for flutter/flutter, pr: 114530, due to - The status or check suite Mac tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label.

Hixie · 2022-11-02T21:56:42Z

packages/flutter/lib/src/foundation/change_notifier.dart

@@ -321,6 +321,7 @@ class ChangeNotifier implements Listenable {
  @mustCallSuper
  void dispose() {
    assert(ChangeNotifier.debugAssertNotDisposed(this));
+    assert(_notificationCallStackDepth == 0, 'dispose can not be called during listener callbacks');


Maybe 'The "dispose()" method on $this was called during the call to "notifyListeners()". This is likely to cause errors since it modifies the list of listeners while the list is being used.' would be clearer?

FWIW, "can not" means that it is possible to not do something. "cannot" means that it is not possible to do something.

So "I can not eat candy because I have self-control, but I do eat it a lot" vs "I cannot eat candy because I am allergic, so I do not eat any."

done. I also removed the attempt to clean it up in case it is called in release mode. if we were to properly clean up, it would probably have performance impact given this class is used a lot.

goderbauer

LGTM

…4530)

* 87cb150 InkResponse highlights can be updated (flutter/flutter#115635) * 10c039b ca5b2f31c Roll Skia from d693b4b9fe5e to ab054a88d7c7 (1 revision) (flutter/engine#37743) (flutter/flutter#115654) * 7fd3bb2 Roll Plugins from a2fa4e9 to b2fe01b (6 revisions) (flutter/flutter#115656) * 3a3a0db Disallow dispose during listener callback (flutter/flutter#114530) * ca78327 Fix `NavigationBar` ripple (flutter/flutter#115499) * 4593917 [flutter_tools] Pin package:archive and manual roll (flutter/flutter#115662) * 9807761 Tiny improvement of RouteSettings display (flutter/flutter#114481) * 2cb2e84 Add more InkWell tests (flutter/flutter#115634) * 24865ad Remove unneeded comparison to `double.nan` (flutter/flutter#115547) * d2e6dfe Fix formatting in BottomAppBar test (flutter/flutter#115520) * 22e1ac7 b32c3a307 Roll Skia from ab054a88d7c7 to 345bceacd298 (3 revisions) (flutter/engine#37745) (flutter/flutter#115658) * 01c1e8e Allows pushing page based route as pageless route (flutter/flutter#114362) * 0e57147 nav drawer (flutter/flutter#115668) * cf2c9f6 Remove package:image dependency (flutter/flutter#115674) * 46d868c Roll Flutter Engine from b32c3a307bb5 to 7a390f97c798 (14 revisions) (flutter/flutter#115672) * 0fc4a3e Revert "Remove package:image dependency (#115674)" (flutter/flutter#115680) * c69fbf1 a77dfaff3 Remove `Linux Web Engine` from recipes CQ (flutter/engine#37758) (flutter/flutter#115675) * c95b693 f81ac3b19 Fix glyph sampling overlap (flutter/engine#37764) (flutter/flutter#115683) * be0c3a7 cef11cb9a Roll Skia from 12f01bc5b57e to c53d8cf5b823 (4 revisions) (flutter/engine#37767) (flutter/flutter#115684) * ae18d7b 6da59402e Roll Skia from c53d8cf5b823 to 0c1fcbe97b1f (1 revision) (flutter/engine#37771) (flutter/flutter#115685) * a17b4c3 91bc694eb Roll Fuchsia Mac SDK from tklUfTsSOVKk49tYq... to UcfQiA4PBOCs_7GEK... (flutter/engine#37773) (flutter/flutter#115686) * 1bee357 2d5e0667e Roll Fuchsia Linux SDK from WdtwlLEce90PjFJ9z... to qc20R_3e8PoqMQWgw... (flutter/engine#37775) (flutter/flutter#115687) * 633d7ef 916fd798d Roll Skia from 0c1fcbe97b1f to ad354e712b96 (2 revisions) (flutter/engine#37776) (flutter/flutter#115689)

* Disallow dispose during listener callback * addressing comment * add comments to code * Addressing comments * fix test

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 2, 2022

chunhtai mentioned this pull request Nov 2, 2022

RangeError when both removing listener and disposing ValueNotifier from listener callback #114224

Closed

chunhtai requested review from goderbauer and Hixie November 2, 2022 16:52

goderbauer reviewed Nov 2, 2022

View reviewed changes

chunhtai requested a review from goderbauer November 2, 2022 18:48

goderbauer approved these changes Nov 2, 2022

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

chunhtai force-pushed the issues/114224 branch from 57f7967 to 58821c7 Compare November 2, 2022 21:47

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

Hixie reviewed Nov 2, 2022

View reviewed changes

chunhtai removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2022

chunhtai requested a review from Hixie November 3, 2022 17:34

goderbauer approved these changes Nov 15, 2022

View reviewed changes

chunhtai force-pushed the issues/114224 branch from 47c54bb to 405752e Compare November 15, 2022 23:14

chunhtai added 4 commits November 17, 2022 09:54

Disallow dispose during listener callback

0ddaa58

addressing comment

6e63b12

add comments to code

a6dd09e

Addressing comments

e835090

chunhtai force-pushed the issues/114224 branch from 405752e to e835090 Compare November 17, 2022 17:54

fix test

a4fce6c

chunhtai merged commit 3a3a0db into flutter:master Nov 18, 2022

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to 3a3a0db13e48 (137 revisions) flutter/plugins#6720

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

3a3a0db Disallow dispose during listener callback (flutter/flutter#11…

9cf7174

…4530)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to ca78327402bf (138 revisions) flutter/plugins#6721

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

3a3a0db Disallow dispose during listener callback (flutter/flutter#11…

54e20f5

…4530)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to d2e6dfefa5ca (143 revisions) flutter/plugins#6722

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

3a3a0db Disallow dispose during listener callback (flutter/flutter#11…

cfeded7

…4530)

engine-flutter-autoroll mentioned this pull request Nov 19, 2022

Roll Flutter from 18c87274cdc9 to 633d7ef046c8 (23 revisions) flutter/packages#2827

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 19, 2022

3a3a0db Disallow dispose during listener callback (flutter/flutter#11…

06bf4cb

…4530)

shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022

Disallow dispose during listener callback (flutter#114530)

29af6f7

* Disallow dispose during listener callback * addressing comment * add comments to code * Addressing comments * fix test

gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023

Disallow dispose during listener callback (flutter#114530)

8b1c2e8

* Disallow dispose during listener callback * addressing comment * add comments to code * Addressing comments * fix test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow dispose during listener callback #114530

Disallow dispose during listener callback #114530

chunhtai commented Nov 2, 2022

goderbauer left a comment

goderbauer Nov 2, 2022

chunhtai Nov 2, 2022

goderbauer Nov 2, 2022

goderbauer Nov 2, 2022

goderbauer Nov 2, 2022

chunhtai Nov 2, 2022

goderbauer Nov 2, 2022

Hixie Nov 2, 2022

goderbauer commented Nov 2, 2022

goderbauer left a comment

goderbauer Nov 2, 2022

auto-submit bot commented Nov 2, 2022

Hixie Nov 2, 2022

chunhtai Nov 3, 2022

goderbauer left a comment

		_listeners = _emptyListeners;
		_reentrantlyRemovedListeners = 0;

Disallow dispose during listener callback #114530

Disallow dispose during listener callback #114530

Conversation

chunhtai commented Nov 2, 2022

Pre-launch Checklist

goderbauer left a 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

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

goderbauer commented Nov 2, 2022

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auto-submit bot commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment