Dispose all disposables in the Network screen (#8937)

* Dispose all disposables in the Network screen

* rnotes

* fix bugs and tests

* rnotes

* add back accidental deletion

* comments

* dcm lints

Author: kenzieschmoll

packages/devtools_app/lib/src/screens/inspector_v2/inspector_tree_controller.dart

@@ -1295,7 +1295,7 @@ class _RowPainter extends CustomPainter {
       final width =
           showExpandCollapse
               ? inspectorColumnIndent * 0.45
-              : inspectorColumnIndent * .6;
+              : inspectorColumnIndent * 0.6;
       canvas.drawLine(
         Offset(parentExpandCollapseX, 0.0),
         Offset(parentExpandCollapseX, inspectorRowHeight * 0.5),

packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_connection.dart

@@ -79,7 +79,9 @@ class ChartVmConnection extends DisposableController
       ),
     );
 
-    _polling = DebounceTimer.periodic(chartUpdateDelay, () async {
+    _polling = DebounceTimer.periodic(chartUpdateDelay, ({
+      DebounceCancelledCallback? cancelledCallback,
+    }) async {
       if (!_isConnected) {
         _polling?.cancel();
         return;

packages/devtools_app/lib/src/screens/network/network_controller.dart

@@ -45,24 +45,22 @@ enum NetworkResponseViewType {
 
 enum _NetworkTrafficType { http, socket }
 
+/// Screen controller for the Network screen.
+///
+/// This controller can be accessed from anywhere in DevTools, as long as it was
+/// first registered, by
+/// calling `screenControllers.lookup<NetworkController>()`.
+///
+/// The controller lifecycle is managed by the [ScreenControllers] class. The
+/// `init` method is called lazily upon the first controller access from
+/// `screenControllers`. The `dispose` method is called by `screenControllers`
+/// when DevTools is destroying a set of DevTools screen controllers.
 class NetworkController extends DevToolsScreenController
     with
         SearchControllerMixin<NetworkRequest>,
         FilterControllerMixin<NetworkRequest>,
         OfflineScreenControllerMixin,
         AutoDisposeControllerMixin {
-  NetworkController() {
-    _networkService = NetworkService(this);
-    _currentNetworkRequests = CurrentNetworkRequests();
-    _initHelper();
-    addAutoDisposeListener(
-      _currentNetworkRequests,
-      _filterAndRefreshSearchMatches,
-    );
-    // TODO(https://github.com/flutter/devtools/issues/7727): add support for
-    // persisting network filter.
-    initFilterController();
-  }
   List<DartIOHttpRequestData>? _httpRequests;
 
   Future<String?> exportAsHarFile() async {
@@ -146,9 +144,7 @@ class NetworkController extends DevToolsScreenController
   ValueListenable<bool> get recordingNotifier => _recordingNotifier;
   final _recordingNotifier = ValueNotifier<bool>(false);
 
-  @visibleForTesting
-  NetworkService get networkService => _networkService;
-  late NetworkService _networkService;
+  final networkService = NetworkService();
 
   /// The timeline timestamps are relative to when the VM started.
   ///
@@ -168,6 +164,32 @@ class NetworkController extends DevToolsScreenController
   @visibleForTesting
   bool get isPolling => _pollingTimer != null;
 
+  static const _pollingDuration = Duration(milliseconds: 2000);
+
+  @override
+  void init() {
+    super.init();
+    _currentNetworkRequests = CurrentNetworkRequests();
+    _initHelper();
+    addAutoDisposeListener(
+      _currentNetworkRequests,
+      _filterAndRefreshSearchMatches,
+    );
+    initFilterController();
+  }
+
+  @override
+  void dispose() {
+    // Cancel and dispose the polling timer before disposing anything else.
+    _pollingTimer?.dispose();
+    _pollingTimer = null;
+    _currentResponseViewType.dispose();
+    selectedRequest.dispose();
+    _recordingNotifier.dispose();
+    _currentNetworkRequests.dispose();
+    super.dispose();
+  }
+
   void _initHelper() async {
     if (offlineDataController.showingOfflineData.value) {
       await maybeLoadOfflineData(
@@ -254,8 +276,8 @@ class NetworkController extends DevToolsScreenController
       _pollingTimer ??= DebounceTimer.periodic(
         // TODO(kenz): look into improving performance by caching more data.
         // Polling less frequently helps performance.
-        const Duration(milliseconds: 2000),
-        _networkService.refreshNetworkData,
+        _pollingDuration,
+        networkService.refreshNetworkData,
       );
     } else {
       _pollingTimer?.cancel();
@@ -286,10 +308,10 @@ class NetworkController extends DevToolsScreenController
     // Cancel existing polling timer before starting recording.
     _updatePollingState(false);
 
-    _networkService.updateLastHttpDataRefreshTime(
+    networkService.updateLastHttpDataRefreshTime(
       alreadyRecordingHttp: alreadyRecordingHttp,
     );
-    final timestamp = await _networkService.updateLastSocketDataRefreshTime(
+    final timestamp = await networkService.updateLastSocketDataRefreshTime(
       alreadyRecordingSocketData: alreadyRecordingSocketData,
     );
 
@@ -319,7 +341,9 @@ class NetworkController extends DevToolsScreenController
   }
 
   Future<void> stopRecording() async {
-    await togglePolling(false);
+    if (!disposed) {
+      await togglePolling(false);
+    }
   }
 
   Future<void> togglePolling(bool state) async {
@@ -339,8 +363,8 @@ class NetworkController extends DevToolsScreenController
   /// This will ensure that future fetches for http and socket requests will at
   /// most fetch requests since [updateLastRefreshTime] was called.
   Future<void> updateLastRefreshTime() async {
-    _networkService.updateLastHttpDataRefreshTime();
-    await _networkService.updateLastSocketDataRefreshTime();
+    networkService.updateLastHttpDataRefreshTime();
+    await networkService.updateLastSocketDataRefreshTime();
   }
 
   Future<bool> _recordingNetworkTraffic({
@@ -370,12 +394,21 @@ class NetworkController extends DevToolsScreenController
   /// Clears the HTTP profile and socket profile from the vm, and resets the
   /// last refresh timestamp to the current time.
   Future<void> clear() async {
-    await _networkService.clearData();
+    await networkService.clearData();
     _currentNetworkRequests.clear();
     _filterAndRefreshSearchMatches();
     _updateSelection();
   }
 
+  @override
+  void setActiveFilter({
+    String? query,
+    SettingFilters<NetworkRequest>? settingFilters,
+  }) {
+    super.setActiveFilter(query: query, settingFilters: settingFilters);
+    _filterAndRefreshSearchMatches();
+  }
+
   void _filterAndRefreshSearchMatches() {
     filterData(activeFilter.value);
     refreshSearchMatches();

packages/devtools_app/lib/src/screens/network/network_screen.dart

@@ -143,10 +143,10 @@ class _NetworkScreenBodyState extends State<NetworkScreenBody>
 
   @override
   void dispose() {
+    unawaited(controller.stopRecording());
     // TODO(kenz): this won't work well if we eventually have multiple clients
     // that want to listen to network data.
     super.dispose();
-    unawaited(controller.stopRecording());
   }
 
   @override

packages/devtools_app/lib/src/screens/network/network_service.dart

@@ -6,12 +6,12 @@ import 'package:vm_service/vm_service.dart';
 
 import '../../shared/globals.dart';
 import '../../shared/primitives/utils.dart';
+import '../../shared/utils/utils.dart';
 import 'network_controller.dart';
 
 class NetworkService {
-  NetworkService(this.networkController);
-
-  final NetworkController networkController;
+  NetworkController get networkController =>
+      screenControllers.lookup<NetworkController>();
 
   /// Tracks the time (microseconds since epoch) that the HTTP profile was last
   /// retrieved for a given isolate ID.
@@ -61,15 +61,26 @@ class NetworkService {
 
   /// Force refreshes the HTTP requests logged to the timeline as well as any
   /// recorded Socket traffic.
-  Future<void> refreshNetworkData() async {
+  ///
+  /// This method calls `cancelledCallback` after each async gap to ensure that
+  /// this operation has not been cancelled during the async gap.
+  Future<void> refreshNetworkData({
+    DebounceCancelledCallback? cancelledCallback,
+  }) async {
     if (serviceConnection.serviceManager.service == null) return;
     final timestampObj =
         await serviceConnection.serviceManager.service!.getVMTimelineMicros();
+    if (cancelledCallback?.call() ?? false) return;
+
     final timestamp = timestampObj.timestamp!;
     final sockets = await _refreshSockets();
+    if (cancelledCallback?.call() ?? false) return;
+
     networkController.lastSocketDataRefreshMicros = timestamp;
     List<HttpProfileRequest>? httpRequests;
     httpRequests = await _refreshHttpProfile();
+    if (cancelledCallback?.call() ?? false) return;
+
     networkController.processNetworkTraffic(
       sockets: sockets,
       httpRequests: httpRequests,

packages/devtools_app/lib/src/screens/vm_developer/object_inspector/vm_ic_data_display.dart

@@ -34,7 +34,7 @@ class _VmICDataDisplayState extends State<VmICDataDisplay> {
   final entries = <ObjRef?>[];
 
   Future<void> get _initialized => _initializingCompleter.future;
-  final Completer<void> _initializingCompleter = Completer<void>();
+  final _initializingCompleter = Completer<void>();
 
   @override
   void initState() {

packages/devtools_app/lib/src/shared/utils/utils.dart

@@ -185,6 +185,8 @@ extension IsKeyType on KeyEvent {
   bool get isKeyDownOrRepeat => this is KeyDownEvent || this is KeyRepeatEvent;
 }
 
+typedef DebounceCancelledCallback = bool Function();
+
 /// A helper class for [Timer] functionality, where the callbacks are debounced.
 class DebounceTimer {
   /// A periodic timer that ensures [callback] is only called at most once
@@ -193,8 +195,11 @@ class DebounceTimer {
   /// [callback] is triggered once immediately, and then every [duration] the
   /// timer checks to see if the previous [callback] call has finished running.
   /// If it has finished, then then next call to [callback] will begin.
-  DebounceTimer.periodic(Duration duration, Future<void> Function() callback)
-    : _callback = callback {
+  DebounceTimer.periodic(
+    Duration duration,
+    Future<void> Function({DebounceCancelledCallback? cancelledCallback})
+    callback,
+  ) : _callback = callback {
     // Start running the first call to the callback.
     _runCallback();
 
@@ -210,23 +215,28 @@ class DebounceTimer {
       return;
     }
 
+    if (isCancelled) return;
+
     try {
       _isRunning = true;
-      await _callback();
+      await _callback(cancelledCallback: () => isCancelled);
     } finally {
       _isRunning = false;
     }
   }
 
-  late final Timer _timer;
-  final Future<void> Function() _callback;
+  Timer? _timer;
+  final Future<void> Function({DebounceCancelledCallback? cancelledCallback})
+  _callback;
   bool _isRunning = false;
+  bool _isCancelled = false;
 
   void cancel() {
-    _timer.cancel();
+    _isCancelled = true;
+    _timer?.cancel();
   }
 
-  bool get isCancelled => !_timer.isActive;
+  bool get isCancelled => _isCancelled || (_timer != null && !_timer!.isActive);
 
   void dispose() {
     cancel();

packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md

@@ -23,7 +23,8 @@ To learn more about DevTools, check out the
 [#8932](https://github.com/flutter/devtools/pull/8932),
 [#8933](https://github.com/flutter/devtools/pull/8933),
 [#8934](https://github.com/flutter/devtools/pull/8934),
-[#8935](https://github.com/flutter/devtools/pull/8935)
+[#8935](https://github.com/flutter/devtools/pull/8935),
+[#8937](https://github.com/flutter/devtools/pull/8937)
 
 ## Inspector updates
 

packages/devtools_app/test/screens/network/network_controller_test.dart

@@ -23,6 +23,7 @@ void main() {
 
     setUp(() {
       setGlobal(OfflineDataController, OfflineDataController());
+      setGlobal(ScreenControllers, ScreenControllers());
       socketProfile = loadSocketProfile();
       httpProfile = loadHttpProfile();
       fakeServiceConnection = FakeServiceConnectionManager(
@@ -33,7 +34,13 @@ void main() {
       );
       setGlobal(ServiceConnectionManager, fakeServiceConnection);
       setGlobal(PreferencesController, PreferencesController());
-      controller = NetworkController();
+      screenControllers.register<NetworkController>(() => NetworkController());
+      // Lookup the controller immediately to force initialization.
+      controller = screenControllers.lookup<NetworkController>();
+    });
+
+    tearDown(() {
+      screenControllers.disposeConnectedControllers();
     });
 
     test('initialize recording state', () async {
@@ -48,11 +55,10 @@ void main() {
 
     test('start and pause recording', () async {
       expect(controller.isPolling, false);
-      final notifier = controller.recordingNotifier;
       await addListenerScope(
-        listenable: notifier,
+        listenable: controller.recordingNotifier,
         listener: () {
-          expect(notifier.value, true);
+          expect(controller.recordingNotifier.value, true);
           expect(controller.isPolling, true);
         },
         callback: () async {
@@ -62,16 +68,16 @@ void main() {
 
       // Pause polling.
       await controller.togglePolling(false);
-      expect(notifier.value, false);
+      expect(controller.recordingNotifier.value, false);
       expect(controller.isPolling, false);
 
       // Resume polling.
       await controller.togglePolling(true);
-      expect(notifier.value, true);
+      expect(controller.recordingNotifier.value, true);
       expect(controller.isPolling, true);
 
       await controller.stopRecording();
-      expect(notifier.value, false);
+      expect(controller.recordingNotifier.value, false);
       expect(controller.isPolling, false);
     });
 

packages/devtools_app/test/screens/network/network_model_test.dart

@@ -87,10 +87,17 @@ void main() {
       setGlobal(ServiceConnectionManager, fakeServiceConnection);
       setGlobal(PreferencesController, PreferencesController());
       setGlobal(OfflineDataController, OfflineDataController());
-      controller = NetworkController();
+      setGlobal(ScreenControllers, ScreenControllers());
+      screenControllers.register<NetworkController>(() => NetworkController());
+      // Lookup the controller immediately to force initialization.
+      controller = screenControllers.lookup<NetworkController>();
       await controller.startRecording();
     });
 
+    tearDown(() {
+      screenControllers.disposeConnectedControllers();
+    });
+
     test('method returns correct value', () {
       expect(httpGet.method, 'GET');
       expect(httpGetWithError.method, 'GET');