Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

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

Merged
merged 4 commits into from
Feb 14, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb84e4e
Fix brreak-on-load breakpoint not hitting on the first line for Chrome
Feb 14, 2018
a5089b5
tslint
Feb 14, 2018
540ae3b
Fixed typo
Feb 14, 2018
d2523cb
Applied feedback
Feb 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/chrome/breakOnLoadHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -67,12 +66,16 @@ export class BreakOnLoadHelper {
}
}

private getPausedScriptUrlFromId(scriptId: string): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just a "paused" script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Renaming to getScriptUrlFromId

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.getPausedScriptUrlFromId(scriptId);
const sourceMapUrl = this._chromeDebugAdapter.scriptsById.get(scriptId).sourceMapURL;
const mappedUrl = await this._chromeDebugAdapter.pathTransformer.scriptParsed(pausedScriptUrl);
let breakpointsResolved = false;
Expand Down Expand Up @@ -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.getPausedScriptUrlFromId(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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
27 changes: 13 additions & 14 deletions src/chrome/chromeDebugAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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();
Expand Down Expand Up @@ -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));
});
});
Expand All @@ -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.findIndex(committedBp => committedBp.breakpointId === params.breakpointId) === -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer if (!committedBps.find(... instead of checking index -1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

committedBps.push({breakpointId: params.breakpointId, actualLocation: params.location});
}
this._committedBreakpointsByUrl.set(script.url, committedBps);

Expand Down Expand Up @@ -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);
});
Expand Down Expand Up @@ -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
Expand Down