fix: race in entrypoint breakpoints (#1360)

* fix: race in entrypoint breakpoints

This was causing a ~10% failure in a test, and could have caused some
failures for users in edge cases (where rarely stepping into the
first function or class declaration in a file would not work.)

* chore: update vscode-test for resiliency, use newer macos image

* chore: use node 16 in CI to match vscode

Author: connor4312

.ci/pipeline.yml

@@ -6,27 +6,27 @@ jobs:
 - job: macOS
   timeoutInMinutes: 20
   pool:
-    vmImage: 'macOS-10.15'
+    vmImage: 'macos-latest'
   steps:
   - template: common-validation.yml
   variables:
-    node_version: 14.18.3
+    node_version: 16.14.2
 
 - job: Linux
   pool:
     vmImage: 'ubuntu-18.04'
   steps:
   - template: common-validation.yml
   variables:
-    node_version: 14.18.3
+    node_version: 16.14.2
 
 - job: LinuxMinspec
   pool:
     vmImage: 'ubuntu-18.04'
   steps:
   - template: common-validation.yml
   variables:
-    node_version: 14.18.3
+    node_version: 16.14.2
     only_minspec: true
 
 - job: Windows
@@ -35,4 +35,4 @@ jobs:
   steps:
   - template: common-validation.yml
   variables:
-    node_version: 14.18.3
+    node_version: 16.14.2

package-lock.json

@@ -122,7 +122,7 @@
     "@types/ws": "^8.5.3",
     "@typescript-eslint/eslint-plugin": "^5.17.0",
     "@typescript-eslint/parser": "^5.17.0",
-    "@vscode/test-electron": "^2.1.3",
+    "@vscode/test-electron": "^2.1.5",
     "chai": "^4.3.6",
     "chai-as-promised": "^7.1.1",
     "chai-string": "^1.5.0",

package.json

@@ -217,6 +217,13 @@ export abstract class Breakpoint {
     cdpId: Cdp.Debugger.BreakpointId,
     resolvedLocations: readonly Cdp.Debugger.Location[],
   ) {
+    // Update with the resolved locations immediately and synchronously. This
+    // prevents a race conditions where a source is parsed immediately before
+    // a breakpoint it hit and not returning correctly in `BreakpointManager.isEntrypointBreak`.
+    // This _can_ be fairly prevelant, especially when resolving UI locations
+    // involves loading or waiting for source maps.
+    this.updateExistingCdpRef(cdpId, bp => ({ ...bp, locations: resolvedLocations }));
+
     const uiLocation = (
       await Promise.all(
         resolvedLocations.map(l => thread.rawLocationToUiLocation(thread.rawLocation(l))),
@@ -232,20 +239,15 @@ export abstract class Breakpoint {
       return;
     }
 
-    this.updateCdpRefs(list =>
-      list.map(bp => {
-        if (bp.state !== CdpReferenceState.Applied || bp.cdpId !== cdpId) {
-          return bp;
-        }
-        const locations = this._manager._sourceContainer.currentSiblingUiLocations(uiLocation);
-        const inPreferredSource = locations.filter(l => l.source === source);
-        return {
-          ...bp,
-          locations: resolvedLocations,
-          uiLocations: inPreferredSource.length ? inPreferredSource : locations,
-        };
-      }),
-    );
+    this.updateExistingCdpRef(cdpId, bp => {
+      const locations = this._manager._sourceContainer.currentSiblingUiLocations(uiLocation);
+      const inPreferredSource = locations.filter(l => l.source === source);
+      return {
+        ...bp,
+        locations: resolvedLocations,
+        uiLocations: inPreferredSource.length ? inPreferredSource : locations,
+      };
+    });
   }
 
   /**
@@ -377,6 +379,20 @@ export abstract class Breakpoint {
     return undefined;
   }
 
+  /**
+   * Updates an existing applied CDP breakpoint, by its CDP ID.
+   */
+  protected updateExistingCdpRef(
+    cdpId: string,
+    mutator: (l: Readonly<BreakpointCdpReference>) => Readonly<BreakpointCdpReference>,
+  ) {
+    this.updateCdpRefs(list =>
+      list.map(bp =>
+        bp.state !== CdpReferenceState.Applied || bp.cdpId !== cdpId ? bp : mutator(bp),
+      ),
+    );
+  }
+
   /**
    * Updates the list of CDP breakpoint references. Used to provide lifecycle
    * hooks to consumers and internal caches.

src/adapter/breakpoints/breakpointBase.ts

@@ -8,6 +8,6 @@
     0: '--some'
     1: 'very fancy'
     2: '--arguments'
-    > __proto__: Array(0)
-    > __proto__: Object
     length: 3
+    > [[Prototype]]: Array(0)
+    > [[Prototype]]: Object

src/test/node/node-runtime-sets-arguments.txt

@@ -497,10 +497,6 @@ describe('node runtime', () => {
   });
 
   itIntegrates('sets arguments', async ({ r }) => {
-    if (process.platform === 'darwin') {
-      return; // the ADO runner on Darwin seems to use the wrong Node version
-    }
-
     createFileTree(testFixturesDir, { 'test.js': 'debugger' });
     const handle = await r.runScript('test.js', {
       args: ['--some', 'very fancy', '--arguments'],