fix: show errors from cond bps, don't pause on internal exceptions (#…

…1902)

* fix: show errors from cond bps, don't pause on internal exceptions

Fixes microsoft/vscode#195062

* fix tests

CHANGELOG.md

@@ -4,6 +4,8 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
 
 ## Nightly (only)
 
+- fix: show errors from conditional breakpoints ([vscode#195062](https://github.com/microsoft/vscode/issues/195062))
+- fix: pausing on exceptions caused by internal scripts ([vscode#195062](https://github.com/microsoft/vscode/issues/195062))
 - fix: automatically reconnect when debugging browsers in port mode ([vscode#174033](https://github.com/microsoft/vscode/issues/174033))
 
 ## v1.85 (November 2023)

src/adapter/breakpoints/conditions/expression.ts

@@ -20,9 +20,7 @@ export class ExpressionCondition implements IBreakpointCondition {
     breakOnError: boolean,
     evaluator: IEvaluator,
   ) {
-    if (breakOnError) {
-      breakCondition = wrapBreakCondition(breakCondition);
-    }
+    breakCondition = wrapBreakCondition(breakCondition, breakOnError);
 
     const err = breakCondition && getSyntaxErrorIn(breakCondition);
     if (err) {
@@ -61,5 +59,5 @@ export class ExpressionCondition implements IBreakpointCondition {
   }
 }
 
-export const wrapBreakCondition = (condition: string) =>
-  `(()=>{try{return ${condition};}catch(e){console.error(e);return true}})()`;
+export const wrapBreakCondition = (condition: string, breakOnError: boolean) =>
+  `(()=>{try{return ${condition};}catch(e){console.error(\`Breakpoint condition error: \${e.message||e}\`);return ${!!breakOnError}}})()`;

src/adapter/exceptionPauseService.test.ts

@@ -35,7 +35,7 @@ describe('ExceptionPauseService', () => {
       upcastPartial<ScriptSkipper>({ isScriptSkipped }),
       stubDap as unknown as Dap.Api,
       upcastPartial<AnyLaunchConfiguration>({}),
-      upcastPartial<SourceContainer>({ getScriptById }),
+      upcastPartial<SourceContainer>({ getScriptById, getSourceScriptById: getScriptById }),
     );
   });
 
@@ -117,7 +117,10 @@ describe('ExceptionPauseService', () => {
       filters: [],
       filterOptions: [{ filterId: PauseOnExceptionsState.All, condition: 'error.name == "hi"' }],
     });
-    expect(prepareEval.args[0]).to.deep.equal(['!!(error.name == "hi")', { hoist: ['error'] }]);
+    expect(prepareEval.args[0]).to.deep.equal([
+      '(()=>{try{return !!(error.name == "hi");}catch(e){console.error(`Breakpoint condition error: ${e.message||e}`);return false}})()',
+      { hoist: ['error'] },
+    ]);
     expect(stubDap.output.called).to.be.false;
     expect(stubCdp.Debugger.setPauseOnExceptions.calledWith({ state: 'all' })).to.be.true;
 

src/adapter/exceptionPauseService.ts

@@ -6,7 +6,7 @@ import { inject, injectable } from 'inversify';
 import Cdp from '../cdp/api';
 import { truthy } from '../common/objUtils';
 import { getDeferred } from '../common/promiseUtil';
-import { getSyntaxErrorIn } from '../common/sourceUtils';
+import { SourceConstants, getSyntaxErrorIn } from '../common/sourceUtils';
 import { AnyLaunchConfiguration } from '../configuration';
 import Dap from '../dap/api';
 import { IDapApi } from '../dap/connection';
@@ -120,6 +120,19 @@ export class ExceptionPauseService implements IExceptionPauseService {
       return false;
     }
 
+    // If there's an internal frame anywhere in the stack, this call is from
+    // some internally-executed script not visible for the user. Never pause
+    // if this results in an exception: the caller should handle it.
+    if (
+      evt.callFrames.some(cf =>
+        this.sourceContainer
+          .getSourceScriptById(cf.location.scriptId)
+          ?.url.endsWith(SourceConstants.InternalExtension),
+      )
+    ) {
+      return false;
+    }
+
     if (this.shouldScriptSkip(evt)) {
       return false;
     }
@@ -233,7 +246,7 @@ export class ExceptionPauseService implements IExceptionPauseService {
         );
       }
 
-      const wrapped = this.breakOnError ? wrapBreakCondition(expr) : expr;
+      const wrapped = wrapBreakCondition(expr, this.breakOnError);
       return this.evaluator.prepare(wrapped, { hoist: ['error'] }).invoke;
     };
 

src/adapter/sourceContainer.ts

@@ -30,6 +30,7 @@ import { IScriptSkipper } from './scriptSkipper/scriptSkipper';
 import {
   ContentGetter,
   ISourceMapLocationProvider,
+  ISourceScript,
   ISourceWithMap,
   IUiLocation,
   LineColumn,
@@ -139,7 +140,7 @@ export class SourceContainer {
   /**
    * Mapping of CDP script IDs to Script objects.
    */
-  private readonly scriptsById: Map<Cdp.Runtime.ScriptId, Script> = new Map();
+  private readonly scriptsById: Map<Cdp.Runtime.ScriptId, Script | ISourceScript> = new Map();
 
   private onSourceMappedSteppingChangeEmitter = new EventEmitter<boolean>();
   private onScriptEmitter = new EventEmitter<Script>();
@@ -280,18 +281,29 @@ export class SourceContainer {
   /**
    * Adds a new script to the source container.
    */
-  public addScriptById(script: Script) {
+  public addScriptById(script: Script | ISourceScript) {
     this.scriptsById.set(script.scriptId, script);
-    this.onScriptEmitter.fire(script);
+    if ('source' in script) {
+      this.onScriptEmitter.fire(script);
+    }
   }
 
   /**
-   * Gets a script by its script ID.
+   * Gets a source script by its script ID. This includes internals scripts
+   * that are not parsed to full Sources.
    */
-  public getScriptById(scriptId: string) {
+  public getSourceScriptById(scriptId: string): ISourceScript | undefined {
     return this.scriptsById.get(scriptId);
   }
 
+  /**
+   * Gets a script by its script ID.
+   */
+  public getScriptById(scriptId: string): Script | undefined {
+    const s = this.scriptsById.get(scriptId);
+    return s && 'source' in s ? s : undefined;
+  }
+
   /**
    * Gets a source by its original URL from the debugger.
    */

src/adapter/threads.ts

@@ -1517,10 +1517,13 @@ export class Thread implements IVariableStoreLocationProvider {
   }
 
   private _onScriptParsed(event: Cdp.Debugger.ScriptParsedEvent) {
-    if (event.url.endsWith(sourceUtils.SourceConstants.InternalExtension)) {
-      // The customer doesn't care about the internal cdp files, so skip this event
-      return;
-    }
+    const sourceScript: Script | ISourceScript = {
+      url: event.url,
+      hasSourceURL: !!event.hasSourceURL,
+      embedderName: event.embedderName,
+      scriptId: event.scriptId,
+      executionContextId: event.executionContextId,
+    };
 
     // normalize paths paths that old Electron versions can add (#1099)
     if (urlUtils.isAbsolute(event.url)) {
@@ -1531,6 +1534,12 @@ export class Thread implements IVariableStoreLocationProvider {
       return;
     }
 
+    if (event.url.endsWith(sourceUtils.SourceConstants.InternalExtension)) {
+      this._sourceContainer.addScriptById(sourceScript);
+      // The customer doesn't care about the internal cdp files, so skip this event
+      return;
+    }
+
     if (event.url) {
       event.url = this.target.scriptUrlToUrl(event.url);
     }
@@ -1619,14 +1628,7 @@ export class Thread implements IVariableStoreLocationProvider {
       return source;
     };
 
-    const script: Script = {
-      url: event.url,
-      hasSourceURL: !!event.hasSourceURL,
-      embedderName: event.embedderName,
-      scriptId: event.scriptId,
-      executionContextId: event.executionContextId,
-      source: createSource(),
-    };
+    const script: Script = { ...sourceScript, source: createSource() };
     script.source.then(s => (script.resolvedSource = s));
     executionContext.scripts.push(script);
 

src/test/breakpoints/breakpoints-condition-ignores-error-by-default.txt

@@ -5,3 +5,14 @@
     threadId : <number>
 }
 <anonymous> @ ${workspaceFolder}/web/condition.js:5:1
+{
+    category : stderr
+    column : 64
+    line : 1
+    output : Breakpoint condition error: oh no
+    source : {
+        name : <eval>/VM<xx>
+        path : <eval>/VM<xx>
+        sourceReference : <number>
+    }
+}

src/test/breakpoints/breakpoints-condition-pauses-on-error.txt

@@ -5,4 +5,4 @@
     threadId : <number>
 }
 <anonymous> @ ${workspaceFolder}/web/condition.js:2:3
-stderr> oh no
+stderr> Breakpoint condition error: oh no

src/test/breakpoints/breakpointsTest.ts

@@ -596,8 +596,10 @@ describe('breakpoints', () => {
         source: { path: p.workspacePath('web/condition.js') },
         breakpoints: [{ line: 2, column: 0, condition: '(() => { throw "oh no" })()' }],
       });
+      const output = p.dap.once('output');
       p.load();
       await waitForPause(p);
+      await r.log(await output); // an error message
       p.assertLog();
     });
 

src/test/threads/threads-pause-on-exceptions-does-not-pause-on-exceptions-in-internals.txt

@@ -0,0 +1 @@
+Evaluating#1: console.log({ [Symbol.for('debug.description')]() { throw 'oops'; } })

src/test/threads/threadsTest.ts

@@ -2,6 +2,7 @@
  * Copyright (C) Microsoft Corporation. All rights reserved.
  *--------------------------------------------------------*/
 
+import { delay } from '../../common/promiseUtil';
 import { TestP, TestRoot } from '../test';
 import { itIntegrates } from '../testIntegrationUtils';
 
@@ -265,6 +266,25 @@ describe('threads', () => {
       p.assertLog();
     });
 
+    itIntegrates('does not pause on exceptions in internals', async ({ r }) => {
+      const p = await r.launchAndLoad('blank');
+
+      await p.dap.setExceptionBreakpoints({ filters: ['all'] });
+      const e = p.evaluate(
+        `console.log({ [Symbol.for('debug.description')]() { throw 'oops'; } })`,
+      );
+
+      await Promise.race([
+        p.dap.once('stopped').then(() => {
+          throw new Error('should not stop');
+        }),
+        delay(1000),
+      ]);
+
+      await e;
+      p.assertLog();
+    });
+
     itIntegrates('pauses on conditional exceptions', async ({ r }) => {
       const p = await r.launchAndLoad('blank');