[MV3 Debug Extension] Handle unexpected extension debugger disconnection events without crashing app

dwds/CHANGELOG.md

@@ -2,6 +2,7 @@
 
 - Fix failure to map JS exceptions to dart. - [#2004](https://github.com/dart-lang/webdev/pull/2004)
 - Fix for listening to custom streams. - [#2011](https://github.com/dart-lang/webdev/pull/2011)
+- Handle unexpected extension debugger disconnect events without crashing the app - [#2021](https://github.com/dart-lang/webdev/pull/2021)
 
 ## 18.0.0
 

dwds/lib/src/handlers/dev_handler.dart

@@ -28,6 +28,7 @@ import 'package:dwds/src/loaders/strategy.dart';
 import 'package:dwds/src/readers/asset_reader.dart';
 import 'package:dwds/src/servers/devtools.dart';
 import 'package:dwds/src/servers/extension_backend.dart';
+import 'package:dwds/src/servers/extension_debugger.dart';
 import 'package:dwds/src/services/app_debug_services.dart';
 import 'package:dwds/src/services/debug_service.dart';
 import 'package:dwds/src/services/expression_compiler.dart';
@@ -473,109 +474,123 @@ class DevHandler {
   }
 
   Future<void> _listenForDebugExtension() async {
-    while (await _extensionBackend!.connections.hasNext) {
-      await _startExtensionDebugService();
+    final extensionBackend = _extensionBackend;
+    if (extensionBackend == null) {
+      _logger.severe('No debug extension backend. Debugging will not work.');
+      return;
+    }
+
+    while (await extensionBackend.connections.hasNext) {
+      final extensionDebugger = await extensionBackend.extensionDebugger;
+      await _startExtensionDebugService(extensionDebugger);
     }
   }
 
   /// Starts a [DebugService] for Dart Debug Extension.
-  Future<void> _startExtensionDebugService() async {
-    final extensionDebugger = await _extensionBackend!.extensionDebugger;
+  Future<void> _startExtensionDebugService(
+      ExtensionDebugger extensionDebugger) async {
     // Waits for a `DevToolsRequest` to be sent from the extension background
     // when the extension is clicked.
     extensionDebugger.devToolsRequestStream.listen((devToolsRequest) async {
-      // TODO(grouma) - Ideally we surface those warnings to the extension so
-      // that it can be displayed to the user through an alert.
-      final tabUrl = devToolsRequest.tabUrl;
-      final appId = devToolsRequest.appId;
-      if (tabUrl == null) {
-        _logger.warning('Failed to start extension debug service. '
-            'Missing tab url in DevTools request for app with id: $appId');
-        return;
-      }
-      final connection = _appConnectionByAppId[appId];
-      if (connection == null) {
-        _logger.warning('Failed to start extension debug service. '
-            'Not connected to an app with id: $appId');
-        return;
-      }
-      final executionContext = extensionDebugger.executionContext;
-      if (executionContext == null) {
-        _logger.warning('Failed to start extension debug service. '
-            'No execution context for app with id: $appId');
-        return;
+      try {
+        await _handleDevToolsRequest(extensionDebugger, devToolsRequest);
+      } catch (error) {
+        _logger.severe('Encountered error handling DevTools request.');
+        extensionDebugger.closeWithError(error);
       }
+    });
+  }
 
-      final debuggerStart = DateTime.now();
-      var appServices = _servicesByAppId[appId];
-      if (appServices == null) {
-        final debugService = await DebugService.start(
-          _hostname,
-          extensionDebugger,
-          executionContext,
-          basePathForServerUri(tabUrl),
-          _assetReader,
-          _loadStrategy,
-          connection,
-          _urlEncoder,
-          onResponse: (response) {
-            if (response['error'] == null) return;
-            _logger
-                .finest('VmService proxy responded with an error:\n$response');
-          },
-          useSse: _useSseForDebugProxy,
-          expressionCompiler: _expressionCompiler,
-          spawnDds: _spawnDds,
-        );
-        appServices = await _createAppDebugServices(
-          devToolsRequest.appId,
-          debugService,
-        );
-        final encodedUri = await debugService.encodedUri;
-        extensionDebugger.sendEvent('dwds.encodedUri', encodedUri);
-        safeUnawaited(appServices
-            .chromeProxyService.remoteDebugger.onClose.first
-            .whenComplete(() async {
-          appServices?.chromeProxyService.destroyIsolate();
-          await appServices?.close();
-          _servicesByAppId.remove(devToolsRequest.appId);
-          _logger.info('Stopped debug service on '
-              '${await appServices?.debugService.encodedUri}\n');
-        }));
-        extensionDebugConnections.add(DebugConnection(appServices));
-        _servicesByAppId[appId] = appServices;
-      }
-      // If we don't have a DevTools instance, then are connecting to an IDE.
-      // Therefore return early instead of opening DevTools:
-      if (_devTools == null) return;
-
-      final encodedUri = await appServices.debugService.encodedUri;
-
-      appServices.dwdsStats.updateLoadTime(
-          debuggerStart: debuggerStart, devToolsStart: DateTime.now());
-
-      // TODO(elliette): Remove handling requests from the MV2 extension after
-      // MV3 release.
-      // If we only want the URI, this means the Dart Debug Extension should
-      // handle how to open it. Therefore return early before opening a new
-      // tab or window:
-      if (devToolsRequest.uriOnly ?? false) {
-        final devToolsUri = _constructDevToolsUri(
-          encodedUri,
-          ideQueryParam: 'ChromeDevTools',
-        );
-        return extensionDebugger.sendEvent('dwds.devtoolsUri', devToolsUri);
-      }
+  Future<void> _handleDevToolsRequest(
+    ExtensionDebugger extensionDebugger,
+    DevToolsRequest devToolsRequest,
+  ) async {
+    // TODO(grouma) - Ideally we surface those warnings to the extension so
+    // that it can be displayed to the user through an alert.
+    final tabUrl = devToolsRequest.tabUrl;
+    final appId = devToolsRequest.appId;
+    if (tabUrl == null) {
+      throw StateError('Failed to start extension debug service. '
+          'Missing tab url in DevTools request for app with id: $appId');
+    }
+    final connection = _appConnectionByAppId[appId];
+    if (connection == null) {
+      throw StateError('Failed to start extension debug service. '
+          'Not connected to an app with id: $appId');
+    }
+    final executionContext = extensionDebugger.executionContext;
+    if (executionContext == null) {
+      throw StateError('Failed to start extension debug service. '
+          'No execution context for app with id: $appId');
+    }
 
-      // Otherwise, launch DevTools in a new tab / window:
-      await _launchDevTools(
+    final debuggerStart = DateTime.now();
+    var appServices = _servicesByAppId[appId];
+    if (appServices == null) {
+      final debugService = await DebugService.start(
+        _hostname,
         extensionDebugger,
-        _constructDevToolsUri(
-          encodedUri,
-          ideQueryParam: 'DebugExtension',
-        ),
+        executionContext,
+        basePathForServerUri(tabUrl),
+        _assetReader,
+        _loadStrategy,
+        connection,
+        _urlEncoder,
+        onResponse: (response) {
+          if (response['error'] == null) return;
+          _logger.finest('VmService proxy responded with an error:\n$response');
+        },
+        useSse: _useSseForDebugProxy,
+        expressionCompiler: _expressionCompiler,
+        spawnDds: _spawnDds,
       );
-    });
+      appServices = await _createAppDebugServices(
+        devToolsRequest.appId,
+        debugService,
+      );
+      final encodedUri = await debugService.encodedUri;
+      extensionDebugger.sendEvent('dwds.encodedUri', encodedUri);
+      safeUnawaited(appServices.chromeProxyService.remoteDebugger.onClose.first
+          .whenComplete(() async {
+        appServices?.chromeProxyService.destroyIsolate();
+        await appServices?.close();
+        _servicesByAppId.remove(devToolsRequest.appId);
+        _logger.info('Stopped debug service on '
+            '${await appServices?.debugService.encodedUri}\n');
+      }));
+      extensionDebugConnections.add(DebugConnection(appServices));
+      _servicesByAppId[appId] = appServices;
+    }
+    // If we don't have a DevTools instance, then are connecting to an IDE.
+    // Therefore return early instead of opening DevTools:
+    if (_devTools == null) return;
+
+    final encodedUri = await appServices.debugService.encodedUri;
+
+    appServices.dwdsStats.updateLoadTime(
+        debuggerStart: debuggerStart, devToolsStart: DateTime.now());
+
+    // TODO(elliette): Remove handling requests from the MV2 extension after
+    // MV3 release.
+    // If we only want the URI, this means the Dart Debug Extension should
+    // handle how to open it. Therefore return early before opening a new
+    // tab or window:
+    if (devToolsRequest.uriOnly ?? false) {
+      final devToolsUri = _constructDevToolsUri(
+        encodedUri,
+        ideQueryParam: 'ChromeDevTools',
+      );
+      return extensionDebugger.sendEvent('dwds.devtoolsUri', devToolsUri);
+    }
+
+    // Otherwise, launch DevTools in a new tab / window:
+    await _launchDevTools(
+      extensionDebugger,
+      _constructDevToolsUri(
+        encodedUri,
+        ideQueryParam: 'DebugExtension',
+      ),
+    );
   }
 
   DevTools _ensureDevTools() {

dwds/lib/src/servers/extension_debugger.dart

@@ -13,7 +13,11 @@ import 'package:dwds/src/debugging/execution_context.dart';
 import 'package:dwds/src/debugging/remote_debugger.dart';
 import 'package:dwds/src/handlers/socket_connections.dart';
 import 'package:dwds/src/services/chrome_debug_exception.dart';
-import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
+import 'package:logging/logging.dart';
+import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'
+    hide StackTrace;
+
+final _logger = Logger('ExtensionDebugger');
 
 /// A remote debugger backed by the Dart Debug Extension with an SSE connection.
 class ExtensionDebugger implements RemoteDebugger {
@@ -140,11 +144,23 @@ class ExtensionDebugger implements RemoteDebugger {
     final completer = Completer<WipResponse>();
     final id = newId();
     _completers[id] = completer;
-    sseConnection.sink
-        .add(jsonEncode(serializers.serialize(ExtensionRequest((b) => b
-          ..id = id
-          ..command = command
-          ..commandParams = jsonEncode(params ?? {})))));
+    try {
+      sseConnection.sink
+          .add(jsonEncode(serializers.serialize(ExtensionRequest((b) => b
+            ..id = id
+            ..command = command
+            ..commandParams = jsonEncode(params ?? {})))));
+    } on StateError catch (error, stackTrace) {
+      if (error.message.contains('Cannot add event after closing')) {
+        _logger.severe('Socket connection closed. Shutting down debugger.');
+        closeWithError(error);
+      } else {
+        _logger.severe('Bad state while sending $command.', error, stackTrace);
+      }
+    } catch (error, stackTrace) {
+      _logger.severe(
+          'Unknown error while sending $command.', error, stackTrace);
+    }
     return completer.future;
   }
 
@@ -161,6 +177,13 @@ class ExtensionDebugger implements RemoteDebugger {
         ]);
       }();
 
+  void closeWithError(Object? error) {
+    _logger.shout(
+        'Closing extension debugger due to error. Restart app for debugging functionality',
+        error);
+    close();
+  }
+
   @override
   Future disable() => sendCommand('Debugger.disable');
 

dwds/test/extension_debugger_test.dart

@@ -115,4 +115,38 @@ void main() async {
       expect(request, extensionRequest);
     });
   });
+  group('when closed', () {
+    test('DebugExtension.detached event closes the connection', () async {
+      final extensionEvent = ExtensionEvent((b) => b
+        ..method = jsonEncode('DebugExtension.detached')
+        ..params = jsonEncode({}));
+
+      connection.controllerIncoming.sink
+          .add(jsonEncode(serializers.serialize(extensionEvent)));
+      // Expect the connection to receive a close event:
+      expect(await extensionDebugger.onClose.first, isNotNull);
+    });
+
+    test(
+        'gracefully handles trying to send events after the connection is closed',
+        () async {
+      // Close the connection:
+      final extensionEvent = ExtensionEvent((b) => b
+        ..method = jsonEncode('DebugExtension.detached')
+        ..params = jsonEncode({}));
+      connection.controllerIncoming.sink
+          .add(jsonEncode(serializers.serialize(extensionEvent)));
+      // Wait for it to be closed:
+      await extensionDebugger.onClose.first;
+      // Try to send an event:
+      callToSendCommand() => extensionDebugger.sendCommand(
+            'Debugger.setBreakpoint',
+            params: {
+              'location': {'scriptId': '555', 'lineNumber': 28}
+            },
+          );
+      // Should not throw any errors:
+      expect(callToSendCommand, returnsNormally);
+    });
+  });
 }