fix: Add debugging information for sourcemapper (#977)

* fix: Add debugging information for sourcemapper

* Fix lint issue

* Separate the debugging info related test cases in sourcemapper

* Use match instead of indexOf, use normalize for windows in tests

* Use indexof for windows in test

Author: Louis-Ye

src/agent/config.ts

@@ -359,10 +359,10 @@ export interface ResolvedDebugAgentConfig extends GoogleAuthOptions {
   apiUrl?: string;
 
   /**
-   * Number of times the V8 pauses (could be breakpoint hits) before the
-   * debugging session is reset. This is to release the memory usage held by V8
-   * engine for each breakpoint hit to prevent the memory leak. The default
-   * value is specified in defaultConfig.
+   * Number of times of the V8 breakpoint hits events before resetting the
+   * breakpoints. This is to release the memory usage held by V8 engine for each
+   * breakpoint hit to prevent the memory leak. The default value is specified
+   * in defaultConfig.
    */
   resetV8DebuggerThreshold: number;
 }

src/agent/debuglet.ts

@@ -396,12 +396,13 @@ export class Debuglet extends EventEmitter {
 
     let mapper;
     try {
-      mapper = await SourceMapper.create(findResults.mapFiles);
+      mapper = await SourceMapper.create(findResults.mapFiles, that.logger);
     } catch (err3) {
       that.logger.error('Error processing the sourcemaps.', err3);
       that.emit('initError', err3);
       return;
     }
+
     that.v8debug = debugapi.create(
       that.logger,
       that.config,

src/agent/io/sourcemapper.ts

@@ -18,6 +18,7 @@ import * as path from 'path';
 import {promisify} from 'util';
 import * as sourceMap from 'source-map';
 
+import {Logger} from '../config';
 import {findScriptsFuzzy} from '../util/utils';
 
 const CONCURRENCY = 10;
@@ -167,17 +168,10 @@ async function processSourcemap(
 }
 
 export class SourceMapper {
+  /** Maps each original source path to the corresponding source map info. */
   infoMap: Map<string, MapInfoInput>;
 
-  /**
-   * @param {Array.<string>} sourcemapPaths An array of paths to .map sourcemap
-   *  files that should be processed.  The paths should be relative to the
-   *  current process's current working directory
-   * @param {Logger} logger A logger that reports errors that occurred while
-   *  processing the given sourcemap files
-   * @constructor
-   */
-  constructor() {
+  constructor(readonly logger: Logger) {
     this.infoMap = new Map();
   }
 
@@ -211,6 +205,8 @@ export class SourceMapper {
       Array.from(this.infoMap.keys())
     );
 
+    this.logger.debug(`sourcemapper fuzzy matches: ${matches}`);
+
     if (matches.length === 1) {
       return this.infoMap.get(matches[0]) as MapInfoInput;
     }
@@ -246,6 +242,8 @@ export class SourceMapper {
     colNumber: number,
     entry: MapInfoInput
   ): MapInfoOutput | null {
+    this.logger.debug(`sourcemapper inputPath: ${inputPath}`);
+
     inputPath = path.normalize(inputPath);
 
     const relPath = path
@@ -273,6 +271,8 @@ export class SourceMapper {
       column: colNumber, // to be zero-based
     };
 
+    this.logger.debug(`sourcemapper sourcePos: ${JSON.stringify(sourcePos)}`);
+
     const allPos = entry.mapConsumer.allGeneratedPositionsFor(sourcePos);
     /*
      * Based on testing, it appears that the following code is needed to
@@ -290,6 +290,8 @@ export class SourceMapper {
           })
         : entry.mapConsumer.generatedPositionFor(sourcePos);
 
+    this.logger.debug(`sourcemapper mappedPos: ${JSON.stringify(mappedPos)}`);
+
     return {
       file: entry.outputFile,
       line: (mappedPos.line ?? 0) - 1, // convert the one-based line numbers returned
@@ -305,11 +307,32 @@ export class SourceMapper {
       // output
     };
   }
+
+  /** Prints the debugging information of the source mapper to the logger. */
+  debug() {
+    this.logger.debug('Printing source mapper debugging information ...');
+    for (const [key, value] of this.infoMap) {
+      this.logger.debug(`  source ${key}:`);
+      this.logger.debug(`    outputFile: ${value.outputFile}`);
+      this.logger.debug(`    mapFile: ${value.mapFile}`);
+      this.logger.debug(`    sources: ${value.sources}`);
+    }
+  }
 }
 
-export async function create(sourcemapPaths: string[]): Promise<SourceMapper> {
+/**
+ * @param {Array.<string>} sourcemapPaths An array of paths to .map sourcemap
+ *  files that should be processed.  The paths should be relative to the
+ *  current process's current working directory
+ * @param {Logger} logger A logger that reports errors that occurred while
+ *  processing the given sourcemap files
+ */
+export async function create(
+  sourcemapPaths: string[],
+  logger: Logger
+): Promise<SourceMapper> {
   const limit = pLimit(CONCURRENCY);
-  const mapper = new SourceMapper();
+  const mapper = new SourceMapper(logger);
   const promises = sourcemapPaths.map(path =>
     limit(() => processSourcemap(mapper.infoMap, path))
   );
@@ -320,5 +343,6 @@ export async function create(sourcemapPaths: string[]): Promise<SourceMapper> {
       'An error occurred while processing the sourcemap files' + err
     );
   }
+  mapper.debug();
   return mapper;
 }

src/agent/v8/inspector-debugapi.ts

@@ -34,6 +34,32 @@ import * as utils from '../util/utils';
 import * as debugapi from './debugapi';
 import {V8Inspector} from './v8inspector';
 
+/**
+ * An interface that describes options that set behavior when interacting with
+ * the V8 Inspector API.
+ */
+interface InspectorOptions {
+  /**
+   * Whether to add a 'file://' prefix to a URL when setting breakpoints.
+   */
+  useWellFormattedUrl: boolean;
+}
+
+/** Data related to the v8 inspector. */
+interface V8Data {
+  session: inspector.Session;
+  // Options for behavior when interfacing with the Inspector API.
+  inspectorOptions: InspectorOptions;
+  inspector: V8Inspector;
+  // Store the v8 setBreakpoint parameters for each v8 breakpoint so that later
+  // the recorded parameters can be used to reset the breakpoints.
+  setBreakpointsParams: {
+    [
+      v8BreakpointId: string
+    ]: inspector.Debugger.SetBreakpointByUrlParameterType;
+  };
+}
+
 /**
  * In older versions of Node, the script source as seen by the Inspector
  * backend is wrapped in `require('module').wrapper`, and in new versions
@@ -73,9 +99,8 @@ export class InspectorDebugApi implements debugapi.DebugApi {
   // stackdriver breakpoint id.
   breakpointMapper: {[id: string]: stackdriver.BreakpointId[]} = {};
   numBreakpoints = 0;
-
-  // The wrapper class that interacts with the V8 debugger.
-  inspector: V8Inspector;
+  numBreakpointHitsBeforeReset = 0;
+  v8: V8Data;
 
   constructor(
     logger: consoleLogLevel.Logger,
@@ -87,28 +112,36 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     this.config = config;
     this.fileStats = jsFiles;
     this.sourcemapper = sourcemapper;
-    this.inspector = new V8Inspector(
-      /* logger=*/ logger,
-      /*useWellFormattedUrl=*/ utils.satisfies(process.version, '>10.11.0'),
-      /*resetV8DebuggerThreshold=*/ this.config.resetV8DebuggerThreshold,
-      /*onScriptParsed=*/
-      scriptParams => {
-        this.scriptMapper[scriptParams.scriptId] = scriptParams;
-      },
-      /*onPaused=*/
-      messageParams => {
-        try {
-          this.handleDebugPausedEvent(messageParams);
-        } catch (error) {
-          this.logger.error(error);
-        }
-      }
-    );
+    this.scriptMapper = {};
+    this.v8 = this.createV8Data();
   }
 
-  /** Disconnects and marks the current V8 data as not connected. */
-  disconnect(): void {
-    this.inspector.detach();
+  /** Creates a new V8 Debugging session and the related data. */
+  private createV8Data(): V8Data {
+    const session = new inspector.Session();
+    session.connect();
+    session.on('Debugger.scriptParsed', script => {
+      this.scriptMapper[script.params.scriptId] = script.params;
+    });
+    session.post('Debugger.enable');
+    session.post('Debugger.setBreakpointsActive', {active: true});
+    session.on('Debugger.paused', message => {
+      try {
+        this.handleDebugPausedEvent(message.params);
+      } catch (error) {
+        this.logger.error(error);
+      }
+    });
+
+    return {
+      session,
+      inspectorOptions: {
+        // Well-Formatted URL is required in Node 10.11.1+.
+        useWellFormattedUrl: utils.satisfies(process.version, '>10.11.0'),
+      },
+      inspector: new V8Inspector(session),
+      setBreakpointsParams: {},
+    };
   }
 
   set(
@@ -235,7 +268,8 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     if (!this.breakpointMapper[breakpointData.id]) {
       // When breakpointmapper does not countain current v8/inspector breakpoint
       // id, we should remove this breakpoint from v8.
-      result = this.inspector.removeBreakpoint(breakpointData.id);
+      result = this.v8.inspector.removeBreakpoint(breakpointData.id);
+      delete this.v8.setBreakpointsParams[breakpointData.id];
     }
     delete this.breakpoints[breakpoint.id];
     delete this.listeners[breakpoint.id];
@@ -312,6 +346,10 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     this.listeners[breakpoint.id] = {enabled: true, listener};
   }
 
+  disconnect(): void {
+    this.v8.session.disconnect();
+  }
+
   numBreakpoints_(): number {
     // Tracks the number of stackdriver breakpoints.
     return Object.keys(this.breakpoints).length;
@@ -499,7 +537,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     let v8BreakpointId; // v8/inspector breakpoint id
     if (!this.locationMapper[locationStr]) {
       // The first time when a breakpoint was set to this location.
-      const rawUrl = this.inspector.shouldUseWellFormattedUrl()
+      const rawUrl = this.v8.inspectorOptions.useWellFormattedUrl
         ? `file://${matchingScript}`
         : matchingScript;
       // on windows on Node 11+, the url must start with file:///
@@ -508,17 +546,19 @@ export class InspectorDebugApi implements debugapi.DebugApi {
         process.platform === 'win32' && utils.satisfies(process.version, '>=11')
           ? rawUrl.replace(/^file:\/\//, 'file:///').replace(/\\/g, '/')
           : rawUrl;
-      const res = this.inspector.setBreakpointByUrl({
+      const params = {
         lineNumber: line - 1,
         url,
         columnNumber: column - 1,
         condition: breakpoint.condition || undefined,
-      });
+      };
+      const res = this.v8.inspector.setBreakpointByUrl(params);
       if (res.error || !res.response) {
         // Error case.
         return null;
       }
       v8BreakpointId = res.response.breakpointId;
+      this.v8.setBreakpointsParams[v8BreakpointId] = params;
 
       this.locationMapper[locationStr] = [];
       this.breakpointMapper[v8BreakpointId] = [];
@@ -606,7 +646,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
         const evaluatedExpressions = breakpoint.expressions.map(exp => {
           // 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.inspector, true);
+          const result = state.evaluate(exp, frame, that.v8.inspector, true);
           if (result.error) {
             return result.error;
           } else {
@@ -621,7 +661,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
         breakpoint,
         this.config,
         this.scriptMapper,
-        this.inspector
+        this.v8.inspector
       );
       if (
         breakpoint.location &&
@@ -655,5 +695,37 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     } catch (e) {
       this.logger.warn('Internal V8 error on breakpoint event: ' + e);
     }
+
+    this.tryResetV8Debugger();
+  }
+
+  /**
+   * Periodically resets breakpoints to prevent memory leaks in V8 (for holding
+   * contexts of previous breakpoint hits).
+   */
+  private tryResetV8Debugger() {
+    this.numBreakpointHitsBeforeReset += 1;
+    if (
+      this.numBreakpointHitsBeforeReset < this.config.resetV8DebuggerThreshold
+    ) {
+      return;
+    }
+    this.numBreakpointHitsBeforeReset = 0;
+
+    const storedParams = this.v8.setBreakpointsParams;
+
+    // Re-connect the session to clean the memory usage.
+    this.disconnect();
+    this.scriptMapper = {};
+    this.v8 = this.createV8Data();
+    this.v8.setBreakpointsParams = storedParams;
+
+    // Setting the v8 breakpoints again according to the stored parameters.
+    for (const params of Object.values(storedParams)) {
+      const res = this.v8.inspector.setBreakpointByUrl(params);
+      if (res.error || !res.response) {
+        this.logger.error('Error upon re-setting breakpoint: ' + res);
+      }
+    }
   }
 }