Allow devtools_server to re-use a disconnected DevTools instance (#8009)

packages/devtools_app/lib/devtools.dart

@@ -9,4 +9,4 @@
 // the constant declaration `const version =`.
 // If you change the declaration you must also modify the regex in
 // tools/update_version.dart.
-const version = '2.37.0';
+const version = '2.37.1';

packages/devtools_app/lib/src/app.dart

@@ -55,6 +55,7 @@ import 'shared/analytics/metrics.dart';
 import 'shared/common_widgets.dart';
 import 'shared/console/primitives/simple_items.dart';
 import 'shared/feature_flags.dart';
+import 'shared/framework_controller.dart';
 import 'shared/globals.dart';
 import 'shared/offline_data.dart';
 import 'shared/offline_screen.dart';
@@ -140,6 +141,10 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
     super.initState();
     setGlobal(GlobalKey<NavigatorState>, routerDelegate.navigatorKey);
 
+    autoDisposeStreamSubscription(
+      frameworkController.onConnectVmEvent.listen(_connectVm),
+    );
+
     // TODO(https://github.com/flutter/devtools/issues/6018): Once
     // https://github.com/flutter/flutter/issues/129692 is fixed, disable the
     // browser's native context menu on secondary-click, and instead use the
@@ -190,6 +195,18 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
     _clearCachedRoutes();
   }
 
+  /// Connects to the VM with the given URI.
+  ///
+  /// This request usually comes from the IDE via the server API to reuse the
+  /// DevTools window after being disconnected (for example if the user stops
+  /// a debug session then launches a new one).
+  Future<void> _connectVm(ConnectVmEvent event) async {
+    await routerDelegate.updateArgsIfChanged({
+      'uri': event.serviceProtocolUri.toString(),
+      if (event.notify) 'notify': 'true',
+    });
+  }
+
   /// Gets the page for a given page/path and args.
   Page _getPage(
     BuildContext context,

packages/devtools_app/lib/src/framework/disconnect_observer.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:async';
+
 import 'package:devtools_app_shared/service.dart';
 import 'package:devtools_app_shared/ui.dart';
 import 'package:devtools_app_shared/utils.dart';
@@ -61,7 +63,7 @@ class DisconnectObserverState extends State<DisconnectObserver>
           // We became disconnected by means other than a manual disconnect
           // action, so show the overlay and ensure the 'uri' query paraemter
           // has been cleared.
-          widget.routerDelegate.clearUriParameter();
+          unawaited(widget.routerDelegate.clearUriParameter());
           showDisconnectedOverlay();
         }
       }

packages/devtools_app/lib/src/framework/home_screen.dart

@@ -291,7 +291,7 @@ class _ConnectInputState extends State<ConnectInput> with BlockingActionMixin {
     if (connected) {
       final connectedUri =
           Uri.parse(serviceConnection.serviceManager.serviceUri!);
-      routerDelegate.updateArgsIfChanged({'uri': '$connectedUri'});
+      await routerDelegate.updateArgsIfChanged({'uri': '$connectedUri'});
       final shortUri = connectedUri.replace(path: '');
       notificationService.push('Successfully connected to $shortUri.');
     } else if (normalizeVmServiceUri(uri) == null) {

packages/devtools_app/lib/src/framework/initializer.dart

@@ -11,7 +11,6 @@ import 'package:flutter/material.dart';
 import '../shared/analytics/constants.dart' as gac;
 import '../shared/common_widgets.dart';
 import '../shared/connection_info.dart';
-import '../shared/framework_controller.dart';
 import '../shared/globals.dart';
 import '../shared/routing.dart';
 
@@ -51,9 +50,6 @@ class _InitializerState extends State<Initializer>
   @override
   void initState() {
     super.initState();
-    autoDisposeStreamSubscription(
-      frameworkController.onConnectVmEvent.listen(_connectVm),
-    );
 
     _timer = Timer(_waitForConnectionTimeout, () {
       setState(() {
@@ -68,18 +64,6 @@ class _InitializerState extends State<Initializer>
     super.dispose();
   }
 
-  /// Connects to the VM with the given URI.
-  ///
-  /// This request usually comes from the IDE via the server API to reuse the
-  /// DevTools window after being disconnected (for example if the user stops
-  /// a debug session then launches a new one).
-  void _connectVm(ConnectVmEvent event) {
-    DevToolsRouterDelegate.of(context).updateArgsIfChanged({
-      'uri': event.serviceProtocolUri.toString(),
-      if (event.notify) 'notify': 'true',
-    });
-  }
-
   @override
   Widget build(BuildContext context) {
     return ValueListenableBuilder(

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

@@ -52,18 +52,6 @@ class DevToolsRouteInformationParser
       uri = uri.copyWith(queryParameters: _testQueryParams!.params);
     }
 
-    final uriFromParams = uri.queryParameters['uri'];
-    if (uriFromParams == null) {
-      // If the uri has been modified and we do not have a vm service uri as a
-      // query parameter, ensure we manually disconnect from any previously
-      // connected applications.
-      await serviceConnection.serviceManager.manuallyDisconnect();
-    } else if (_testQueryParams == null) {
-      // Otherwise, connect to the vm service from the query parameter before
-      // loading the route (but do not do this in a testing environment).
-      await FrameworkCore.initVmService(serviceUriAsString: uriFromParams);
-    }
-
     // routeInformation.path comes from the address bar and (when not empty) is
     // prefixed with a leading slash. Internally we use "page IDs" that do not
     // start with slashes but match the screenId for each screen.
@@ -112,11 +100,23 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
         ChangeNotifier,
         PopNavigatorRouterDelegateMixin<DevToolsRouteConfiguration> {
   DevToolsRouterDelegate(this._getPage, [GlobalKey<NavigatorState>? key])
-      : navigatorKey = key ?? GlobalKey<NavigatorState>();
+      : navigatorKey = key ?? GlobalKey<NavigatorState>(),
+        _isTestMode = false;
+
+  @visibleForTesting
+  DevToolsRouterDelegate.test(this._getPage, [GlobalKey<NavigatorState>? key])
+      : navigatorKey = key ?? GlobalKey<NavigatorState>(),
+        _isTestMode = true;
 
   static DevToolsRouterDelegate of(BuildContext context) =>
       Router.of(context).routerDelegate as DevToolsRouterDelegate;
 
+  /// Whether or not the router is being used for testing purposes.
+  ///
+  /// This is to be used in a testing environment only and can be set via the
+  /// [DevToolsRouterDelegate.test] constructor.
+  final bool _isTestMode;
+
   @override
   final GlobalKey<NavigatorState> navigatorKey;
 
@@ -199,15 +199,10 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
     final newParams = currentConfiguration?.params.withUpdates(argUpdates) ??
         DevToolsQueryParams.empty();
 
-    // Ensure we disconnect from any previously connected applications if we do
-    // not have a vm service uri as a query parameter, unless we are loading an
-    // offline file.
-    if (page != snapshotScreenId && newParams.vmServiceUri == null) {
-      unawaited(serviceConnection.serviceManager.manuallyDisconnect());
-    }
-
-    _replaceStack(
-      DevToolsRouteConfiguration(page, newParams, state),
+    unawaited(
+      _replaceStack(
+        DevToolsRouteConfiguration(page, newParams, state),
+      ),
     );
     notifyListeners();
   }
@@ -226,16 +221,30 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
   }
 
   /// Replaces the navigation stack with a new route.
-  void _replaceStack(DevToolsRouteConfiguration configuration) {
+  Future<void> _replaceStack(DevToolsRouteConfiguration configuration) async {
     _currentPage = configuration.page;
     _routes
       ..clear()
       ..add(configuration);
+
+    if (configuration.page != snapshotScreenId) {
+      // Handle changing the VM service connection (ignored if we are loading an
+      // offline file):
+      final vmServiceUri = configuration.params.vmServiceUri;
+
+      if (vmServiceUri == null) {
+        // Disconnect from any previously connected applications if we do not
+        // have a vm service uri as a query parameter.
+        await serviceConnection.serviceManager.manuallyDisconnect();
+      } else {
+        await _maybeConnectToVmService(vmServiceUri);
+      }
+    }
   }
 
   @override
-  Future<void> setNewRoutePath(DevToolsRouteConfiguration configuration) {
-    _replaceStack(configuration);
+  Future<void> setNewRoutePath(DevToolsRouteConfiguration configuration) async {
+    await _replaceStack(configuration);
     notifyListeners();
     return SynchronousFuture<void>(null);
   }
@@ -244,7 +253,7 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
   ///
   /// Existing arguments (for example &uri=) will be preserved unless
   /// overwritten by [argUpdates].
-  void updateArgsIfChanged(Map<String, String?> argUpdates) {
+  Future<void> updateArgsIfChanged(Map<String, String?> argUpdates) async {
     final argsChanged = _changesArgs(argUpdates);
     if (!argsChanged) {
       return;
@@ -253,7 +262,8 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
     final currentConfig = currentConfiguration!;
     final currentPage = currentConfig.page;
     final newArgs = currentConfig.params.withUpdates(argUpdates);
-    _replaceStack(
+
+    await _replaceStack(
       DevToolsRouteConfiguration(
         currentPage,
         newArgs,
@@ -263,13 +273,13 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
     notifyListeners();
   }
 
-  void clearUriParameter() {
-    updateArgsIfChanged({'uri': null});
+  Future<void> clearUriParameter() async {
+    await updateArgsIfChanged({'uri': null});
   }
 
   Future<void> replaceState(DevToolsNavigationState state) async {
     final currentConfig = currentConfiguration!;
-    _replaceStack(
+    await _replaceStack(
       DevToolsRouteConfiguration(
         currentConfig.page,
         currentConfig.params,
@@ -298,11 +308,13 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
     }
 
     final currentConfig = currentConfiguration!;
-    _replaceStack(
-      DevToolsRouteConfiguration(
-        currentConfig.page,
-        currentConfig.params,
-        currentConfig.state?.merge(stateUpdate) ?? stateUpdate,
+    unawaited(
+      _replaceStack(
+        DevToolsRouteConfiguration(
+          currentConfig.page,
+          currentConfig.params,
+          currentConfig.state?.merge(stateUpdate) ?? stateUpdate,
+        ),
       ),
     );
     // Add the new state to the browser history.
@@ -328,6 +340,15 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
     }
     return currentState.hasChanges(changes);
   }
+
+  /// Connects to the VM Service if it is not already connected.
+  Future<void> _maybeConnectToVmService(String vmServiceUri) async {
+    final alreadyConnected =
+        serviceConnection.serviceManager.connectedState.value.connected;
+    // Skip connecting if we are already connected or in a test environment.
+    if (alreadyConnected || _isTestMode) return;
+    await FrameworkCore.initVmService(serviceUriAsString: vmServiceUri);
+  }
 }
 
 /// Encapsulates state associated with a [Router] navigation event.

packages/devtools_app/pubspec.yaml

@@ -4,7 +4,7 @@ publish_to: none
 
 # Note: this version should only be updated by running tools/update_version.dart
 # that updates all versions of devtools packages (devtools_app, devtools_test).
-version: 2.37.0
+version: 2.37.1
 
 repository: https://github.com/flutter/devtools/tree/master/packages/devtools_app
 

packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md

@@ -1,9 +1,9 @@
 This is draft for future release notes, that are going to land on
 [the Flutter website](https://docs.flutter.dev/tools/devtools/release-notes).
 
-# DevTools 2.37.0 release notes
+# DevTools 2.37.1 release notes
 
-The 2.37.0 release of the Dart and Flutter DevTools
+The 2.37.1 release of the Dart and Flutter DevTools
 includes the following changes among other general improvements.
 To learn more about DevTools, check out the
 [DevTools overview](/tools/devtools/overview).
@@ -14,6 +14,8 @@ To learn more about DevTools, check out the
 connected app. - [#7958](https://github.com/flutter/devtools/pull/7958)
 * Fix a bug where an infinite spinner was shown upon app
 disconnect. - [#7992](https://github.com/flutter/devtools/pull/7992)
+* Fix a bug where trying to reuse a disconnected DevTools instance would
+fail. - [#8009](https://github.com/flutter/devtools/pull/8009)
 
 ## Inspector updates
 

packages/devtools_app/test/logging/logging_screen_v2/logging_model_test.dart

@@ -7,10 +7,12 @@ import 'dart:async';
 import 'package:devtools_app/src/screens/logging/logging_screen_v2/logging_controller_v2.dart';
 import 'package:devtools_app/src/screens/logging/logging_screen_v2/logging_model.dart';
 import 'package:devtools_app/src/screens/logging/logging_screen_v2/logging_table_row.dart';
+import 'package:devtools_app/src/service/service_manager.dart';
 import 'package:devtools_app/src/shared/globals.dart';
 import 'package:devtools_app/src/shared/preferences/preferences.dart';
 import 'package:devtools_app_shared/ui.dart';
 import 'package:devtools_app_shared/utils.dart';
+import 'package:devtools_test/devtools_test.dart';
 import 'package:devtools_test/helpers.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_test/flutter_test.dart';
@@ -26,6 +28,7 @@ void main() {
       setGlobal(PreferencesController, PreferencesController());
       setGlobal(IdeTheme, getIdeTheme());
       setGlobal(GlobalKey<NavigatorState>, GlobalKey<NavigatorState>());
+      setGlobal(ServiceConnectionManager, FakeServiceConnectionManager());
       TestWidgetsFlutterBinding.ensureInitialized();
       loggingTableModel = LoggingTableModel();
     });

packages/devtools_app/test/standalone_ui/vs_code/debug_sessions_test.dart

@@ -26,6 +26,7 @@ void main() {
   setUpAll(() {
     // Set test mode so that the debug list of extensions will be used.
     setTestMode();
+    setGlobal(ServiceConnectionManager, FakeServiceConnectionManager());
 
     final devices = stubbedDevices.map((d) => MapEntry(d.id, d));
     deviceMap = {for (final d in devices) d.key: d.value};

packages/devtools_app/test/standalone_ui/vs_code/devtools_test.dart

@@ -41,6 +41,7 @@ void main() {
     when(mockEditorClient.supportsHotRestart).thenReturn(true);
     setGlobal(IdeTheme, IdeTheme());
     setGlobal(PreferencesController, PreferencesController());
+    setGlobal(ServiceConnectionManager, FakeServiceConnectionManager());
 
     final mockDtdManager = MockDTDManager();
     when(

packages/devtools_test/lib/src/helpers/wrappers.dart

@@ -39,7 +39,7 @@ Widget wrap(Widget widget, {DevToolsQueryParams? queryParams}) {
         colorScheme: lightColorScheme,
       ),
     ),
-    routerDelegate: DevToolsRouterDelegate(
+    routerDelegate: DevToolsRouterDelegate.test(
       (context, page, args, state) => MaterialPage(
         child: Material(
           child: Directionality(

packages/devtools_test/lib/src/mocks/generated_mocks_factories.dart

@@ -174,6 +174,8 @@ MockServiceManager<VmServiceWrapper> _createMockServiceManagerWithDefaults() {
   when(mockServiceManager.isolateManager).thenReturn(fakeIsolateManager);
   when(mockServiceManager.serviceExtensionManager)
       .thenReturn(fakeServiceExtensionManager);
+  when(mockServiceManager.connectedState)
+      .thenReturn(ValueNotifier(const ConnectedState(true)));
   return mockServiceManager;
 }