Merge pull request #1293 from microsoft/chore/1283

feat: support new canBeRestarted flag from the runtime

Author: connor4312

CHANGELOG.md

@@ -6,6 +6,7 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
 
 - fix: debugged child processes in ext host causing teardown ([#1289](https://github.com/microsoft/vscode-js-debug/issues/1289))
 - fix: errors thrown in process tree lookup not being visible ([vscode#150754](https://github.com/microsoft/vscode/issues/150754))
+- chore: adopt new restartFrame semantics from Chrome 104 ([#1283](https://github.com/microsoft/vscode-js-debug/issues/1283))
 
 ## v1.68 (May 2022)
 

src/adapter/console/textualMessage.ts

@@ -11,7 +11,7 @@ import { formatMessage } from '../messageFormat';
 import { messageFormatters, previewAsObject } from '../objectPreview';
 import { AnyObject } from '../objectPreview/betterTypes';
 import { IUiLocation } from '../sources';
-import { StackTrace } from '../stackTrace';
+import { StackFrame, StackTrace } from '../stackTrace';
 import { Thread } from '../threads';
 import { IConsoleMessage } from './consoleMessage';
 
@@ -41,8 +41,12 @@ export abstract class TextualMessage<T extends { stackTrace?: Cdp.Runtime.StackT
     }
 
     let firstExistingLocation: IUiLocation | undefined;
-    for (let i = 0; i < stackTrace.frames.length; i++) {
-      const uiLocation = await stackTrace.frames[i].uiLocation();
+    for (const frame of stackTrace.frames) {
+      if (!(frame instanceof StackFrame)) {
+        continue;
+      }
+
+      const uiLocation = await frame.uiLocation();
       if (!uiLocation) {
         continue;
       }

src/adapter/smartStepping.ts

@@ -4,6 +4,7 @@
 
 import { inject, injectable } from 'inversify';
 import { ILogger, LogTag } from '../common/logging';
+import { isInstanceOf } from '../common/objUtils';
 import { AnyLaunchConfiguration } from '../configuration';
 import { isSourceWithMap, UnmappedReason } from './sources';
 import { StackFrame } from './stackTrace';
@@ -78,8 +79,8 @@ export class SmartStepper {
       return;
     }
 
-    const frame = (await pausedDetails.stackTrace.loadFrames(1))[0];
-    const should = await shouldStepOverStackFrame(frame);
+    const frame = (await pausedDetails.stackTrace.loadFrames(1)).find(isInstanceOf(StackFrame));
+    const should = frame && (await shouldStepOverStackFrame(frame));
     if (should === StackFrameStepOverReason.NotStepped) {
       this.resetSmartStepCount();
       return;

src/adapter/stackTrace.ts

@@ -16,17 +16,28 @@ import { IExtraProperty, IScopeRef, IVariableContainer } from './variableStore';
 
 const localize = nls.loadMessageBundle();
 
+export interface IFrameElement {
+  /** Formats the stack element as V8 would format it */
+  formatAsNative(): Promise<string>;
+  /** Pretty formats the stack element as text */
+  format(): Promise<string>;
+  /** Formats the element for DAP */
+  toDap(format?: Dap.StackFrameFormat): Promise<Dap.StackFrame>;
+}
+
+type FrameElement = StackFrame | AsyncSeparator;
+
 export class StackTrace {
-  public readonly frames: StackFrame[] = [];
+  public readonly frames: FrameElement[] = [];
   private _frameById: Map<number, StackFrame> = new Map();
   private _asyncStackTraceId?: Cdp.Runtime.StackTraceId;
   private _lastFrameThread?: Thread;
 
   public static fromRuntime(thread: Thread, stack: Cdp.Runtime.StackTrace): StackTrace {
     const result = new StackTrace(thread);
-    for (let frameNo = 0; frameNo < stack.callFrames.length; frameNo++) {
-      if (!stack.callFrames[frameNo].url.endsWith(SourceConstants.InternalExtension)) {
-        result.frames.push(StackFrame.fromRuntime(thread, stack.callFrames[frameNo], false));
+    for (const frame of stack.callFrames) {
+      if (!frame.url.endsWith(SourceConstants.InternalExtension)) {
+        result.frames.push(StackFrame.fromRuntime(thread, frame, false));
       }
     }
 
@@ -88,7 +99,7 @@ export class StackTrace {
     this._lastFrameThread = thread;
   }
 
-  async loadFrames(limit: number): Promise<StackFrame[]> {
+  async loadFrames(limit: number): Promise<FrameElement[]> {
     while (this.frames.length < limit && this._asyncStackTraceId) {
       if (this._asyncStackTraceId.debuggerId)
         this._lastFrameThread = Thread.threadForDebuggerId(this._asyncStackTraceId.debuggerId);
@@ -117,9 +128,10 @@ export class StackTrace {
         stackTrace.callFrames.shift();
 
       if (stackTrace.callFrames.length) {
-        this._appendFrame(StackFrame.asyncSeparator(thread, stackTrace.description || 'async'));
-        for (const callFrame of stackTrace.callFrames)
+        this._appendFrame(new AsyncSeparator(stackTrace.description || 'async'));
+        for (const callFrame of stackTrace.callFrames) {
           this._appendFrame(StackFrame.fromRuntime(thread, callFrame, true));
+        }
       }
 
       if (stackTrace.parentId) {
@@ -131,9 +143,11 @@ export class StackTrace {
     }
   }
 
-  _appendFrame(frame: StackFrame) {
+  _appendFrame(frame: FrameElement) {
     this.frames.push(frame);
-    this._frameById.set(frame._id, frame);
+    if (frame instanceof StackFrame) {
+      this._frameById.set(frame._id, frame);
+    }
   }
 
   async formatAsNative(): Promise<string> {
@@ -174,7 +188,23 @@ interface IScope {
   callFrameId: string;
 }
 
-export class StackFrame {
+export class AsyncSeparator implements IFrameElement {
+  constructor(private readonly label = 'async') {}
+
+  public async toDap(): Promise<Dap.StackFrame> {
+    return { name: this.label, id: 0, line: 0, column: 0, presentationHint: 'label' };
+  }
+
+  public async formatAsNative(): Promise<string> {
+    return `    --- ${this.label} ---`;
+  }
+
+  public async format(): Promise<string> {
+    return `◀ ${this.label} ▶`;
+  }
+}
+
+export class StackFrame implements IFrameElement {
   private static _lastFrameId = 0;
 
   _id: number;
@@ -184,7 +214,6 @@ export class StackFrame {
     | Promise<IPreferredUiLocation | undefined>
     | IPreferredUiLocation
     | undefined;
-  private _isAsyncSeparator = false;
   private _scope: IScope | undefined;
   private _thread: Thread;
 
@@ -198,11 +227,11 @@ export class StackFrame {
     callFrame: Cdp.Runtime.CallFrame,
     isAsync: boolean,
   ): StackFrame {
-    return new StackFrame(thread, callFrame.functionName, thread.rawLocation(callFrame), isAsync);
+    return new StackFrame(thread, callFrame, thread.rawLocation(callFrame), isAsync);
   }
 
   static fromDebugger(thread: Thread, callFrame: Cdp.Debugger.CallFrame): StackFrame {
-    const result = new StackFrame(thread, callFrame.functionName, thread.rawLocation(callFrame));
+    const result = new StackFrame(thread, callFrame, thread.rawLocation(callFrame));
     result._scope = {
       chain: callFrame.scopeChain,
       thisObject: callFrame.this,
@@ -214,37 +243,32 @@ export class StackFrame {
     return result;
   }
 
-  static asyncSeparator(thread: Thread, name: string): StackFrame {
-    // todo@connor4312: this should probably be a different type of object
-    const result = new StackFrame(
-      thread,
-      name,
-      { lineNumber: 1, columnNumber: 1, scriptId: '' },
-      true,
-    );
-    result._isAsyncSeparator = true;
-    return result;
-  }
-
   constructor(
     thread: Thread,
-    name: string,
+    private readonly callFrame: Cdp.Debugger.CallFrame | Cdp.Runtime.CallFrame,
     rawLocation: RawLocation,
     private readonly isAsync = false,
   ) {
     this._id = ++StackFrame._lastFrameId;
-    this._name = name || '<anonymous>';
+    this._name = callFrame.functionName || '<anonymous>';
     this._rawLocation = rawLocation;
     this.uiLocation = once(() => thread.rawLocationToUiLocation(rawLocation));
     this._thread = thread;
   }
 
+  /**
+   * Gets whether the runtime explicitly said this frame can be restarted.
+   */
+  public get canExplicitlyBeRestarted() {
+    return !!(this.callFrame as Cdp.Debugger.CallFrame).canBeRestarted;
+  }
+
   /**
    * Gets whether this stackframe is at the same position as the other frame.
    */
-  public equivalentTo(other: StackFrame | undefined) {
+  public equivalentTo(other: unknown) {
     return (
-      other &&
+      other instanceof StackFrame &&
       other._rawLocation.columnNumber === this._rawLocation.columnNumber &&
       other._rawLocation.lineNumber === this._rawLocation.lineNumber &&
       other._rawLocation.scriptId === this._rawLocation.scriptId
@@ -336,15 +360,12 @@ export class StackFrame {
     return { scopes };
   }
 
+  /** @inheritdoc */
   async toDap(format?: Dap.StackFrameFormat): Promise<Dap.StackFrame> {
     const uiLocation = await this.uiLocation();
     const source = uiLocation ? await uiLocation.source.toDap() : undefined;
     const isSmartStepped = await shouldStepOverStackFrame(this);
-    const presentationHint = this._isAsyncSeparator
-      ? 'label'
-      : isSmartStepped
-      ? 'deemphasize'
-      : 'normal';
+    const presentationHint = isSmartStepped ? 'deemphasize' : 'normal';
     if (isSmartStepped && source) {
       source.origin =
         isSmartStepped === StackFrameStepOverReason.SmartStep
@@ -374,12 +395,14 @@ export class StackFrame {
       column,
       source,
       presentationHint,
-      canRestart: !this.isAsync,
+      // If `canBeRestarted` is present, use that
+      // https://github.com/microsoft/vscode-js-debug/issues/1283
+      canRestart: (this.callFrame as Cdp.Debugger.CallFrame).canBeRestarted ?? !this.isAsync,
     } as Dap.StackFrame;
   }
 
+  /** @inheritdoc */
   async formatAsNative(): Promise<string> {
-    if (this._isAsyncSeparator) return `    --- ${this._name} ---`;
     const uiLocation = await this.uiLocation();
     const url =
       (await uiLocation?.source.existingAbsolutePath()) ||
@@ -389,8 +412,8 @@ export class StackFrame {
     return `    at ${this._name} (${url}:${lineNumber}:${columnNumber})`;
   }
 
+  /** @inheritdoc */
   async format(): Promise<string> {
-    if (this._isAsyncSeparator) return `◀ ${this._name} ▶`;
     const uiLocation = await this.uiLocation();
     const prettyName = (await uiLocation?.source.prettyName()) || '<unknown>';
     const anyLocation = uiLocation || this._rawLocation;

src/adapter/threads.ts

@@ -8,6 +8,7 @@ import { DebugType } from '../common/contributionUtils';
 import { EventEmitter } from '../common/events';
 import { HrTime } from '../common/hrnow';
 import { ILogger, LogTag } from '../common/logging';
+import { isInstanceOf } from '../common/objUtils';
 import { Base1Position } from '../common/positions';
 import { delay, getDeferred, IDeferred } from '../common/promiseUtil';
 import { IRenameProvider } from '../common/sourceMaps/renameProvider';
@@ -303,12 +304,32 @@ export class Thread implements IVariableStoreLocationProvider {
       );
     }
 
-    await this._cdp.Debugger.restartFrame({ callFrameId });
+    // Cast is necessary since the devtools-protocol is being slow to update:
+    // https://github.com/microsoft/vscode-js-debug/issues/1283#issuecomment-1148219994
+    // https://github.com/ChromeDevTools/devtools-protocol/issues/263
+    const ok = await this._cdp.Debugger.restartFrame({
+      callFrameId,
+      mode: 'StepInto',
+    } as Cdp.Debugger.RestartFrameParams);
+    if (!ok) {
+      return errors.createUserError(
+        localize('error.unknownRestartError', 'Frame could not be restarted'),
+      );
+    }
+
     this._expectedPauseReason = {
       reason: 'frame_entry',
       description: localize('reason.description.restart', 'Paused on frame entry'),
     };
-    await this._cdp.Debugger.stepInto({});
+
+    // Chromium versions before 104 didn't have an explicit `canBeRestarted`
+    // flag on their call frame. And on those versions, when we `restartFrame`,
+    // we need to manually `stepInto` to unpause. However, with 104, restarting
+    // the frame will automatically resume execution.
+    if (!stackFrame.canExplicitlyBeRestarted) {
+      await this._cdp.Debugger.stepInto({});
+    }
+
     return {};
   }
 
@@ -770,7 +791,8 @@ export class Thread implements IVariableStoreLocationProvider {
       return false;
     }
 
-    const first = await trace.frames[0]?.uiLocation();
+    const firstFrame = trace.frames[0];
+    const first = firstFrame instanceof StackFrame && (await firstFrame.uiLocation());
     if (!first) {
       return false;
     }
@@ -792,9 +814,14 @@ export class Thread implements IVariableStoreLocationProvider {
       if (!stackLocations) {
         // for some reason, if this is assigned directly to stackLocations,
         // then TS will think it can still be undefined below
-        const x = await trace
-          .loadFrames(excludedCallerSearchDepth)
-          .then(frames => Promise.all(frames.slice(1).map(f => f.uiLocation())));
+        const x = await trace.loadFrames(excludedCallerSearchDepth).then(frames =>
+          Promise.all(
+            frames
+              .slice(1)
+              .filter(isInstanceOf(StackFrame))
+              .map(f => f.uiLocation()),
+          ),
+        );
         stackLocations = x;
       }
 
@@ -1431,7 +1458,7 @@ export class Thread implements IVariableStoreLocationProvider {
         Promise.all(
           details.hitBreakpoints
             .map(bp => this._breakpointManager._resolvedBreakpoints.get(bp))
-            .filter((bp): bp is UserDefinedBreakpoint => bp instanceof UserDefinedBreakpoint)
+            .filter(isInstanceOf(UserDefinedBreakpoint))
             .map(r => r.untilSetCompleted().then(() => r.dapId)),
         ),
       ]);