Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

Commit fa41721

Browse files
gaofanmichaelgaofanmichael
authored
Fix system test break (#333)
1. Resolving breakpoint and logpoint at same location case. 2. Add returnValue to evaluateOnCallframe to convert object to JSON value on logpoint.
1 parent be0f2fd commit fa41721

File tree

7 files changed

+146
-109
lines changed

7 files changed

+146
-109
lines changed

‎src/agent/inspectordebugapi.ts‎

Lines changed: 112 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@ import {V8Inspector} from './v8inspector';
3333

3434
export class BreakpointData {
3535
constructor(
36-
public id: string, public active: boolean,
36+
public id: inspector.Debugger.BreakpointId,
3737
public apiBreakpoint: apiTypes.Breakpoint,
38-
public parsedCondition: estree.Node,
38+
public parsedCondition: estree.Node, public locationStr: string,
3939
public compile: null|((src: string) => string)) {}
4040
}
4141

@@ -46,9 +46,17 @@ export class InspectorDebugApi implements debugapi.DebugApi {
4646
breakpoints: {[id: string]: BreakpointData} = {};
4747
sourcemapper: SourceMapper;
4848
session: inspector.Session;
49-
scriptmapper: {[id: string]: any} = {};
50-
49+
// TODO: listeners, scrpitmapper, location mapper and breakpointmapper can use
50+
// Map in the future after resolving Map.prototype.get(key) returns V or
51+
// undefined.
5152
listeners: {[id: string]: utils.Listener} = {};
53+
// scriptmapper maps scriptId to actual script path.
54+
scriptMapper: {[id: string]: any} = {};
55+
// locationmapper maps location string to a list of stackdriver breakpoint id.
56+
locationMapper: {[id: string]: apiTypes.BreakpointId[]} = {};
57+
// breakpointmapper maps v8/inspector breakpoint id to a list of
58+
// stackdriver breakpoint id.
59+
breakpointMapper: {[id: string]: apiTypes.BreakpointId[]} = {};
5260
numBreakpoints = 0;
5361
v8Inspector: V8Inspector;
5462
constructor(
@@ -61,7 +69,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
6169
this.session = new inspector.Session();
6270
this.session.connect();
6371
this.session.on('Debugger.scriptParsed', (script) => {
64-
this.scriptmapper[script.params.scriptId] = script.params;
72+
this.scriptMapper[script.params.scriptId] = script.params;
6573
});
6674
this.session.post('Debugger.enable');
6775
this.session.post('Debugger.setBreakpointsActive', {active: true});
@@ -127,11 +135,29 @@ export class InspectorDebugApi implements debugapi.DebugApi {
127135
cb, breakpoint, StatusMessage.BREAKPOINT_CONDITION,
128136
utils.messages.V8_BREAKPOINT_CLEAR_ERROR);
129137
}
130-
const id = breakpoint.id;
131-
const result = this.v8Inspector.removeBreakpoint(breakpointData.id);
132-
const breakpointId = this.breakpoints[id].id;
133-
delete this.breakpoints[id];
134-
delete this.listeners[breakpointId];
138+
let locationStr = breakpointData.locationStr;
139+
const v8BreakpointId = breakpointData.id;
140+
141+
// delete current breakpoint from locationmapper and breakpointmapper.
142+
utils.removeFirstOccurrenceInArray(
143+
this.locationMapper[locationStr], breakpoint.id);
144+
if (this.locationMapper[locationStr].length === 0) {
145+
delete this.locationMapper[locationStr];
146+
}
147+
utils.removeFirstOccurrenceInArray(
148+
this.breakpointMapper[v8BreakpointId], breakpoint.id);
149+
if (this.breakpointMapper[v8BreakpointId].length === 0) {
150+
delete this.breakpointMapper[v8BreakpointId];
151+
}
152+
153+
let result: {error?: Error} = {};
154+
if (!this.breakpointMapper[breakpointData.id]) {
155+
// When breakpointmapper does not countain current v8/inspector breakpoint
156+
// id, we should remove this breakpoint from v8.
157+
result = this.v8Inspector.removeBreakpoint(breakpointData.id);
158+
}
159+
delete this.breakpoints[breakpoint.id];
160+
delete this.listeners[breakpoint.id];
135161
this.numBreakpoints--;
136162
setImmediate(function() {
137163
if (result.error) {
@@ -143,25 +169,23 @@ export class InspectorDebugApi implements debugapi.DebugApi {
143169

144170
wait(breakpoint: apiTypes.Breakpoint, callback: (err?: Error) => void): void {
145171
// TODO: Address the case whree `breakpoint.id` is `null`.
146-
const bp = this.breakpoints[breakpoint.id as string];
147172
const listener =
148173
this.onBreakpointHit.bind(this, breakpoint, (err: Error) => {
149-
this.listeners[bp.id].enabled = false;
174+
this.listeners[breakpoint.id].enabled = false;
150175
// This method is called from the debug event listener, which
151176
// swallows all exception. We defer the callback to make sure
152177
// the user errors aren't silenced.
153178
setImmediate(function() {
154179
callback(err);
155180
});
156181
});
157-
this.listeners[bp.id] = {enabled: true, listener: listener};
182+
this.listeners[breakpoint.id] = {enabled: true, listener: listener};
158183
}
159184

160185
log(breakpoint: apiTypes.Breakpoint,
161186
print: (format: string, exps: string[]) => void,
162187
shouldStop: () => boolean): void {
163188
// TODO: Address the case whree `breakpoint.id` is `null`.
164-
const bpId = this.breakpoints[breakpoint.id as string].id;
165189
let logsThisSecond = 0;
166190
let timesliceEnd = Date.now() + 1000;
167191
// TODO: Determine why the Error argument is not used.
@@ -181,29 +205,30 @@ export class InspectorDebugApi implements debugapi.DebugApi {
181205
(obj: any) => JSON.stringify(obj)));
182206
logsThisSecond++;
183207
if (shouldStop()) {
184-
this.listeners[bpId].enabled = false;
208+
this.listeners[breakpoint.id].enabled = false;
185209
} else {
186210
if (logsThisSecond >= this.config.log.maxLogsPerSecond) {
187-
this.listeners[bpId].enabled = false;
211+
this.listeners[breakpoint.id].enabled = false;
188212
setTimeout(() => {
189213
// listeners[num] may have been deleted by `clear` during the
190214
// async hop. Make sure it is valid before setting a property
191215
// on it.
192-
if (!shouldStop() && this.listeners[bpId]) {
193-
this.listeners[bpId].enabled = true;
216+
if (!shouldStop() && this.listeners[breakpoint.id]) {
217+
this.listeners[breakpoint.id].enabled = true;
194218
}
195219
}, this.config.log.logDelaySeconds * 1000);
196220
}
197221
}
198222
});
199-
this.listeners[bpId] = {enabled: true, listener: listener};
223+
this.listeners[breakpoint.id] = {enabled: true, listener: listener};
200224
}
201225

202226
disconnect(): void {
203227
this.session.disconnect();
204228
}
205229

206230
numBreakpoints_(): number {
231+
// Tracks the number of stackdriver breakpoints.
207232
return Object.keys(this.breakpoints).length;
208233
}
209234

@@ -312,37 +337,68 @@ export class InspectorDebugApi implements debugapi.DebugApi {
312337
if (line === 1) {
313338
column += debugapi.MODULE_WRAP_PREFIX_LENGTH - 1;
314339
}
315-
let condition;
316-
if (breakpoint.condition) condition = breakpoint.condition;
317-
let res = this.v8Inspector.setBreakpointByUrl(
318-
line - 1, matchingScript, undefined, column - 1, condition);
319-
if (res.error || !res.response) {
340+
341+
let result =
342+
this.setAndStoreBreakpoint(breakpoint, line, column, matchingScript);
343+
if (!result) {
320344
return utils.setErrorStatusAndCallback(
321345
cb, breakpoint, StatusMessage.BREAKPOINT_SOURCE_LOCATION,
322346
utils.messages.V8_BREAKPOINT_ERROR);
323347
}
324-
this.breakpoints[breakpoint.id as string] = new BreakpointData(
325-
res.response.breakpointId, true, breakpoint, ast as estree.Program,
326-
compile);
348+
349+
this.breakpoints[breakpoint.id] = new BreakpointData(
350+
result.v8BreakpointId, breakpoint, ast as estree.Program,
351+
result.locationStr, compile);
352+
327353
this.numBreakpoints++;
328354
setImmediate(function() {
329355
cb(null);
330356
}); // success.
331357
}
332358

359+
private setAndStoreBreakpoint(
360+
breakpoint: apiTypes.Breakpoint, line: number, column: number,
361+
matchingScript: string):
362+
{v8BreakpointId: inspector.Debugger.BreakpointId,
363+
locationStr: string}|null {
364+
// location Str will be a JSON string of Stackdriver breakpoint location.
365+
// It will be used as key at locationmapper to ensure there will be no
366+
// duplicate breakpoints at the same location.
367+
const locationStr = JSON.stringify(breakpoint.location);
368+
let v8BreakpointId; // v8/inspector breakpoint id
369+
if (!this.locationMapper[locationStr]) {
370+
// The first time when a breakpoint was set to this location.
371+
372+
let res = this.v8Inspector.setBreakpointByUrl({
373+
lineNumber: line - 1,
374+
url: matchingScript,
375+
columnNumber: column - 1,
376+
condition: breakpoint.condition || undefined
377+
});
378+
if (res.error || !res.response) {
379+
// Error case.
380+
return null;
381+
}
382+
v8BreakpointId = res.response.breakpointId;
383+
this.locationMapper[locationStr] = [];
384+
this.breakpointMapper[v8BreakpointId] = [];
385+
} else {
386+
// Breakpoint found at this location. Acquire the v8/inspector breakpoint
387+
// id.
388+
v8BreakpointId = this.breakpoints[this.locationMapper[locationStr][0]].id;
389+
}
390+
391+
// Adding current stackdriver breakpoint id to location mapper and
392+
// breakpoint mapper.
393+
this.locationMapper[locationStr].push(breakpoint.id);
394+
this.breakpointMapper[v8BreakpointId].push(breakpoint.id);
395+
396+
return {v8BreakpointId, locationStr};
397+
}
398+
333399
private onBreakpointHit(
334400
breakpoint: apiTypes.Breakpoint, callback: (err: Error|null) => void,
335401
callFrames: inspector.Debugger.CallFrame[]): void {
336-
// TODO: Address the situation where `breakpoint.id` is `null`.
337-
const bp = this.breakpoints[breakpoint.id as string];
338-
if (!bp.active) {
339-
// Breakpoint exists, but not active. We never disable breakpoints, so
340-
// this is theoretically not possible. Perhaps this is possible if there
341-
// is a second debugger present? Regardless, report the error.
342-
return utils.setErrorStatusAndCallback(
343-
callback, breakpoint, StatusMessage.BREAKPOINT_SOURCE_LOCATION,
344-
utils.messages.V8_BREAKPOINT_DISABLED);
345-
}
346402
// Breakpoint Hit
347403
const start = process.hrtime();
348404
try {
@@ -363,15 +419,14 @@ export class InspectorDebugApi implements debugapi.DebugApi {
363419
const expressionErrors: Array<apiTypes.Variable|null> = [];
364420
const that = this;
365421
// TODO: Address the case where `breakpoint.id` is `null`.
366-
if (breakpoint.expressions &&
367-
this.breakpoints[breakpoint.id as string].compile) {
422+
if (breakpoint.expressions && this.breakpoints[breakpoint.id].compile) {
368423
for (let i = 0; i < breakpoint.expressions.length; i++) {
369424
try {
370425
// TODO: Address the case where `breakpoint.id` is `null`.
371426
breakpoint.expressions[i] =
372427
// TODO: Address the case where `compile` is `null`.
373-
(this.breakpoints[breakpoint.id as string].compile as
374-
(text: string) => string)(breakpoint.expressions[i]);
428+
(this.breakpoints[breakpoint.id].compile as (text: string) =>
429+
string)(breakpoint.expressions[i]);
375430
} catch (e) {
376431
this.logger.info(
377432
'Unable to compile watch expression >> ' +
@@ -394,17 +449,20 @@ export class InspectorDebugApi implements debugapi.DebugApi {
394449
} else {
395450
const frame = callFrames[0];
396451
const evaluatedExpressions = breakpoint.expressions.map(function(exp) {
397-
const result = state.evaluate(exp, frame, that.v8Inspector);
398-
// TODO: Address the case where `result.mirror` is `undefined`.
399-
return result.error ?
400-
result.error :
401-
(result.object as inspector.Runtime.RemoteObject).value();
452+
// returnByValue is set to true here so that the JSON string of the
453+
// value will be returned to log.
454+
const result = state.evaluate(exp, frame, that.v8Inspector, true);
455+
if (result.error) {
456+
return result.error;
457+
} else {
458+
return (result.object as inspector.Runtime.RemoteObject).value;
459+
}
402460
});
403461
breakpoint.evaluatedExpressions = evaluatedExpressions;
404462
}
405463
} else {
406464
const captured = state.capture(
407-
callFrames, breakpoint, this.config, this.scriptmapper,
465+
callFrames, breakpoint, this.config, this.scriptMapper,
408466
this.v8Inspector);
409467
if (breakpoint.location) {
410468
breakpoint.location.line = callFrames[0].location.lineNumber + 1;
@@ -422,11 +480,13 @@ export class InspectorDebugApi implements debugapi.DebugApi {
422480
inspector.Debugger.PausedEventDataType) {
423481
try {
424482
if (!params.hitBreakpoints) return;
425-
const bpId: string = params.hitBreakpoints[0];
426-
if (this.listeners[bpId].enabled) {
427-
this.logger.info('>>>breakpoint hit<<< number: ' + bpId);
428-
this.listeners[bpId].listener(params.callFrames);
429-
}
483+
const v8BreakpointId: string = params.hitBreakpoints[0];
484+
this.breakpointMapper[v8BreakpointId].forEach((id: string) => {
485+
if (this.listeners[id].enabled) {
486+
this.logger.info('>>>breakpoint hit<<< number: ' + id);
487+
this.listeners[id].listener(params.callFrames);
488+
}
489+
});
430490
} catch (e) {
431491
this.logger.warn('Internal V8 error on breakpoint event: ' + e);
432492
}

‎src/agent/state-inspector.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ const ARG_LOCAL_LIMIT_MESSAGE_INDEX = 3;
4747
*/
4848
export function evaluate(
4949
expression: string, frame: inspector.Debugger.CallFrame,
50-
v8inspector: V8Inspector):
50+
v8inspector: V8Inspector, returnByValue: boolean):
5151
{error: string|null, object?: inspector.Runtime.RemoteObject} {
5252
// First validate the expression to make sure it doesn't mutate state
5353
const acorn = require('acorn');
@@ -62,7 +62,8 @@ export function evaluate(
6262
}
6363

6464
// Now actually ask V8 Inspector to evaluate the expression
65-
const result = v8inspector.evaluateOnCallFrame(frame.callFrameId, expression);
65+
const result = v8inspector.evaluateOnCallFrame(
66+
{callFrameId: frame.callFrameId, expression, returnByValue});
6667
if (result.error || !result.response) {
6768
return {
6869
error: result.error ? String(result.error) : 'no reponse in result'
@@ -152,7 +153,7 @@ class StateResolver {
152153
if (this.expressions_) {
153154
this.expressions_.forEach((expression, index2) => {
154155
const result =
155-
evaluate(expression, this.callFrames_[0], this.v8Inspector_);
156+
evaluate(expression, this.callFrames_[0], this.v8Inspector_, false);
156157
let evaluated;
157158
if (result.error) {
158159
evaluated = {
@@ -199,6 +200,7 @@ class StateResolver {
199200
this.trimVariableTable_(index, frames);
200201
}
201202
return {
203+
id: this.breakpoint_.id,
202204
stackFrames: frames,
203205
variableTable: this.resolvedVariableTable_,
204206
evaluatedExpressions: this.evaluatedExpressions_
@@ -380,7 +382,7 @@ class StateResolver {
380382
count -= 1;
381383
for (let i = 0; i < count; ++i) {
382384
let result = this.v8Inspector_.getProperties(
383-
frame.scopeChain[i].object.objectId as string);
385+
{objectId: frame.scopeChain[i].object.objectId as string});
384386
// TODO: Handle when result.error exists.
385387
if (result.response && !isEmpty(result.response.result)) {
386388
for (let j = 0; j < result.response.result.length; ++j) {
@@ -488,7 +490,8 @@ class StateResolver {
488490
object: inspector.Runtime.RemoteObject,
489491
isEvaluated: boolean): apiTypes.Variable {
490492
const maxProps = this.config_.capture.maxProperties;
491-
let result = this.v8Inspector_.getProperties(object.objectId as string);
493+
let result =
494+
this.v8Inspector_.getProperties({objectId: object.objectId as string});
492495
let members: Array<any> = [];
493496
if (result.error || !result.response) {
494497
members.push({

‎src/agent/state.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,7 @@ class StateResolver {
192192
return {
193193
// TODO (fgao): Add path attribute to avoid explicit cast to
194194
// apiTypes.SourceLocation once breakpoint is passed in this class.
195+
id: 'dummy-id',
195196
location: {line: this.state_.frame(0).sourceLine() + 1} as
196197
apiTypes.SourceLocation,
197198
stackFrames: frames,

‎src/agent/utils.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,3 +178,10 @@ export function getBreakpointCompiler(breakpoint: apiTypes.Breakpoint):
178178
return null;
179179
}
180180
}
181+
182+
export function removeFirstOccurrenceInArray<T>(array: T[], element: T): void {
183+
const index = array.indexOf(element);
184+
if (index >= 0) {
185+
array.splice(index, 1);
186+
}
187+
}

0 commit comments

Comments
 (0)