fix: assorted bad source behaviors when reloading a page

.husky/pre-commit

@@ -1,4 +1,4 @@
 #!/bin/sh
 . "$(dirname "$0")/_/husky.sh"
 
-npm run test:lint && npm run test:types
+npm run -S precommit

CHANGELOG.md

@@ -7,6 +7,7 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
 - fix: repl stacktrace with renames showing too much info ([#1259](https://github.com/microsoft/vscode-js-debug/issues/1259#issuecomment-1409443564))
 - fix: recursive source map resolution parsing ignored locations ([vscode#169733](https://github.com/microsoft/vscode/issues/169733))
 - fix: unbound breakpoints in sourcemaps on Chrome 112 ([#1567](https://github.com/microsoft/vscode-js-debug/issues/1567))
+- fix: assorted bad source behaviors when reloading a page ([#1582](https://github.com/microsoft/vscode-js-debug/issues/1582))
 - chore: remove webpack, adopt esbuild
 
 ## v1.76 (February 2023)

package.json

@@ -38,6 +38,7 @@
     "prepare": "husky install",
     "package": "gulp package",
     "publish": "gulp publish",
+    "precommit": "npm-run-all --parallel test:lint test:types",
     "updatetypes": "cd src/typings && npx vscode-dts dev && npx vscode-dts master",
     "updatenodeapi": "python src/build/getNodePdl.py && prettier --write src/build/nodeCustom.ts",
     "generateapis": "node dist/src/build/generateDap.js && node dist/src/build/generateCdp.js",

src/adapter/breakpoints.ts

@@ -26,17 +26,17 @@ import { PatternEntryBreakpoint } from './breakpoints/patternEntrypointBreakpoin
 import { UserDefinedBreakpoint } from './breakpoints/userDefinedBreakpoint';
 import { DiagnosticToolSuggester } from './diagnosticToolSuggester';
 import {
-  base0To1,
-  base1To0,
   ISourceWithMap,
-  isSourceWithMap,
   IUiLocation,
-  rawToUiOffset,
   Source,
   SourceContainer,
+  base0To1,
+  base1To0,
+  isSourceWithMap,
+  rawToUiOffset,
   uiToRawOffset,
 } from './sources';
-import { Script, ScriptWithSourceMapHandler, Thread } from './threads';
+import { ScriptWithSourceMapHandler, Thread } from './threads';
 
 /**
  * Differential result used internally in setBreakpoints.
@@ -348,16 +348,15 @@ export class BreakpointManager {
         continue;
       }
 
-      // Only take the first script that matches this source. The breakpoints
+      // Only take the last script that matches this source. The breakpoints
       // are all coming from the same source code, so possible breakpoints
       // at one location where this source is present should match every other.
       const lsrc = start.source;
-      const scripts = thread.scriptsFromSource(lsrc);
-      if (scripts.size === 0) {
+      if (!lsrc.scripts.length) {
         continue;
       }
 
-      const { scriptId } = scripts.values().next().value as Script;
+      const { scriptId } = lsrc.scripts[lsrc.scripts.length - 1];
       todo.push(
         thread
           .cdp()
@@ -610,7 +609,7 @@ export class BreakpointManager {
     await Promise.all(
       result.unbound.map(b => {
         this._byDapId.delete(b.dapId);
-        b.disable();
+        return b.disable();
       }),
     );
 

src/adapter/breakpoints/breakpointBase.ts

@@ -7,7 +7,14 @@ import { LogTag } from '../../common/logging';
 import { absolutePathToFileUrl, urlToRegex } from '../../common/urlUtils';
 import Dap from '../../dap/api';
 import { BreakpointManager } from '../breakpoints';
-import { base1To0, IUiLocation, Source, SourceFromMap, uiToRawOffset } from '../sources';
+import {
+  base1To0,
+  ISourceScript,
+  IUiLocation,
+  Source,
+  SourceFromMap,
+  uiToRawOffset,
+} from '../sources';
 import { Script, Thread } from '../threads';
 
 export type LineColumn = { lineNumber: number; columnNumber: number }; // 1-based
@@ -451,10 +458,9 @@ export abstract class Breakpoint {
   }
 
   private async _setByUiLocation(thread: Thread, uiLocation: IUiLocation): Promise<void> {
-    const promises: Promise<void>[] = [];
-    const scripts = thread.scriptsFromSource(uiLocation.source);
-    for (const script of scripts) promises.push(this._setByScriptId(thread, script, uiLocation));
-    await Promise.all(promises);
+    await Promise.all(
+      uiLocation.source.scripts.map(script => this._setByScriptId(thread, script, uiLocation)),
+    );
   }
 
   protected async _setByPath(thread: Thread, lineColumn: LineColumn): Promise<void> {
@@ -503,7 +509,7 @@ export abstract class Breakpoint {
    * requests to avoid triggering any logpoint breakpoints multiple times,
    * as would happen if we set a breakpoint both by script and URL.
    */
-  protected hasSetOnLocation(script: Partial<Script>, lineColumn: LineColumn) {
+  protected hasSetOnLocation(script: ISourceScript, lineColumn: LineColumn) {
     return this.cdpBreakpoints.find(
       bp =>
         (script.scriptId &&
@@ -566,7 +572,7 @@ export abstract class Breakpoint {
 
   private async _setByScriptId(
     thread: Thread,
-    script: Script,
+    script: ISourceScript,
     lineColumn: LineColumn,
   ): Promise<void> {
     lineColumn = base1To0(lineColumn);

src/adapter/diagnosics.ts

@@ -140,7 +140,7 @@ export class Diagnostics {
           sourceReference: source.sourceReference,
           absolutePath: source.absolutePath,
           actualAbsolutePath: await source.existingAbsolutePath(),
-          scriptIds: source.scriptIds(),
+          scriptIds: source.scripts.map(s => s.scriptId),
           prettyName: await source.prettyName(),
           compiledSourceRefToUrl:
             source instanceof SourceFromMap

src/adapter/scriptSkipper/implementation.ts

@@ -12,19 +12,20 @@ import { ILogger, LogTag } from '../../common/logging';
 import { node15InternalsPrefix } from '../../common/node15Internal';
 import { memoizeLast, trailingEdgeThrottle, truthy } from '../../common/objUtils';
 import * as pathUtils from '../../common/pathUtils';
-import { getDeferred, IDeferred } from '../../common/promiseUtil';
+import { IDeferred, getDeferred } from '../../common/promiseUtil';
 import { ISourcePathResolver } from '../../common/sourcePathResolver';
 import { escapeRegexSpecialChars } from '../../common/stringUtils';
 import * as urlUtils from '../../common/urlUtils';
 import { AnyLaunchConfiguration } from '../../configuration';
 import Dap from '../../dap/api';
 import { ITarget } from '../../targets/targets';
 import {
+  ISourceScript,
   ISourceWithMap,
-  isSourceWithMap,
   Source,
   SourceContainer,
   SourceFromMap,
+  isSourceWithMap,
 } from '../sources';
 import { getSourceSuffix } from '../templates';
 import { simpleGlobsToRe } from './simpleGlobToRe';
@@ -205,7 +206,7 @@ export class ScriptSkipper {
 
   private async _updateSourceWithSkippedSourceMappedSources(
     source: ISourceWithMap,
-    scriptIds: Cdp.Runtime.ScriptId[],
+    scripts: readonly ISourceScript[],
   ): Promise<void> {
     // Order "should" be correct
     const parentIsSkipped = this.isScriptSkipped(source.url);
@@ -238,14 +239,14 @@ export class ScriptSkipper {
       }
     });
 
-    let targets = scriptIds;
+    let targets = scripts;
     if (!skipRanges.length) {
-      targets = targets.filter(t => this._scriptsWithSkipping.has(t));
-      targets.forEach(t => this._scriptsWithSkipping.delete(t));
+      targets = targets.filter(t => this._scriptsWithSkipping.has(t.scriptId));
+      targets.forEach(t => this._scriptsWithSkipping.delete(t.scriptId));
     }
 
     await Promise.all(
-      targets.map(scriptId =>
+      targets.map(({ scriptId }) =>
         this.cdp.Debugger.setBlackboxedRanges({ scriptId, positions: skipRanges }),
       ),
     );
@@ -255,7 +256,7 @@ export class ScriptSkipper {
     this._initializeSkippingValueForSource(source);
   }
 
-  private _initializeSkippingValueForSource(source: Source, scriptIds = source.scriptIds()) {
+  private _initializeSkippingValueForSource(source: Source, scripts = source.scripts) {
     const url = source.url;
     let skipped = this.isScriptSkipped(url);
 
@@ -281,10 +282,10 @@ export class ScriptSkipper {
       }
 
       for (const nestedSource of source.sourceMap.sourceByUrl.values()) {
-        this._initializeSkippingValueForSource(nestedSource, scriptIds);
+        this._initializeSkippingValueForSource(nestedSource, scripts);
       }
 
-      this._updateSourceWithSkippedSourceMappedSources(source, scriptIds);
+      this._updateSourceWithSkippedSourceMappedSources(source, scripts);
     }
   }
 
@@ -333,10 +334,7 @@ export class ScriptSkipper {
       const compiledSources = Array.from(source.compiledToSourceUrl.keys());
       await Promise.all(
         compiledSources.map(compiledSource =>
-          this._updateSourceWithSkippedSourceMappedSources(
-            compiledSource,
-            compiledSource.scriptIds(),
-          ),
+          this._updateSourceWithSkippedSourceMappedSources(compiledSource, compiledSource.scripts),
         ),
       );
     } else {

src/adapter/sources.ts

@@ -17,7 +17,7 @@ import { forceForwardSlashes, isSubdirectoryOf, properResolve } from '../common/
 import { delay, getDeferred } from '../common/promiseUtil';
 import { ISourceMapMetadata, SourceMap } from '../common/sourceMaps/sourceMap';
 import { CachingSourceMapFactory, ISourceMapFactory } from '../common/sourceMaps/sourceMapFactory';
-import { InlineScriptOffset, ISourcePathResolver } from '../common/sourcePathResolver';
+import { ISourcePathResolver, InlineScriptOffset } from '../common/sourcePathResolver';
 import * as sourceUtils from '../common/sourceUtils';
 import { prettyPrintAsSourceMap } from '../common/sourceUtils';
 import * as utils from '../common/urlUtils';
@@ -95,6 +95,12 @@ const defaultTimeouts: SourceMapTimeouts = {
   sourceMapCumulativePause: 10000,
 };
 
+export interface ISourceScript {
+  executionContextId: Cdp.Runtime.ExecutionContextId;
+  scriptId: Cdp.Runtime.ScriptId;
+  url: string;
+}
+
 // Represents a text source visible to the user.
 //
 // Source maps flow (start with compiled1 and compiled2). Two different compiled sources
@@ -134,7 +140,7 @@ export class Source {
   // This is the same as |_absolutePath|, but additionally checks that file exists to
   // avoid errors when page refers to non-existing paths/urls.
   private readonly _existingAbsolutePath: Promise<string | undefined>;
-  private readonly _scriptIds: Cdp.Runtime.ScriptId[] = [];
+  private _scripts: ISourceScript[] = [];
 
   /**
    * @param inlineScriptOffset Offset of the start location of the script in
@@ -157,7 +163,7 @@ export class Source {
     sourceMapUrl?: string,
     public readonly inlineScriptOffset?: InlineScriptOffset,
     public readonly runtimeScriptOffset?: InlineScriptOffset,
-    contentHash?: string,
+    public readonly contentHash?: string,
   ) {
     this.sourceReference = container.getSourceReference(url);
     this._contentGetter = once(contentGetter);
@@ -191,12 +197,26 @@ export class Source {
     };
   }
 
-  addScriptId(scriptId: Cdp.Runtime.ScriptId): void {
-    this._scriptIds.push(scriptId);
+  /**
+   * Associated a script with this source. This is only valid for a source
+   * from the runtime, not a {@link SourceFromMap}.
+   */
+  addScript(script: ISourceScript): void {
+    this._scripts.push(script);
+  }
+
+  /**
+   * Filters scripts from a source, done when an execution context is removed.
+   */
+  filterScripts(fn: (s: ISourceScript) => boolean): void {
+    this._scripts = this._scripts.filter(fn);
   }
 
-  scriptIds(): Cdp.Runtime.ScriptId[] {
-    return this._scriptIds;
+  /**
+   * Gets scripts associated with this source.
+   */
+  get scripts(): ReadonlyArray<ISourceScript> {
+    return this._scripts;
   }
 
   /**
@@ -903,6 +923,9 @@ export class SourceContainer {
   }
 
   private async _addSource(source: Source) {
+    // todo: we should allow the same source at multiple uri's if their scripts
+    // have different executionContextId. We only really need the overwrite
+    // behavior in Node for tools that transpile sources inline.
     const existingByUrl = source.url && this._sourceByOriginalUrl.get(source.url);
     if (existingByUrl && !isOriginalSourceOf(existingByUrl, source)) {
       this.removeSource(existingByUrl, true);
@@ -1023,6 +1046,12 @@ export class SourceContainer {
       'Expected source to be the same as the existing reference',
     );
     this._sourceByReference.delete(source.sourceReference);
+
+    // check for overwrites:
+    if (this._sourceByOriginalUrl.get(source.url) === source) {
+      this._sourceByOriginalUrl.delete(source.url);
+    }
+
     if (source instanceof SourceFromMap) {
       this._sourceMapSourcesByUrl.delete(source.url);
       for (const [compiled, key] of source.compiledToSourceUrl) {