Don't terminate Dart process pids from VM Service, record flutter_tools VM pid (#100223)

* Don't terminate Dart process pids from VM Service

These processes may be on another device, and in the case of attach the debugee should not be terminated anyway.

packages/flutter_tools/lib/src/debug_adapters/flutter_adapter.dart

@@ -191,15 +191,11 @@ class FlutterDebugAdapter extends DartDebugAdapter<FlutterLaunchRequestArguments
 
   @override
   Future<void> debuggerConnected(vm.VM vmInfo) async {
-    // Capture the PID from the VM Service so that we can terminate it when
-    // cleaning up. Terminating the process might not be enough as it could be
-    // just a shell script (e.g. flutter.bat on Windows) and may not pass the
-    // signal on correctly.
-    // See: https://github.com/Dart-Code/Dart-Code/issues/907
-    final int? pid = vmInfo.pid;
-    if (pid != null) {
-      pidsToTerminate.add(pid);
-    }
+    // Usually we'd capture the pid from the VM here and record it for
+    // terminating, however for Flutter apps it may be running on a remote
+    // device so it's not valid to terminate a process with that pid locally.
+    // For attach, pids should never be collected as terminateRequest() should
+    // not terminate the debugee.
   }
 
   /// Called by [disconnectRequest] to request that we forcefully shut down the app being run (or in the case of an attach, disconnect).
@@ -407,6 +403,17 @@ class FlutterDebugAdapter extends DartDebugAdapter<FlutterLaunchRequestArguments
     _connectDebuggerIfReady();
   }
 
+  /// Handles the daemon.connected event, recording the pid of the flutter_tools process.
+  void _handleDaemonConnected(Map<String, Object?> params) {
+    // On Windows, the pid from the process we spawn is the shell running
+    // flutter.bat and terminating it may not be reliable, so we also take the
+    // pid provided from the VM running flutter_tools.
+    final int? pid = params['pid'] as int?;
+    if (pid != null) {
+      pidsToTerminate.add(pid);
+    }
+  }
+
   /// Handles the app.debugPort event from Flutter, connecting to the VM Service if everything else is ready.
   void _handleDebugPort(Map<String, Object?> params) {
     // When running in noDebug mode, Flutter may still provide us a VM Service
@@ -434,6 +441,9 @@ class FlutterDebugAdapter extends DartDebugAdapter<FlutterLaunchRequestArguments
   void _handleJsonEvent(String event, Map<String, Object?>? params) {
     params ??= <String, Object?>{};
     switch (event) {
+      case 'daemon.connected':
+        _handleDaemonConnected(params);
+        break;
       case 'app.debugPort':
         _handleDebugPort(params);
         break;

packages/flutter_tools/test/general.shard/dap/flutter_adapter_test.dart

@@ -7,7 +7,9 @@ import 'dart:async';
 import 'package:flutter_tools/src/cache.dart';
 import 'package:flutter_tools/src/debug_adapters/flutter_adapter_args.dart';
 import 'package:flutter_tools/src/globals.dart' as globals;
+import 'package:test/fake.dart';
 import 'package:test/test.dart';
+import 'package:vm_service/vm_service.dart';
 
 import 'mocks.dart';
 
@@ -23,6 +25,84 @@ void main() {
           : '/fake/flutter';
     });
 
+
+  group('launchRequest', () {
+    test('runs "flutter run" with --machine', () async {
+      final MockFlutterDebugAdapter adapter = MockFlutterDebugAdapter(fileSystem: globals.fs, platform: globals.platform);
+        final Completer<void> responseCompleter = Completer<void>();
+
+        final FlutterLaunchRequestArguments args = FlutterLaunchRequestArguments(
+          cwd: '/project',
+          program: 'foo.dart',
+        );
+
+        await adapter.configurationDoneRequest(MockRequest(), null, () {});
+        await adapter.launchRequest(MockRequest(), args, responseCompleter.complete);
+        await responseCompleter.future;
+
+        expect(adapter.processArgs, containsAllInOrder(<String>['run', '--machine']));
+      });
+
+      test('does not record the VMs PID for terminating', () async {
+        final MockFlutterDebugAdapter adapter = MockFlutterDebugAdapter(fileSystem: globals.fs, platform: globals.platform);
+        final Completer<void> responseCompleter = Completer<void>();
+
+        final FlutterLaunchRequestArguments args = FlutterLaunchRequestArguments(
+          cwd: '/project',
+          program: 'foo.dart',
+        );
+
+        await adapter.configurationDoneRequest(MockRequest(), null, () {});
+        await adapter.launchRequest(MockRequest(), args, responseCompleter.complete);
+        await responseCompleter.future;
+
+        // Trigger a fake debuggerConnected with a pid that we expect the
+        // adapter _not_ to record, because it may be on another device.
+        await adapter.debuggerConnected(_FakeVm(pid: 123));
+
+        // Ensure the VM's pid was not recorded.
+        expect(adapter.pidsToTerminate, isNot(contains(123)));
+      });
+    });
+
+
+  group('attachRequest', () {
+    test('runs "flutter attach" with --machine', () async {
+      final MockFlutterDebugAdapter adapter = MockFlutterDebugAdapter(fileSystem: globals.fs, platform: globals.platform);
+        final Completer<void> responseCompleter = Completer<void>();
+
+        final FlutterAttachRequestArguments args = FlutterAttachRequestArguments(
+          cwd: '/project',
+        );
+
+        await adapter.configurationDoneRequest(MockRequest(), null, () {});
+        await adapter.attachRequest(MockRequest(), args, responseCompleter.complete);
+        await responseCompleter.future;
+
+        expect(adapter.processArgs, containsAllInOrder(<String>['attach', '--machine']));
+      });
+
+      test('does not record the VMs PID for terminating', () async {
+        final MockFlutterDebugAdapter adapter = MockFlutterDebugAdapter(fileSystem: globals.fs, platform: globals.platform);
+        final Completer<void> responseCompleter = Completer<void>();
+
+        final FlutterAttachRequestArguments args = FlutterAttachRequestArguments(
+          cwd: '/project',
+        );
+
+        await adapter.configurationDoneRequest(MockRequest(), null, () {});
+        await adapter.attachRequest(MockRequest(), args, responseCompleter.complete);
+        await responseCompleter.future;
+
+        // Trigger a fake debuggerConnected with a pid that we expect the
+        // adapter _not_ to record, because it may be on another device.
+        await adapter.debuggerConnected(_FakeVm(pid: 123));
+
+        // Ensure the VM's pid was not recorded.
+        expect(adapter.pidsToTerminate, isNot(contains(123)));
+      });
+    });
+
     group('--start-paused', () {
       test('is passed for debug mode', () async {
         final MockFlutterDebugAdapter adapter = MockFlutterDebugAdapter(fileSystem: globals.fs, platform: globals.platform);
@@ -156,3 +236,10 @@ void main() {
     });
   });
 }
+
+class _FakeVm extends Fake implements VM {
+  _FakeVm({this.pid = 1});
+
+  @override
+  final int pid;
+}