refactor: throw correct errors, avoid showing duplicate messages on l…

…aunch failure

src/binder.ts

@@ -6,7 +6,7 @@ import { CancellationToken } from 'vscode';
 import * as nls from 'vscode-nls';
 import { DebugAdapter } from './adapter/debugAdapter';
 import { Thread } from './adapter/threads';
-import { CancellationTokenSource, TaskCancelledError } from './common/cancellation';
+import { CancellationTokenSource } from './common/cancellation';
 import { IDisposable, EventEmitter } from './common/events';
 import { LogTag, ILogger, resolveLoggerOptions } from './common/logging';
 import * as urlUtils from './common/urlUtils';
@@ -203,11 +203,17 @@ export class Binder implements IDisposable {
 
     if (params.rootPath) params.rootPath = urlUtils.platformPathToPreferredCase(params.rootPath);
     this._launchParams = params;
-    let results = await Promise.all(
-      [...this.getLaunchers()].map(l => this._launch(l, params, cts.token)),
-    );
-    results = results.filter(result => !!result);
-    if (results.length) return errors.createUserError(results.join('\n'));
+
+    try {
+      await Promise.all([...this.getLaunchers()].map(l => this._launch(l, params, cts.token)));
+    } catch (e) {
+      if (e instanceof errors.ProtocolError) {
+        e.cause.showUser = false; // avoid duplicate error messages in the UI
+      }
+
+      throw e;
+    }
+
     return {};
   }
 
@@ -255,12 +261,8 @@ export class Binder implements IDisposable {
     launcher: ILauncher,
     params: AnyLaunchConfiguration,
     cancellationToken: CancellationToken,
-  ): Promise<string | undefined> {
+  ): Promise<void> {
     const result = await this.captureLaunch(launcher, params, cancellationToken);
-    if (result.error) {
-      return result.error;
-    }
-
     if (!result.blockSessionTermination) {
       return;
     }
@@ -301,31 +303,16 @@ export class Binder implements IDisposable {
         dap: await this._dap,
       });
     } catch (e) {
-      if (e instanceof TaskCancelledError) {
-        result = {
-          error: localize('errors.timeout', '{0}: timeout after {1}ms', e.message, params.timeout),
-        };
-      }
-
-      result = { error: e.message };
-    }
-
-    if (result.error) {
-      // Assume it was precipiated from some timeout, if we got an error after cancellation
-      if (cancellationToken.isCancellationRequested) {
-        result.error = localize(
-          'errors.timeout',
-          '{0}: timeout after {1}ms',
-          result.error,
-          params.timeout,
-        );
-      }
-
       this._rootServices.get<ILogger>(ILogger).warn(LogTag.RuntimeLaunch, 'Launch returned error', {
-        error: result.error,
+        error: e,
+        wasCancelled: cancellationToken.isCancellationRequested,
         name,
       });
-    } else if (result.blockSessionTermination) {
+
+      throw e;
+    }
+
+    if (result.blockSessionTermination) {
       this._rootServices
         .get<ILogger>(ILogger)
         .info(LogTag.RuntimeLaunch, 'Launched successfully', { name });

src/dap/errors.ts

@@ -26,6 +26,9 @@ export const enum ErrorCodes {
   InvalidBreakpointCondition,
   ReplError,
   SourceMapParseFailed,
+  BrowserLaunchFailed,
+  TargetPageNotFound,
+  BrowserAttachFailed,
 }
 
 export function reportToConsole(dap: Dap.Api, error: string) {
@@ -177,6 +180,27 @@ export const browserNotFound = (
     ErrorCodes.BrowserNotFound,
   );
 
+export const browserLaunchFailed = (innerError: Error) =>
+  createUserError(
+    localize('error.browserLaunchError', 'Unable to launch browser: "{0}"', innerError.message),
+    ErrorCodes.BrowserLaunchFailed,
+  );
+
+export const browserAttachFailed = (message?: string) =>
+  createUserError(
+    message ?? localize('error.browserAttachError', 'Unable to attach to browser'),
+    ErrorCodes.BrowserAttachFailed,
+  );
+
+export const targetPageNotFound = () =>
+  createUserError(
+    localize(
+      'error.threadNotFound',
+      'Target page not found. You may need to update your "urlFilter" to match the page you want to debug.',
+    ),
+    ErrorCodes.TargetPageNotFound,
+  );
+
 export const invalidLogPointSyntax = (error: string) =>
   createUserError(error, ErrorCodes.InvalidLogPointBreakpointSyntax);
 

src/targets/browser/browserAttacher.ts

@@ -20,6 +20,7 @@ import { ILogger } from '../../common/logging';
 import { injectable, inject, optional } from 'inversify';
 import { ISourcePathResolver } from '../../common/sourcePathResolver';
 import { VSCodeApi } from '../../ioc-extras';
+import { browserAttachFailed, targetPageNotFound, ProtocolError } from '../../dap/errors';
 
 const localize = nls.loadMessageBundle();
 
@@ -58,8 +59,8 @@ export class BrowserAttacher implements ILauncher {
 
     this._lastLaunchParams = params;
 
-    const error = await this._attemptToAttach(params, context);
-    return error ? { error } : { blockSessionTermination: true };
+    await this._attemptToAttach(params, context);
+    return { blockSessionTermination: true };
   }
 
   _scheduleAttach(params: AnyChromiumAttachConfiguration, context: ILaunchContext) {
@@ -75,9 +76,6 @@ export class BrowserAttacher implements ILauncher {
       params,
       context.cancellationToken,
     );
-    if (typeof connection === 'string') {
-      return connection; // an error
-    }
 
     this._connection = connection;
     connection.onDisconnected(
@@ -128,14 +126,9 @@ export class BrowserAttacher implements ILauncher {
 
     const result = await Promise.race([
       targetManager.waitForMainTarget(await this.getTargetFilter(targetManager, params)),
-      delay(params.timeout).then(() =>
-        localize(
-          'chrome.attach.noMatchingTarget',
-          "Can't find a valid target that matches {0} within {1}ms",
-          params.urlFilter || params.url,
-          params.timeout,
-        ),
-      ),
+      delay(params.timeout).then(() => {
+        throw new ProtocolError(targetPageNotFound());
+      }),
     ]);
 
     return typeof result === 'string' ? result : undefined;
@@ -195,23 +188,31 @@ export class BrowserAttacher implements ILauncher {
         );
       } catch (e) {
         if (cancellationToken.isCancellationRequested) {
-          return localize(
-            'attach.cannotConnect',
-            'Cannot connect to the target at {0}: {1}',
-            browserURL,
-            e.message,
+          throw new ProtocolError(
+            browserAttachFailed(
+              localize(
+                'attach.cannotConnect',
+                'Cannot connect to the target at {0}: {1}',
+                browserURL,
+                e.message,
+              ),
+            ),
           );
         }
 
         await delay(1000);
       }
     }
 
-    return localize(
-      'attach.cannotConnect',
-      'Cannot connect to the target at {0}: {1}',
-      browserURL,
-      'Cancelled',
+    throw new ProtocolError(
+      browserAttachFailed(
+        localize(
+          'attach.cannotConnect',
+          'Cannot connect to the target at {0}: {1}',
+          browserURL,
+          'Cancelled',
+        ),
+      ),
     );
   }
 

src/targets/browser/browserLauncher.ts

@@ -5,7 +5,6 @@
 import * as fs from 'fs';
 import * as path from 'path';
 import { CancellationToken } from 'vscode';
-import * as nls from 'vscode-nls';
 import CdpConnection from '../../cdp/connection';
 import { timeoutPromise } from '../../common/cancellation';
 import { EnvironmentVars } from '../../common/environmentVars';
@@ -23,8 +22,12 @@ import { StoragePath, IInitializeParams } from '../../ioc-extras';
 import { Quality } from 'vscode-js-debug-browsers';
 import { DisposableList } from '../../common/disposable';
 import { ISourcePathResolver } from '../../common/sourcePathResolver';
-
-const localize = nls.loadMessageBundle();
+import {
+  browserLaunchFailed,
+  targetPageNotFound,
+  browserAttachFailed,
+  ProtocolError,
+} from '../../dap/errors';
 
 export interface IDapInitializeParamsWithExtensions extends Dap.InitializeParams {
   supportsLaunchUnelevatedProcessRequest?: boolean;
@@ -134,12 +137,12 @@ export abstract class BrowserLauncher<T extends AnyChromiumLaunchConfiguration>
   private async prepareLaunch(
     params: T,
     { dap, targetOrigin, cancellationToken, telemetryReporter }: ILaunchContext,
-  ): Promise<BrowserTarget | string> {
+  ): Promise<BrowserTarget> {
     let launched: launcher.ILaunchResult;
     try {
       launched = await this.launchBrowser(params, dap, cancellationToken, telemetryReporter);
     } catch (e) {
-      return localize('error.browserLaunchError', 'Unable to launch browser: "{0}"', e.message);
+      throw new ProtocolError(browserLaunchFailed(e));
     }
 
     this._disposables.push(launched.cdp.onDisconnected(() => this.fireTerminatedEvent()));
@@ -155,8 +158,9 @@ export abstract class BrowserLauncher<T extends AnyChromiumLaunchConfiguration>
       telemetryReporter,
       targetOrigin,
     );
-    if (!this._targetManager)
-      return localize('error.unableToAttachToBrowser', 'Unable to attach to the browser');
+    if (!this._targetManager) {
+      throw new ProtocolError(browserAttachFailed());
+    }
 
     this._targetManager.serviceWorkerModel.onDidChange(() =>
       this._onTargetListChangedEmitter.fire(),
@@ -179,10 +183,14 @@ export abstract class BrowserLauncher<T extends AnyChromiumLaunchConfiguration>
       'Could not attach to main target',
     );
 
-    if (!this._mainTarget) return localize('error.threadNotFound', 'Target page not found');
+    if (!this._mainTarget) {
+      throw new ProtocolError(targetPageNotFound());
+    }
+
     this._targetManager.onTargetRemoved((target: BrowserTarget) => {
       if (target === this._mainTarget) this.fireTerminatedEvent();
     });
+
     return this._mainTarget;
   }
 
@@ -217,12 +225,8 @@ export abstract class BrowserLauncher<T extends AnyChromiumLaunchConfiguration>
       return { blockSessionTermination: false };
     }
 
-    const targetOrError = await this.prepareLaunch(resolved, context);
-    if (typeof targetOrError === 'string') {
-      return { error: targetOrError };
-    }
-
-    await this.finishLaunch(targetOrError, resolved);
+    const target = await this.prepareLaunch(resolved, context);
+    await this.finishLaunch(target, resolved);
     return { blockSessionTermination: true };
   }
 

src/targets/delegate/delegateLauncher.ts

@@ -89,17 +89,17 @@ export class DelegateLauncher implements ILauncher {
 
     const delegate = this.parentList.get(params.delegateId);
     if (delegate === undefined) {
-      return { error: `Could not get debug session delegate ID ${params.delegateId}` };
+      throw new Error(`Could not get debug session delegate ID ${params.delegateId}`);
     }
 
     const origin = delegate.target.targetOrigin();
     if (!(origin instanceof MutableTargetOrigin)) {
-      return { error: `Expected delegate session to have a mutable target origin` };
+      throw new Error(`Expected delegate session to have a mutable target origin`);
     }
 
     const logger = delegate.target.logger;
     if (!(logger instanceof ProxyLogger)) {
-      return { error: `Expected delegate session to have a proxy logger` };
+      throw new Error(`Expected delegate session to have a proxy logger`);
     }
 
     // Update the origin to 're-home' it under the current debug session,

src/targets/node/extensionHostAttacher.ts

@@ -45,7 +45,7 @@ export class ExtensionHostAttacher extends NodeAttacherBase<IExtensionHostAttach
    */
   protected async launchProgram(
     runData: IRunData<IExtensionHostAttachConfiguration>,
-  ): Promise<string | void> {
+  ): Promise<void> {
     const inspectorURL = await retryGetWSEndpoint(
       `http://localhost:${runData.params.port}`,
       runData.context.cancellationToken,

src/targets/node/nodeLauncherBase.ts

@@ -156,8 +156,8 @@ export abstract class NodeLauncherBase<T extends AnyNodeConfiguration> implement
       ),
     });
 
-    const error = await this.launchProgram(run);
-    return error ? { error } : { blockSessionTermination: true };
+    await this.launchProgram(run);
+    return { blockSessionTermination: true };
   }
 
   /**
@@ -227,7 +227,7 @@ export abstract class NodeLauncherBase<T extends AnyNodeConfiguration> implement
   /**
    * Launches the program. Called after the server is running and upon restart.
    */
-  protected abstract launchProgram(runData: IRunData<T>): Promise<string | void>;
+  protected abstract launchProgram(runData: IRunData<T>): Promise<void>;
 
   /**
    * Method that should be called when the program from launchProgram() exits.

src/targets/targets.ts

@@ -66,7 +66,6 @@ export interface ILaunchContext {
 }
 
 export interface ILaunchResult {
-  error?: string;
   blockSessionTermination?: boolean;
 }