diff --git a/src/agent/inspectordebugapi.ts b/src/agent/inspectordebugapi.ts index 7d048d12..46fe4d92 100644 --- a/src/agent/inspectordebugapi.ts +++ b/src/agent/inspectordebugapi.ts @@ -33,9 +33,9 @@ import {V8Inspector} from './v8inspector'; export class BreakpointData { constructor( - public id: string, public active: boolean, + public id: inspector.Debugger.BreakpointId, public apiBreakpoint: apiTypes.Breakpoint, - public parsedCondition: estree.Node, + public parsedCondition: estree.Node, public locationStr: string, public compile: null|((src: string) => string)) {} } @@ -46,9 +46,17 @@ export class InspectorDebugApi implements debugapi.DebugApi { breakpoints: {[id: string]: BreakpointData} = {}; sourcemapper: SourceMapper; session: inspector.Session; - scriptmapper: {[id: string]: any} = {}; - + // TODO: listeners, scrpitmapper, location mapper and breakpointmapper can use + // Map in the future after resolving Map.prototype.get(key) returns V or + // undefined. listeners: {[id: string]: utils.Listener} = {}; + // scriptmapper maps scriptId to actual script path. + scriptMapper: {[id: string]: any} = {}; + // locationmapper maps location string to a list of stackdriver breakpoint id. + locationMapper: {[id: string]: apiTypes.BreakpointId[]} = {}; + // breakpointmapper maps v8/inspector breakpoint id to a list of + // stackdriver breakpoint id. + breakpointMapper: {[id: string]: apiTypes.BreakpointId[]} = {}; numBreakpoints = 0; v8Inspector: V8Inspector; constructor( @@ -61,7 +69,7 @@ export class InspectorDebugApi implements debugapi.DebugApi { this.session = new inspector.Session(); this.session.connect(); this.session.on('Debugger.scriptParsed', (script) => { - this.scriptmapper[script.params.scriptId] = script.params; + this.scriptMapper[script.params.scriptId] = script.params; }); this.session.post('Debugger.enable'); this.session.post('Debugger.setBreakpointsActive', {active: true}); @@ -127,11 +135,29 @@ export class InspectorDebugApi implements debugapi.DebugApi { cb, breakpoint, StatusMessage.BREAKPOINT_CONDITION, utils.messages.V8_BREAKPOINT_CLEAR_ERROR); } - const id = breakpoint.id; - const result = this.v8Inspector.removeBreakpoint(breakpointData.id); - const breakpointId = this.breakpoints[id].id; - delete this.breakpoints[id]; - delete this.listeners[breakpointId]; + let locationStr = breakpointData.locationStr; + const v8BreakpointId = breakpointData.id; + + // delete current breakpoint from locationmapper and breakpointmapper. + utils.removeFirstOccurrenceInArray( + this.locationMapper[locationStr], breakpoint.id); + if (this.locationMapper[locationStr].length === 0) { + delete this.locationMapper[locationStr]; + } + utils.removeFirstOccurrenceInArray( + this.breakpointMapper[v8BreakpointId], breakpoint.id); + if (this.breakpointMapper[v8BreakpointId].length === 0) { + delete this.breakpointMapper[v8BreakpointId]; + } + + let result: {error?: Error} = {}; + if (!this.breakpointMapper[breakpointData.id]) { + // When breakpointmapper does not countain current v8/inspector breakpoint + // id, we should remove this breakpoint from v8. + result = this.v8Inspector.removeBreakpoint(breakpointData.id); + } + delete this.breakpoints[breakpoint.id]; + delete this.listeners[breakpoint.id]; this.numBreakpoints--; setImmediate(function() { if (result.error) { @@ -143,10 +169,9 @@ export class InspectorDebugApi implements debugapi.DebugApi { wait(breakpoint: apiTypes.Breakpoint, callback: (err?: Error) => void): void { // TODO: Address the case whree `breakpoint.id` is `null`. - const bp = this.breakpoints[breakpoint.id as string]; const listener = this.onBreakpointHit.bind(this, breakpoint, (err: Error) => { - this.listeners[bp.id].enabled = false; + this.listeners[breakpoint.id].enabled = false; // This method is called from the debug event listener, which // swallows all exception. We defer the callback to make sure // the user errors aren't silenced. @@ -154,14 +179,13 @@ export class InspectorDebugApi implements debugapi.DebugApi { callback(err); }); }); - this.listeners[bp.id] = {enabled: true, listener: listener}; + this.listeners[breakpoint.id] = {enabled: true, listener: listener}; } log(breakpoint: apiTypes.Breakpoint, print: (format: string, exps: string[]) => void, shouldStop: () => boolean): void { // TODO: Address the case whree `breakpoint.id` is `null`. - const bpId = this.breakpoints[breakpoint.id as string].id; let logsThisSecond = 0; let timesliceEnd = Date.now() + 1000; // TODO: Determine why the Error argument is not used. @@ -181,22 +205,22 @@ export class InspectorDebugApi implements debugapi.DebugApi { (obj: any) => JSON.stringify(obj))); logsThisSecond++; if (shouldStop()) { - this.listeners[bpId].enabled = false; + this.listeners[breakpoint.id].enabled = false; } else { if (logsThisSecond >= this.config.log.maxLogsPerSecond) { - this.listeners[bpId].enabled = false; + this.listeners[breakpoint.id].enabled = false; setTimeout(() => { // listeners[num] may have been deleted by `clear` during the // async hop. Make sure it is valid before setting a property // on it. - if (!shouldStop() && this.listeners[bpId]) { - this.listeners[bpId].enabled = true; + if (!shouldStop() && this.listeners[breakpoint.id]) { + this.listeners[breakpoint.id].enabled = true; } }, this.config.log.logDelaySeconds * 1000); } } }); - this.listeners[bpId] = {enabled: true, listener: listener}; + this.listeners[breakpoint.id] = {enabled: true, listener: listener}; } disconnect(): void { @@ -204,6 +228,7 @@ export class InspectorDebugApi implements debugapi.DebugApi { } numBreakpoints_(): number { + // Tracks the number of stackdriver breakpoints. return Object.keys(this.breakpoints).length; } @@ -312,37 +337,68 @@ export class InspectorDebugApi implements debugapi.DebugApi { if (line === 1) { column += debugapi.MODULE_WRAP_PREFIX_LENGTH - 1; } - let condition; - if (breakpoint.condition) condition = breakpoint.condition; - let res = this.v8Inspector.setBreakpointByUrl( - line - 1, matchingScript, undefined, column - 1, condition); - if (res.error || !res.response) { + + let result = + this.setAndStoreBreakpoint(breakpoint, line, column, matchingScript); + if (!result) { return utils.setErrorStatusAndCallback( cb, breakpoint, StatusMessage.BREAKPOINT_SOURCE_LOCATION, utils.messages.V8_BREAKPOINT_ERROR); } - this.breakpoints[breakpoint.id as string] = new BreakpointData( - res.response.breakpointId, true, breakpoint, ast as estree.Program, - compile); + + this.breakpoints[breakpoint.id] = new BreakpointData( + result.v8BreakpointId, breakpoint, ast as estree.Program, + result.locationStr, compile); + this.numBreakpoints++; setImmediate(function() { cb(null); }); // success. } + private setAndStoreBreakpoint( + breakpoint: apiTypes.Breakpoint, line: number, column: number, + matchingScript: string): + {v8BreakpointId: inspector.Debugger.BreakpointId, + locationStr: string}|null { + // location Str will be a JSON string of Stackdriver breakpoint location. + // It will be used as key at locationmapper to ensure there will be no + // duplicate breakpoints at the same location. + const locationStr = JSON.stringify(breakpoint.location); + let v8BreakpointId; // v8/inspector breakpoint id + if (!this.locationMapper[locationStr]) { + // The first time when a breakpoint was set to this location. + + let res = this.v8Inspector.setBreakpointByUrl({ + lineNumber: line - 1, + url: matchingScript, + columnNumber: column - 1, + condition: breakpoint.condition || undefined + }); + if (res.error || !res.response) { + // Error case. + return null; + } + v8BreakpointId = res.response.breakpointId; + this.locationMapper[locationStr] = []; + this.breakpointMapper[v8BreakpointId] = []; + } else { + // Breakpoint found at this location. Acquire the v8/inspector breakpoint + // id. + v8BreakpointId = this.breakpoints[this.locationMapper[locationStr][0]].id; + } + + // Adding current stackdriver breakpoint id to location mapper and + // breakpoint mapper. + this.locationMapper[locationStr].push(breakpoint.id); + this.breakpointMapper[v8BreakpointId].push(breakpoint.id); + + return {v8BreakpointId, locationStr}; + } + private onBreakpointHit( breakpoint: apiTypes.Breakpoint, callback: (err: Error|null) => void, callFrames: inspector.Debugger.CallFrame[]): void { - // TODO: Address the situation where `breakpoint.id` is `null`. - const bp = this.breakpoints[breakpoint.id as string]; - if (!bp.active) { - // Breakpoint exists, but not active. We never disable breakpoints, so - // this is theoretically not possible. Perhaps this is possible if there - // is a second debugger present? Regardless, report the error. - return utils.setErrorStatusAndCallback( - callback, breakpoint, StatusMessage.BREAKPOINT_SOURCE_LOCATION, - utils.messages.V8_BREAKPOINT_DISABLED); - } // Breakpoint Hit const start = process.hrtime(); try { @@ -363,15 +419,14 @@ export class InspectorDebugApi implements debugapi.DebugApi { const expressionErrors: Array = []; const that = this; // TODO: Address the case where `breakpoint.id` is `null`. - if (breakpoint.expressions && - this.breakpoints[breakpoint.id as string].compile) { + if (breakpoint.expressions && this.breakpoints[breakpoint.id].compile) { for (let i = 0; i < breakpoint.expressions.length; i++) { try { // TODO: Address the case where `breakpoint.id` is `null`. breakpoint.expressions[i] = // TODO: Address the case where `compile` is `null`. - (this.breakpoints[breakpoint.id as string].compile as - (text: string) => string)(breakpoint.expressions[i]); + (this.breakpoints[breakpoint.id].compile as (text: string) => + string)(breakpoint.expressions[i]); } catch (e) { this.logger.info( 'Unable to compile watch expression >> ' + @@ -394,17 +449,20 @@ export class InspectorDebugApi implements debugapi.DebugApi { } else { const frame = callFrames[0]; const evaluatedExpressions = breakpoint.expressions.map(function(exp) { - const result = state.evaluate(exp, frame, that.v8Inspector); - // TODO: Address the case where `result.mirror` is `undefined`. - return result.error ? - result.error : - (result.object as inspector.Runtime.RemoteObject).value(); + // returnByValue is set to true here so that the JSON string of the + // value will be returned to log. + const result = state.evaluate(exp, frame, that.v8Inspector, true); + if (result.error) { + return result.error; + } else { + return (result.object as inspector.Runtime.RemoteObject).value; + } }); breakpoint.evaluatedExpressions = evaluatedExpressions; } } else { const captured = state.capture( - callFrames, breakpoint, this.config, this.scriptmapper, + callFrames, breakpoint, this.config, this.scriptMapper, this.v8Inspector); if (breakpoint.location) { breakpoint.location.line = callFrames[0].location.lineNumber + 1; @@ -422,11 +480,13 @@ export class InspectorDebugApi implements debugapi.DebugApi { inspector.Debugger.PausedEventDataType) { try { if (!params.hitBreakpoints) return; - const bpId: string = params.hitBreakpoints[0]; - if (this.listeners[bpId].enabled) { - this.logger.info('>>>breakpoint hit<<< number: ' + bpId); - this.listeners[bpId].listener(params.callFrames); - } + const v8BreakpointId: string = params.hitBreakpoints[0]; + this.breakpointMapper[v8BreakpointId].forEach((id: string) => { + if (this.listeners[id].enabled) { + this.logger.info('>>>breakpoint hit<<< number: ' + id); + this.listeners[id].listener(params.callFrames); + } + }); } catch (e) { this.logger.warn('Internal V8 error on breakpoint event: ' + e); } diff --git a/src/agent/state-inspector.ts b/src/agent/state-inspector.ts index e4e2571e..006eb041 100644 --- a/src/agent/state-inspector.ts +++ b/src/agent/state-inspector.ts @@ -47,7 +47,7 @@ const ARG_LOCAL_LIMIT_MESSAGE_INDEX = 3; */ export function evaluate( expression: string, frame: inspector.Debugger.CallFrame, - v8inspector: V8Inspector): + v8inspector: V8Inspector, returnByValue: boolean): {error: string|null, object?: inspector.Runtime.RemoteObject} { // First validate the expression to make sure it doesn't mutate state const acorn = require('acorn'); @@ -62,7 +62,8 @@ export function evaluate( } // Now actually ask V8 Inspector to evaluate the expression - const result = v8inspector.evaluateOnCallFrame(frame.callFrameId, expression); + const result = v8inspector.evaluateOnCallFrame( + {callFrameId: frame.callFrameId, expression, returnByValue}); if (result.error || !result.response) { return { error: result.error ? String(result.error) : 'no reponse in result' @@ -152,7 +153,7 @@ class StateResolver { if (this.expressions_) { this.expressions_.forEach((expression, index2) => { const result = - evaluate(expression, this.callFrames_[0], this.v8Inspector_); + evaluate(expression, this.callFrames_[0], this.v8Inspector_, false); let evaluated; if (result.error) { evaluated = { @@ -199,6 +200,7 @@ class StateResolver { this.trimVariableTable_(index, frames); } return { + id: this.breakpoint_.id, stackFrames: frames, variableTable: this.resolvedVariableTable_, evaluatedExpressions: this.evaluatedExpressions_ @@ -380,7 +382,7 @@ class StateResolver { count -= 1; for (let i = 0; i < count; ++i) { let result = this.v8Inspector_.getProperties( - frame.scopeChain[i].object.objectId as string); + {objectId: frame.scopeChain[i].object.objectId as string}); // TODO: Handle when result.error exists. if (result.response && !isEmpty(result.response.result)) { for (let j = 0; j < result.response.result.length; ++j) { @@ -488,7 +490,8 @@ class StateResolver { object: inspector.Runtime.RemoteObject, isEvaluated: boolean): apiTypes.Variable { const maxProps = this.config_.capture.maxProperties; - let result = this.v8Inspector_.getProperties(object.objectId as string); + let result = + this.v8Inspector_.getProperties({objectId: object.objectId as string}); let members: Array = []; if (result.error || !result.response) { members.push({ diff --git a/src/agent/state.ts b/src/agent/state.ts index a1dedf11..8a3b53a1 100644 --- a/src/agent/state.ts +++ b/src/agent/state.ts @@ -192,6 +192,7 @@ class StateResolver { return { // TODO (fgao): Add path attribute to avoid explicit cast to // apiTypes.SourceLocation once breakpoint is passed in this class. + id: 'dummy-id', location: {line: this.state_.frame(0).sourceLine() + 1} as apiTypes.SourceLocation, stackFrames: frames, diff --git a/src/agent/utils.ts b/src/agent/utils.ts index f0278720..6036f251 100644 --- a/src/agent/utils.ts +++ b/src/agent/utils.ts @@ -178,3 +178,10 @@ export function getBreakpointCompiler(breakpoint: apiTypes.Breakpoint): return null; } } + +export function removeFirstOccurrenceInArray(array: T[], element: T): void { + const index = array.indexOf(element); + if (index >= 0) { + array.splice(index, 1); + } +} diff --git a/src/agent/v8debugapi.ts b/src/agent/v8debugapi.ts index 0f74055d..e419a1d1 100644 --- a/src/agent/v8debugapi.ts +++ b/src/agent/v8debugapi.ts @@ -165,9 +165,8 @@ export class V8DebugApi implements debugapi.DebugApi { } wait(breakpoint: apiTypes.Breakpoint, callback: (err?: Error) => void): void { - // TODO: Address the case whree `breakpoint.id` is `null`. const that = this; - const num = that.breakpoints[breakpoint.id as string].v8Breakpoint.number(); + const num = that.breakpoints[breakpoint.id].v8Breakpoint.number(); const listener = this.onBreakpointHit.bind(this, breakpoint, function(err: Error) { that.listeners[num].enabled = false; @@ -186,8 +185,7 @@ export class V8DebugApi implements debugapi.DebugApi { print: (format: string, exps: string[]) => void, shouldStop: () => boolean): void { const that = this; - // TODO: Address the case whree `breakpoint.id` is `null`. - const num = that.breakpoints[breakpoint.id as string].v8Breakpoint.number(); + const num = that.breakpoints[breakpoint.id].v8Breakpoint.number(); let logsThisSecond = 0; let timesliceEnd = Date.now() + 1000; // TODO: Determine why the Error argument is not used. @@ -276,7 +274,6 @@ export class V8DebugApi implements debugapi.DebugApi { // debuglet, we are going to assume that repository root === the starting // working directory. let matchingScript; - // TODO: Address the case where `breakpoint.location` is `null`. const scripts = utils.findScripts( mapInfo ? mapInfo.file : path.normalize( @@ -295,7 +292,6 @@ export class V8DebugApi implements debugapi.DebugApi { utils.messages.SOURCE_FILE_AMBIGUOUS); } - // TODO: Address the case where `breakpoint.location` is `null`. // TODO: Address the case where `fileStats[matchingScript]` is `null`. if ((breakpoint.location as apiTypes.SourceLocation).line >= (this.fileStats[matchingScript] as FileStats).lines) { @@ -337,8 +333,7 @@ export class V8DebugApi implements debugapi.DebugApi { this.logger.info('activating v8 breakpoint listener'); this.v8.setListener(this.handleDebugEvents); } - // TODO: Address the case whree `breakpoint.id` is `null`. - this.breakpoints[breakpoint.id as string] = + this.breakpoints[breakpoint.id] = // TODO: Address the case where `ast` is `null`. new V8BreakpointData(breakpoint, v8bp, ast as estree.Program, compile); this.numBreakpoints++; @@ -360,7 +355,7 @@ export class V8DebugApi implements debugapi.DebugApi { breakpoint: apiTypes.Breakpoint, callback: (err: Error|null) => void, execState: v8Types.ExecutionState): void { // TODO: Address the situation where `breakpoint.id` is `null`. - const v8bp = this.breakpoints[breakpoint.id as string].v8Breakpoint; + const v8bp = this.breakpoints[breakpoint.id].v8Breakpoint; if (!v8bp.active()) { // Breakpoint exists, but not active. We never disable breakpoints, so // this is theoretically not possible. Perhaps this is possible if there @@ -421,16 +416,13 @@ export class V8DebugApi implements debugapi.DebugApi { breakpoint: apiTypes.Breakpoint, execState: v8Types.ExecutionState): void { const expressionErrors: Array = []; - // TODO: Address the case where `breakpoint.id` is `null`. - if (breakpoint.expressions && - this.breakpoints[breakpoint.id as string].compile) { + if (breakpoint.expressions && this.breakpoints[breakpoint.id].compile) { for (let i = 0; i < breakpoint.expressions.length; i++) { try { - // TODO: Address the case where `breakpoint.id` is `null`. breakpoint.expressions[i] = // TODO: Address the case where `compile` is `null`. - (this.breakpoints[breakpoint.id as string].compile as - (text: string) => string)(breakpoint.expressions[i]); + (this.breakpoints[breakpoint.id].compile as (text: string) => + string)(breakpoint.expressions[i]); } catch (e) { this.logger.info( 'Unable to compile watch expression >> ' + diff --git a/src/agent/v8inspector.ts b/src/agent/v8inspector.ts index 379712bf..4ab487f0 100644 --- a/src/agent/v8inspector.ts +++ b/src/agent/v8inspector.ts @@ -21,21 +21,14 @@ export class V8Inspector { constructor(session: inspector.Session) { this.session = session; } - setBreakpointByUrl( - lineNumber: number, url?: string, urlRegex?: string, - columnNumber?: number, condition?: string) { + setBreakpointByUrl(options: + inspector.Debugger.SetBreakpointByUrlParameterType) { const result: { error?: Error, response?: inspector.Debugger.SetBreakpointByUrlReturnType } = {}; this.session.post( - 'Debugger.setBreakpointByUrl', { - lineNumber: lineNumber, - url: url, - urlRegex: urlRegex, - columnNumber: columnNumber, - condition: condition - }, + 'Debugger.setBreakpointByUrl', options, (error: Error|null, response: inspector.Debugger.SetBreakpointByUrlReturnType) => { if (error) result.error = error; @@ -54,26 +47,14 @@ export class V8Inspector { return result; } - evaluateOnCallFrame( - callFrameId: string, expression: string, objectGroup?: string, - includeCommandLineAPI?: boolean, silent?: boolean, - returnByValue?: boolean, generatePreview?: boolean, - throwOnSideEffect?: boolean) { + evaluateOnCallFrame(options: + inspector.Debugger.EvaluateOnCallFrameParameterType) { const result: { error?: Error, response?: inspector.Debugger.EvaluateOnCallFrameReturnType } = {}; this.session.post( - 'Debugger.evaluateOnCallFrame', { - callFrameId: callFrameId, - expression: expression, - objectGroup: objectGroup, - includeCommandLineAPI: includeCommandLineAPI, - silent: silent, - returnByValue: returnByValue, - generatePreview: generatePreview, - throwOnSideEffect: throwOnSideEffect - }, + 'Debugger.evaluateOnCallFrame', options, (error: Error|null, response: inspector.Debugger.EvaluateOnCallFrameReturnType) => { if (error) result.error = error; @@ -82,19 +63,12 @@ export class V8Inspector { return result; } - getProperties( - objectId: string, ownProperties?: boolean, - accessorPropertiesOnly?: boolean, generatePreview?: boolean) { + getProperties(options: inspector.Runtime.GetPropertiesParameterType) { const result: {error?: Error, response?: inspector.Runtime.GetPropertiesReturnType} = {}; this.session.post( - 'Runtime.getProperties', { - objectId: objectId, - ownProperties: ownProperties, - accessorPropertiesOnly: accessorPropertiesOnly, - generatePreview: generatePreview - }, + 'Runtime.getProperties', options, (error: Error|null, response: inspector.Runtime.GetPropertiesReturnType) => { if (error) result.error = error; diff --git a/src/types/api-types.ts b/src/types/api-types.ts index 37d3c18a..12e2556d 100644 --- a/src/types/api-types.ts +++ b/src/types/api-types.ts @@ -76,9 +76,7 @@ export interface Breakpoint { evaluatedExpressions: Array; // TODO: Update the code so that `|null` is not needed. variableTable: Array; - // TODO: The `controller.ts` file assumes `id` is not null or undefined. - // Verify it it should be optional. - id?: string; + id: BreakpointId; // TODO: The debug code assumes the rest of these members // are optional. Determine if this is correct. action?: Action; @@ -104,6 +102,8 @@ export interface Breakpoint { }; } +export type BreakpointId = string; + export interface ListBreakpointsQuery { waitToken?: string; successOnTimeout?: boolean;