[DAP] Set requirePermissionToResume and requireUserPermissionToResume for onPauseStart and onPauseExit so that DDS waits for DAP's permission before resuming the isolate.

Bug: #54843
Change-Id: I12686d6c683983b7af5a3148681410ff9c684343
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350649
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Elliott Brooks <[email protected]>

Author: elliette

pkg/dds/CHANGELOG.md

@@ -2,6 +2,7 @@
 - [DAP]: Fixed an issue where breakpoint `changed` events might contain incorrect location information when new isolates are created, causing breakpoints to appear to move in the editor.
 - [DAP]: For consistency with other values, automatic `toString()` invocations for debugger views no longer expand long strings and instead show truncated values. Full values continue to be returned for evaluation (`context=="repl"`) and when copying to the clipboard (`context=="clipboard"`).
 - [DAP]: Improved handling of sentinel responses when building `variables` responses. This prevents entire map/list requests from failing when only some values inside are sentinels.
+- [DAP] Set `requirePermissionToResume` and `requireUserPermissionToResume` for `onPauseStart` and `onPauseExit` so that DDS waits for DAP's permission before resuming the isolate.
 
 # 4.2.0
 - [DAP] All `OutputEvent`s are now scanned for stack frames to attach `source` metadata to. The [parseStackFrames] parameter for `sendOutput` is ignored and deprecated.

pkg/dds/lib/src/dap/adapters/dart.dart

@@ -8,6 +8,7 @@ import 'dart:io';
 
 import 'package:collection/collection.dart';
 import 'package:dap/dap.dart';
+import 'package:dds_service_extensions/dds_service_extensions.dart';
 import 'package:json_rpc_2/error_code.dart' as json_rpc_errors;
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
@@ -434,12 +435,19 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// vs 'detached').
   bool isDetaching = false;
 
-  /// Whether isolates that pause in the PauseExit state should be automatically
-  /// resumed after any in-process log events have completed.
+  /// Whether this adapter set the --pause-isolates-on-start flag, specifying
+  /// that isolates should pause on starting.
   ///
   /// Normally this will be true, but it may be set to false if the user
-  /// also manually passes pause-isolates-on-exit.
-  bool resumeIsolatesAfterPauseExit = true;
+  /// also manually passed the --pause-isolates-on-start flag.
+  bool pauseIsolatesOnStartSetByDap = true;
+
+  /// Whether this adapter set the --pause-isolates-on-exit flag, specifying
+  /// that isolates should pause on exiting.
+  ///
+  /// Normally this will be true, but it may be set to false if the user
+  /// also manually passed the --pause-isolates-on-exit flag.
+  bool pauseIsolatesOnExitSetByDap = true;
 
   /// A [Future] that completes when the last queued OutputEvent has been sent.
   ///
@@ -559,10 +567,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     isAttach = true;
     _subscribeToOutputStreams = true;
 
-    // When attaching to a process, suppress auto-resuming isolates until the
-    // first time the user resumes anything.
-    isolateManager.autoResumeStartingIsolates = false;
-
     // Common setup.
     await _prepareForLaunchOrAttach(null);
 
@@ -702,6 +706,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     // Let the subclass do any existing setup once we have a connection.
     await debuggerConnected(vmInfo);
 
+    await _configureIsolateSettings(vmService);
     await _withErrorHandling(
       () => _configureExistingIsolates(vmService, vmInfo),
     );
@@ -756,6 +761,68 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     return DapProgressReporter.start(this, id, title, message: message);
   }
 
+  Future<void> _configureIsolateSettings(
+    vm.VmService vmService,
+  ) async {
+    // If this is an attach workflow, check whether pause_isolates_on_start or
+    // pause_isolates_on_exit were already set, and if not set them (note: this
+    // is already done as part of the launch workflow):
+    if (isAttach) {
+      const pauseIsolatesOnStart = 'pause_isolates_on_start';
+      const pauseIsolatesOnExit = 'pause_isolates_on_exit';
+      final flags = (await vmService.getFlagList()).flags ?? <vm.Flag>[];
+      for (final flag in flags) {
+        final flagName = flag.name;
+        final isPauseIsolatesFlag =
+            flagName == pauseIsolatesOnStart || flagName == pauseIsolatesOnExit;
+        if (flagName == null || !isPauseIsolatesFlag) continue;
+
+        if (flag.valueAsString == 'true') {
+          if (flagName == pauseIsolatesOnStart) {
+            pauseIsolatesOnStartSetByDap = false;
+          }
+          if (flagName == pauseIsolatesOnExit) {
+            pauseIsolatesOnExitSetByDap = false;
+          }
+        } else {
+          _setVmFlagTo(vmService, flagName: flagName, valueAsString: 'true');
+        }
+      }
+    }
+
+    try {
+      // Make sure DDS waits for DAP to be ready to resume before forwarding
+      // resume requests to the VM Service:
+      await vmService.requirePermissionToResume(
+        onPauseStart: true,
+        onPauseExit: true,
+      );
+
+      // Specify whether DDS should wait for a user-initiated resume as well as a
+      // DAP-initiated resume:
+      await vmService.requireUserPermissionToResume(
+        onPauseStart: !pauseIsolatesOnStartSetByDap,
+        onPauseExit: !pauseIsolatesOnExitSetByDap,
+      );
+    } catch (e) {
+      // If DDS is not enabled, calling these DDS service extensions will fail.
+      // Therefore catch and log any errors.
+      logger?.call('Failure configuring isolate settings: $e');
+    }
+  }
+
+  Future<void> _setVmFlagTo(
+    vm.VmService vmService, {
+    required String flagName,
+    required String valueAsString,
+  }) async {
+    try {
+      await vmService.setFlag(flagName, valueAsString);
+    } catch (e) {
+      logger?.call('Failed to to set VM flag $flagName to $valueAsString: $e');
+    }
+  }
+
   /// Process any existing isolates that may have been created before the
   /// streams above were set up.
   Future<void> _configureExistingIsolates(
@@ -775,8 +842,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
       final pauseEventKind = isolate.runnable ?? false
           ? vm.EventKind.kIsolateRunnable
           : vm.EventKind.kIsolateStart;
-      final thread =
-          await isolateManager.registerIsolate(isolate, pauseEventKind);
+      await isolateManager.registerIsolate(isolate, pauseEventKind);
 
       // If the Isolate already has a Pause event we can give it to the
       // IsolateManager to handle (if it's PausePostStart it will re-configure
@@ -788,13 +854,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           isolate.pauseEvent!,
         );
       } else if (isolate.runnable == true) {
-        // If requested, automatically resume. Otherwise send a Stopped event to
-        // inform the client UI the thread is paused.
-        if (isolateManager.autoResumeStartingIsolates) {
-          await isolateManager.resumeIsolate(isolate);
-        } else {
-          isolateManager.sendStoppedOnEntryEvent(thread.threadId);
-        }
+        await isolateManager.readyToResumeIsolate(isolate);
       }
     }));
   }
@@ -2352,11 +2412,9 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     // We pause isolates on exit to allow requests for resolving URIs in
     // stderr call stacks, so when we see an isolate pause, wait for any
     // pending logs and then resume it (so it exits).
-    if (resumeIsolatesAfterPauseExit &&
-        eventKind == vm.EventKind.kPauseExit &&
-        isolate != null) {
+    if (eventKind == vm.EventKind.kPauseExit && isolate != null) {
       await _waitForPendingOutputEvents();
-      await isolateManager.resumeIsolate(isolate);
+      await isolateManager.readyToResumeIsolate(isolate);
     }
   }
 

pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart

@@ -11,6 +11,7 @@ import 'package:dap/dap.dart';
 import 'package:path/path.dart' as path;
 import 'package:vm_service/vm_service.dart' as vm;
 
+import '../utils.dart';
 import 'dart.dart';
 import 'mixins.dart';
 
@@ -68,14 +69,6 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     terminatePids(ProcessSignal.sigkill);
   }
 
-  /// Checks whether [flag] is in [args], allowing for both underscore and
-  /// dash format.
-  bool _containsVmFlag(List<String> args, String flag) {
-    final flagUnderscores = flag.replaceAll('-', '_');
-    final flagDashes = flag.replaceAll('_', '-');
-    return args.contains(flagUnderscores) || args.contains(flagDashes);
-  }
-
   @override
   Future<void> launchImpl() {
     throw UnsupportedError(
@@ -115,7 +108,6 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
       ...?args.vmAdditionalArgs,
       if (debug) ...[
         '--enable-vm-service=${args.vmServicePort ?? 0}${ipv6 ? '/::1' : ''}',
-        '--pause_isolates_on_start',
         if (!enableAuthCodes) '--disable-service-auth-codes'
       ],
       '--disable-dart-dev',
@@ -128,12 +120,20 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     final toolArgs = args.toolArgs ?? [];
     if (debug) {
       // If the user has explicitly set pause-isolates-on-exit we need to
-      // not add it ourselves, and disable auto-resuming.
-      if (_containsVmFlag(toolArgs, '--pause_isolates_on_exit')) {
-        resumeIsolatesAfterPauseExit = false;
+      // not add it ourselves, and specify that we didn't set it.
+      if (containsVmFlag(toolArgs, '--pause_isolates_on_exit')) {
+        pauseIsolatesOnExitSetByDap = false;
       } else {
         vmArgs.add('--pause_isolates_on_exit');
       }
+
+      // If the user has explicitly set pause-isolates-on-start we need to
+      // not add it ourselves, and specify that we didn't set it.
+      if (containsVmFlag(toolArgs, '--pause_isolates_on_start')) {
+        pauseIsolatesOnStartSetByDap = false;
+      } else {
+        vmArgs.add('--pause_isolates_on_start');
+      }
     }
 
     // Handle customTool and deletion of any arguments for it.

pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart

@@ -10,6 +10,7 @@ import 'dart:math' as math;
 import 'package:vm_service/vm_service.dart' as vm;
 
 import '../stream_transformers.dart';
+import '../utils.dart';
 import 'dart.dart';
 import 'mixins.dart';
 
@@ -78,17 +79,32 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
           .then((uri) => connectDebugger(uri)));
     }
 
-    final vmArgs = <String>[
+    var vmArgs = <String>[
       ...?args.vmAdditionalArgs,
       if (debug) ...[
         '--enable-vm-service=${args.vmServicePort ?? 0}${ipv6 ? '/::1' : ''}',
-        '--pause_isolates_on_start',
         if (!enableAuthCodes) '--disable-service-auth-codes'
       ],
       if (debug && vmServiceInfoFile != null) ...[
         '-DSILENT_VM_SERVICE=true',
         '--write-service-info=${Uri.file(vmServiceInfoFile.path)}'
       ],
+    ];
+
+    final toolArgs = args.toolArgs ?? [];
+    if (debug) {
+      // If the user has explicitly set pause-isolates-on-start we need to
+      // not add it ourselves, and specify that we didn't set it.
+      if (containsVmFlag(toolArgs, '--pause_isolates_on_start') ||
+          containsVmFlag(vmArgs, '--pause_isolates_on_start')) {
+        pauseIsolatesOnStartSetByDap = false;
+      } else {
+        vmArgs.add('--pause_isolates_on_start');
+      }
+    }
+
+    vmArgs = [
+      ...vmArgs,
       // TODO(dantup): This should be changed from "dart run test:test" to
       // "dart test" once the start-paused flags are working correctly.
       // Currently they start paused but do not write the vm-service-info file