Merge pull request #285 from digeff/fix_first_line_bp_chrome

Fix break-on-load breakpoint not hitting on the first line for Chrome

src/chrome/breakOnLoadHelper.ts

@@ -12,7 +12,6 @@ import * as ChromeUtils from './chromeUtils';
 
 export class BreakOnLoadHelper {
 
-    public userBreakpointOnLine1Col1: boolean = false;
     private _instrumentationBreakpointSet: boolean = false;
 
     // Break on load: Store some mapping between the requested file names, the regex for the file, and the chrome breakpoint id to perform lookup operations efficiently
@@ -67,12 +66,16 @@ export class BreakOnLoadHelper {
         }
     }
 
+    private getScriptUrlFromId(scriptId: string): string {
+        return this._chromeDebugAdapter.scriptsById.get(scriptId).url;
+    }
+
     /**
      * Checks and resolves the pending breakpoints given a script Id. If any breakpoints were resolved returns true, else false.
      * Used when break on load active, either through Chrome's Instrumentation Breakpoint API or the regex approach
      */
     private async resolvePendingBreakpointsOfPausedScript(scriptId: string): Promise<boolean> {
-        const pausedScriptUrl = this._chromeDebugAdapter.scriptsById.get(scriptId).url;
+        const pausedScriptUrl = this.getScriptUrlFromId(scriptId);
         const sourceMapUrl = this._chromeDebugAdapter.scriptsById.get(scriptId).sourceMapURL;
         const mappedUrl = await this._chromeDebugAdapter.pathTransformer.scriptParsed(pausedScriptUrl);
         let breakpointsResolved = false;
@@ -122,16 +125,24 @@ export class BreakOnLoadHelper {
      * Returns whether we should continue on hitting a stopOnEntry breakpoint
      * Only used when using regex approach for break on load
      */
-    private async shouldContinueOnStopOnEntryBreakpoint(scriptId: string): Promise<boolean> {
+    private async shouldContinueOnStopOnEntryBreakpoint(pausedLocation: Crdp.Debugger.Location): Promise<boolean> {
         // If the file has no unbound breakpoints or none of the resolved breakpoints are at (1,1), we should continue after hitting the stopOnEntry breakpoint
         let shouldContinue = true;
-        let anyPendingBreakpointsResolved = await this.resolvePendingBreakpointsOfPausedScript(scriptId);
+
+        // Important: For the logic that verifies if a user breakpoint is set in the paused location, we need to resolve pending breakpoints, and commit them, before
+        // using committedBreakpointsByUrl for our logic.
+        let anyPendingBreakpointsResolved = await this.resolvePendingBreakpointsOfPausedScript(pausedLocation.scriptId);
+
+        const pausedScriptUrl = this.getScriptUrlFromId(pausedLocation.scriptId);
+        // Important: We need to get the committed breakpoints only after all the pending breakpoints for this file have been resolved. If not this logic won't work
+        const committedBps = this._chromeDebugAdapter.committedBreakpointsByUrl.get(pausedScriptUrl);
+        const anyBreakpointsAtPausedLocation = committedBps.filter(bp =>
+            bp.actualLocation.lineNumber === pausedLocation.lineNumber && bp.actualLocation.columnNumber === pausedLocation.columnNumber).length > 0;
 
         // If there were any pending breakpoints resolved and any of them was at (1,1) we shouldn't continue
-        if (anyPendingBreakpointsResolved && this.userBreakpointOnLine1Col1) {
+        if (anyPendingBreakpointsResolved && anyBreakpointsAtPausedLocation) {
             // Here we need to store this information per file, but since we can safely assume that scriptParsed would immediately be followed by onPaused event
             // for the breakonload files, this implementation should be fine
-            this.userBreakpointOnLine1Col1 = false;
             shouldContinue = false;
         }
 
@@ -163,8 +174,8 @@ export class BreakOnLoadHelper {
         // in itself. So when the file is actually loaded, we would have 2 stopOnEntry breakpoints */
 
         if (allStopOnEntryBreakpoints) {
-            const pausedScriptId = notification.callFrames[0].location.scriptId;
-            let shouldContinue = await this.shouldContinueOnStopOnEntryBreakpoint(pausedScriptId);
+            const pausedLocation = notification.callFrames[0].location;
+            let shouldContinue = await this.shouldContinueOnStopOnEntryBreakpoint(pausedLocation);
             if (shouldContinue) {
                 return true;
             }

src/chrome/chromeDebugAdapter.ts

@@ -88,7 +88,7 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
     protected _domains = new Map<CrdpDomain, Crdp.Schema.Domain>();
     private _clientAttached: boolean;
     private _currentPauseNotification: Crdp.Debugger.PausedEvent;
-    private _committedBreakpointsByUrl: Map<string, Crdp.Debugger.BreakpointId[]>;
+    private _committedBreakpointsByUrl: Map<string, ISetBreakpointResult[]>;
     private _exception: Crdp.Runtime.RemoteObject;
     private _setBreakpointsRequestQ: Promise<any>;
     private _expectingResumedEvent: boolean;
@@ -175,6 +175,10 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         return this._pendingBreakpointsByUrl;
     }
 
+    public get committedBreakpointsByUrl(): Map<string, ISetBreakpointResult[]> {
+        return this._committedBreakpointsByUrl;
+    }
+
     public get sourceMapTransformer(): BaseSourceMapTransformer{
         return this._sourceMapTransformer;
     }
@@ -188,7 +192,7 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         this._scriptsById = new Map<Crdp.Runtime.ScriptId, Crdp.Debugger.ScriptParsedEvent>();
         this._scriptsByUrl = new Map<string, Crdp.Debugger.ScriptParsedEvent>();
 
-        this._committedBreakpointsByUrl = new Map<string, Crdp.Debugger.BreakpointId[]>();
+        this._committedBreakpointsByUrl = new Map<string, ISetBreakpointResult[]>();
         this._setBreakpointsRequestQ = Promise.resolve();
 
         this._pathTransformer.clearTargetContext();
@@ -923,10 +927,6 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         return this.setBreakpoints(pendingBP.args, pendingBP.requestSeq, pendingBP.ids).then(response => {
             response.breakpoints.forEach((bp, i) => {
                 bp.id = pendingBP.ids[i];
-                // If any of the unbound breakpoints in this file is on (1,1), we set userBreakpointOnLine1Col1 to true
-                if (bp.line === 1 && bp.column === 1 && this.breakOnLoadActive) {
-                    this._breakOnLoadHelper.userBreakpointOnLine1Col1 = true;
-                }
                 this._session.sendEvent(new BreakpointEvent('changed', bp));
             });
         });
@@ -945,8 +945,8 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         }
 
         const committedBps = this._committedBreakpointsByUrl.get(script.url) || [];
-        if (committedBps.indexOf(params.breakpointId) === -1) {
-            committedBps.push(params.breakpointId);
+        if (!committedBps.find(committedBp => committedBp.breakpointId === params.breakpointId)) {
+            committedBps.push({breakpointId: params.breakpointId, actualLocation: params.location});
         }
         this._committedBreakpointsByUrl.set(script.url, committedBps);
 
@@ -1233,8 +1233,8 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         // but there is a chrome bug where when removing 5+ or so breakpoints at once, it gets into a weird
         // state where later adds on the same line will fail with 'breakpoint already exists' even though it
         // does not break there.
-        return this._committedBreakpointsByUrl.get(url).reduce((p, breakpointId) => {
-            return p.then(() => this.chrome.Debugger.removeBreakpoint({ breakpointId })).then(() => { });
+        return this._committedBreakpointsByUrl.get(url).reduce((p, bp) => {
+            return p.then(() => this.chrome.Debugger.removeBreakpoint({ breakpointId: bp.breakpointId })).then(() => { });
         }, Promise.resolve()).then(() => {
             this._committedBreakpointsByUrl.delete(url);
         });
@@ -1318,12 +1318,11 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
 
     private targetBreakpointResponsesToClientBreakpoints(url: string, responses: ISetBreakpointResult[], requestBps: DebugProtocol.SourceBreakpoint[], ids?: number[]): DebugProtocol.Breakpoint[] {
         // Don't cache errored responses
-        const committedBpIds = responses
-            .filter(response => response && response.breakpointId)
-            .map(response => response.breakpointId);
+        const committedBps = responses
+            .filter(response => response && response.breakpointId);
 
         // Cache successfully set breakpoint ids from chrome in committedBreakpoints set
-        this._committedBreakpointsByUrl.set(url, committedBpIds);
+        this._committedBreakpointsByUrl.set(url, committedBps);
 
         // Map committed breakpoints to DebugProtocol response breakpoints
         return responses