From b3c4d01a68b70ab0b42f89121260d05e69c68485 Mon Sep 17 00:00:00 2001 From: Rob Date: Sun, 1 Nov 2015 22:11:40 -0800 Subject: [PATCH] Switch to setBreakpointByUrl so breakpoints will persist through a page refresh, add tests --- test/webkit/webKitDebugAdapter.test.ts | 58 +++++++++++++++++--- webkit/webKitConnection.ts | 4 ++ webkit/webKitDebugAdapter.ts | 73 ++++++++++++++++---------- webkit/webKitDebugSession.ts | 6 +-- webkit/webKitProtocol.d.ts | 24 +++++++-- 5 files changed, 122 insertions(+), 43 deletions(-) diff --git a/test/webkit/webKitDebugAdapter.test.ts b/test/webkit/webKitDebugAdapter.test.ts index b79f601..dfeb238 100644 --- a/test/webkit/webKitDebugAdapter.test.ts +++ b/test/webkit/webKitDebugAdapter.test.ts @@ -82,20 +82,19 @@ suite('WebKitDebugAdapter', () => { }); suite('setBreakpoints()', () => { - const SCRIPT_ID = 'id'; const BP_ID = 'bpId'; const FILE_NAME = 'file:///a.js'; - function expectSetBreakpoint(lines: number[], cols?: number[]): void { + function expectSetBreakpoint(lines: number[], cols?: number[], scriptId: string = 'SCRIPT_ID'): void { lines.forEach((lineNumber, i) => { let columnNumber; if (cols) { columnNumber = cols[i]; } - mockWebKitConnection.expects('debugger_setBreakpoint') + mockWebKitConnection.expects('debugger_setBreakpointByUrl') .once() - .withArgs({ scriptId: SCRIPT_ID, lineNumber, columnNumber }) - .returns({ id: 0, result: { breakpointId: BP_ID + i, actualLocation: { scriptId: SCRIPT_ID, lineNumber, columnNumber } } }); + .withArgs(FILE_NAME, lineNumber, columnNumber) + .returns({ id: 0, result: { breakpointId: BP_ID + i, locations: [{ scriptId, lineNumber, columnNumber }] } }); }); } @@ -127,7 +126,6 @@ suite('WebKitDebugAdapter', () => { const wkda = instantiateWKDA(); return attach(wkda).then(() => { - DefaultMockWebKitConnection.EE.emit('Debugger.scriptParsed', { scriptId: SCRIPT_ID, url: FILE_NAME }); return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); }).then(response => { mockWebKitConnection.verify(); @@ -142,7 +140,6 @@ suite('WebKitDebugAdapter', () => { const wkda = instantiateWKDA(); return attach(wkda).then(() => { - DefaultMockWebKitConnection.EE.emit('Debugger.scriptParsed', { scriptId: SCRIPT_ID, url: FILE_NAME }); return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); }).then(response => { mockWebKitConnection.verify(); @@ -157,7 +154,6 @@ suite('WebKitDebugAdapter', () => { const wkda = instantiateWKDA(); return attach(wkda).then(() => { - DefaultMockWebKitConnection.EE.emit('Debugger.scriptParsed', { scriptId: SCRIPT_ID, url: FILE_NAME }); return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); }).then(response => { lines.push(321); @@ -171,6 +167,52 @@ suite('WebKitDebugAdapter', () => { assert.deepEqual(response, makeExpectedResponse(lines, cols)); }); }); + + test('The adapter handles removing a breakpoint', () => { + const lines = [14, 200]; + const cols = [33, 16]; + expectSetBreakpoint(lines, cols); + + const wkda = instantiateWKDA(); + return attach(wkda).then(() => { + return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); + }).then(response => { + lines.shift(); + cols.shift(); + + expectRemoveBreakpoint([0, 1]); + expectSetBreakpoint(lines, cols); + return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); + }).then(response => { + mockWebKitConnection.verify(); + assert.deepEqual(response, makeExpectedResponse(lines, cols)); + }); + }); + + test('After a page refresh, clears the newly resolved breakpoints before adding new ones', () => { + const lines = [14, 200]; + const cols = [33, 16]; + expectSetBreakpoint(lines, cols); + + const wkda = instantiateWKDA(); + return attach(wkda).then(() => { + return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); + }).then(response => { + expectRemoveBreakpoint([2, 3]); + DefaultMockWebKitConnection.EE.emit('Debugger.globalObjectCleared'); + DefaultMockWebKitConnection.EE.emit('Debugger.scriptParsed', { scriptId: 'afterRefreshScriptId', url: FILE_NAME }); + DefaultMockWebKitConnection.EE.emit('Debugger.breakpointResolved', { breakpointId: BP_ID + 2, location: { scriptId: 'afterRefreshScriptId' } }); + DefaultMockWebKitConnection.EE.emit('Debugger.breakpointResolved', { breakpointId: BP_ID + 3, location: { scriptId: 'afterRefreshScriptId' } }); + + lines.push(321); + cols.push(123); + expectSetBreakpoint(lines, cols, 'afterRefreshScriptId'); + return wkda.setBreakpoints({ source: { path: FILE_NAME }, lines, cols }); + }).then(response => { + mockWebKitConnection.verify(); + assert.deepEqual(response, makeExpectedResponse(lines, cols)); + }); + }); }); suite('launch()', () => { diff --git a/webkit/webKitConnection.ts b/webkit/webKitConnection.ts index 5d38ca6..7316763 100644 --- a/webkit/webKitConnection.ts +++ b/webkit/webKitConnection.ts @@ -146,6 +146,10 @@ export class WebKitConnection { return this.sendMessage('Debugger.setBreakpoint', { location, condition }); } + public debugger_setBreakpointByUrl(url: string, lineNumber: number, columnNumber: number): Promise { + return this.sendMessage('Debugger.setBreakpointByUrl', { url, lineNumber, columnNumber }); + } + public debugger_removeBreakpoint(breakpointId: string): Promise { return this.sendMessage('Debugger.removeBreakpoint', { breakpointId }); } diff --git a/webkit/webKitDebugAdapter.ts b/webkit/webKitDebugAdapter.ts index e3c9ee5..a6aa050 100644 --- a/webkit/webKitDebugAdapter.ts +++ b/webkit/webKitDebugAdapter.ts @@ -22,7 +22,7 @@ export class WebKitDebugAdapter implements IDebugAdapter { private _clientAttached: boolean; private _variableHandles: Handles; private _currentStack: WebKitProtocol.Debugger.CallFrame[]; - private _committedBreakpointsByScriptId: Map; + private _committedBreakpointsByUrl: Map; private _overlayHelper: Utilities.DebounceHelper; private _chromeProc: ChildProcess; @@ -31,7 +31,6 @@ export class WebKitDebugAdapter implements IDebugAdapter { // Scripts private _scriptsById: Map; - private _scriptsByUrl: Map; private _setBreakpointsRequestQ: Promise; @@ -48,8 +47,7 @@ export class WebKitDebugAdapter implements IDebugAdapter { private clearTargetContext(): void { this._scriptsById = new Map(); - this._scriptsByUrl = new Map(); - this._committedBreakpointsByScriptId = new Map(); + this._committedBreakpointsByUrl = new Map(); this._setBreakpointsRequestQ = Promise.resolve(); this.fireEvent(new Event('clearTargetContext')); } @@ -112,6 +110,7 @@ export class WebKitDebugAdapter implements IDebugAdapter { this._webKitConnection.on('Debugger.resumed', () => this.onDebuggerResumed()); this._webKitConnection.on('Debugger.scriptParsed', params => this.onScriptParsed(params)); this._webKitConnection.on('Debugger.globalObjectCleared', () => this.onGlobalObjectCleared()); + this._webKitConnection.on('Debugger.breakpointResolved', params => this.onBreakpointResolved(params)); this._webKitConnection.on('Inspector.detached', () => this.terminateSession()); this._webKitConnection.on('close', () => this.terminateSession()); @@ -196,11 +195,22 @@ export class WebKitDebugAdapter implements IDebugAdapter { } private onScriptParsed(script: WebKitProtocol.Debugger.Script): void { - this._scriptsByUrl.set(script.url, script); this._scriptsById.set(script.scriptId, script); this.fireEvent(new Event('scriptParsed', { scriptUrl: script.url })); } + private onBreakpointResolved(params: WebKitProtocol.Debugger.BreakpointResolvedParams): void { + const script = this._scriptsById.get(params.location.scriptId); + if (!script) { + // Breakpoint resolved for a script we don't know about + return; + } + + const committedBps = this._committedBreakpointsByUrl.get(script.url) || []; + committedBps.push(params.breakpointId); + this._committedBreakpointsByUrl.set(script.url, committedBps); + } + public disconnect(): Promise { if (this._chromeProc) { this._chromeProc.kill(); @@ -225,19 +235,22 @@ export class WebKitDebugAdapter implements IDebugAdapter { } public setBreakpoints(args: ISetBreakpointsArgs): Promise { - let targetScript: WebKitProtocol.Debugger.Script; + let targetScriptUrl: string; if (args.source.path) { - targetScript = this._scriptsByUrl.get(args.source.path); + targetScriptUrl = args.source.path; } else if (args.source.sourceReference) { - targetScript = this._scriptsById.get(sourceReferenceToScriptId(args.source.sourceReference)); + const targetScript = this._scriptsById.get(sourceReferenceToScriptId(args.source.sourceReference)); + if (targetScript) { + targetScriptUrl = targetScript.url; + } } - if (targetScript) { + if (targetScriptUrl) { // DebugProtocol sends all current breakpoints for the script. Clear all scripts for the breakpoint then add all of them const setBreakpointsPFailOnError = this._setBreakpointsRequestQ - .then(() => this._clearAllBreakpoints(targetScript.scriptId)) - .then(() => this._addBreakpoints(targetScript.scriptId, args.lines, args.cols)) - .then(responses => ({ breakpoints: this._webkitBreakpointResponsesToODPBreakpoints(targetScript, responses, args.lines) })); + .then(() => this._clearAllBreakpoints(targetScriptUrl)) + .then(() => this._addBreakpoints(targetScriptUrl, args.lines, args.cols)) + .then(responses => ({ breakpoints: this._webkitBreakpointResponsesToODPBreakpoints(targetScriptUrl, responses, args.lines) })); const setBreakpointsPTimeout = Utilities.promiseTimeout(setBreakpointsPFailOnError, /*timeoutMs*/2000, 'Set breakpoints request timed out'); @@ -246,46 +259,50 @@ export class WebKitDebugAdapter implements IDebugAdapter { this._setBreakpointsRequestQ = setBreakpointsPTimeout.catch(() => undefined); return setBreakpointsPTimeout; } else { - // Implies that PathTransformer is out of sync with the scripts here return Promise.reject(`Can't find script for breakpoint request`); } } - private _clearAllBreakpoints(scriptId: WebKitProtocol.Debugger.ScriptId): Promise { - const committedBps = this._committedBreakpointsByScriptId.get(scriptId) || []; + private _clearAllBreakpoints(url: string): Promise { + if (!this._committedBreakpointsByUrl.has(url)) { + return Promise.resolve(); + } // Remove breakpoints one at a time. Seems like it would be ok to send the removes all at once, // 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 committedBps.reduce>((p, bpId) => { + return this._committedBreakpointsByUrl.get(url).reduce((p, bpId) => { return p.then(() => this._webKitConnection.debugger_removeBreakpoint(bpId)).then(() => { }); - }, Promise.resolve()); + }, Promise.resolve()).then(() => { + this._committedBreakpointsByUrl.set(url, null); + }); } - private _addBreakpoints(scriptId: WebKitProtocol.Debugger.ScriptId, lines: number[], cols?: number[]): Promise { + private _addBreakpoints(url: string, lines: number[], cols?: number[]): Promise { // Call setBreakpoint for all breakpoints in the script simultaneously const responsePs = lines - .map((lineNumber, i) => this._webKitConnection.debugger_setBreakpoint({ scriptId: scriptId, lineNumber, columnNumber: cols ? cols[i] : 0 })); + .map((lineNumber, i) => this._webKitConnection.debugger_setBreakpointByUrl(url, lineNumber, cols ? cols[i] : 0)); // Join all setBreakpoint requests to a single promise return Promise.all(responsePs); } - private _webkitBreakpointResponsesToODPBreakpoints(script: WebKitProtocol.Debugger.Script, responses: WebKitProtocol.Debugger.SetBreakpointResponse[], requestLines: number[]): DebugProtocol.Breakpoint[] { - // Ignore errors - const successfulResponses = responses - .filter(response => !response.error); + private _webkitBreakpointResponsesToODPBreakpoints(url: string, responses: WebKitProtocol.Debugger.SetBreakpointByUrlResponse[], requestLines: number[]): DebugProtocol.Breakpoint[] { + // Don't cache errored responses + const committedBpIds = responses + .filter(response => !response.error) + .map(response => response.result.breakpointId); - // Cache breakpoint ids from webkit in committedBreakpoints set - this._committedBreakpointsByScriptId.set(script.scriptId, successfulResponses.map(response => response.result.breakpointId)); + // Cache successfully set breakpoint ids from webkit in committedBreakpoints set + this._committedBreakpointsByUrl.set(url, committedBpIds); // Map committed breakpoints to DebugProtocol response breakpoints const bps = responses .map((response, i) => { // The output list needs to be the same length as the input list, so map errors to // unverified breakpoints. - if (response.error) { + if (response.error || !response.result.locations.length) { return { verified: false, line: requestLines[i], @@ -295,8 +312,8 @@ export class WebKitDebugAdapter implements IDebugAdapter { return { verified: true, - line: response.result.actualLocation.lineNumber, - column: response.result.actualLocation.columnNumber + line: response.result.locations[0].lineNumber, + column: response.result.locations[0].columnNumber }; }); diff --git a/webkit/webKitDebugSession.ts b/webkit/webKitDebugSession.ts index ef92475..753fcfe 100644 --- a/webkit/webKitDebugSession.ts +++ b/webkit/webKitDebugSession.ts @@ -65,12 +65,12 @@ export class WebKitDebugSession extends DebugSession { if (request.command === 'evaluate') { // Errors from evaluate show up in the console or watches pane. Doesn't seem right // as it's not really a failed request. So it doesn't need the tag and worth special casing. - response.message = e.toString(); + response.message = eStr; } else { // These errors show up in the message bar at the top (or nowhere), sometimes not obvious that they // come from the adapter - response.message = '[webkit-debug-adapter] ' + e.toString(); - Logger.log('Error: ' + e.toString()); + response.message = '[webkit-debug-adapter] ' + eStr; + Logger.log('Error: ' + eStr); } response.success = false; diff --git a/webkit/webKitProtocol.d.ts b/webkit/webKitProtocol.d.ts index bb460a8..d170556 100644 --- a/webkit/webKitProtocol.d.ts +++ b/webkit/webKitProtocol.d.ts @@ -45,10 +45,6 @@ declare namespace WebKitProtocol { type: string; } - interface ScriptParsedNotification extends Notification { - params: Script; - } - interface PausedNotificationParams { callFrames: CallFrame[]; // 'exception' or 'other' @@ -57,6 +53,11 @@ declare namespace WebKitProtocol { hitBreakpoints: BreakpointId[]; } + interface BreakpointResolvedParams { + breakpointId: BreakpointId; + location: Location; + } + interface Location { scriptId: ScriptId; lineNumber: number; @@ -75,6 +76,21 @@ declare namespace WebKitProtocol { }; } + interface SetBreakpointByUrlParams { + url?: string; + urlRegex?: string; + lineNumber: number; + columnNumber: number; + condition?: string; + } + + interface SetBreakpointByUrlResponse extends Response { + result: { + breakpointId: BreakpointId; + locations: Location[]; + }; + } + interface RemoveBreakpointParams { breakpointId: BreakpointId; }