Don't repeat frame arguments in frame locals

Matt Loring · Matt Loring · commit 54c8a97c73c7 · 2016-01-04T19:44:07.000-08:00
It appears that function arguments captured in closures show up as both an argument and a local to v8. This patch stops us from double reporting in such cases. Fixes #59
diff --git a/lib/state.js b/lib/state.js
@@ -237,11 +237,12 @@ StateResolver.prototype.isPathInNodeModulesDirectory_ = function(path) {
 
 
 StateResolver.prototype.resolveFrame_ = function(frame) {
+  var args = this.resolveArgumentList_(frame);
   return {
     function: this.resolveFunctionName_(frame.func()),
     location: this.resolveLocation_(frame),
-    arguments: this.resolveArgumentList_(frame),
-    locals: this.resolveLocalsList_(frame)
+    arguments: args,
+    locals: this.resolveLocalsList_(frame, args)
   };
 };
 
@@ -277,11 +278,21 @@ StateResolver.prototype.resolveArgumentList_ = function(frame) {
 };
 
 
-StateResolver.prototype.resolveLocalsList_ = function(frame) {
+StateResolver.prototype.resolveLocalsList_ = function(frame,
+    resolvedArguments) {
   var locals = [];
+  // Arguments may have been captured as locals in a nested closure.
+  // We filter them out here.
+  var predicate = function(localEntry, argEntry) {
+    return argEntry.varTableIndex === localEntry.varTableIndex;
+  };
   for (var i = 0; i < frame.localCount(); i++) {
-    locals.push(this.resolveVariable_(
+    var localEntry = this.resolveVariable_(
+        frame.localName(i), frame.localValue(i));
+    if (!resolvedArguments.some(predicate.bind(null, localEntry))) {
+      locals.push(this.resolveVariable_(
         frame.localName(i), frame.localValue(i)));
+    }
   }
   return locals;
 };
diff --git a/test/standalone/test-duplicate-expressions.js b/test/standalone/test-duplicate-expressions.js
@@ -0,0 +1,81 @@
+/*1* KEEP THIS CODE AT THE TOP TO AVOID LINE NUMBER CHANGES */
+/*2*/'use strict';
+/*3*/function foo(a) {
+/*4*/  process.nextTick(function() {
+/*5*/    a = 0;
+/*6*/  });
+/*7*/}
+/**
+ * Copyright 2015 Google Inc. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+var breakpointInFoo = {
+  id: 'fake-id-123',
+  location: { path: 'test-duplicate-expressions.js', line: 4 }
+};
+
+var assert = require('assert');
+var v8debugapi = require('../../lib/v8debugapi.js');
+var logModule = require('@google/cloud-diagnostics-common').logger;
+var config = require('../../config.js');
+var scanner = require('../../lib/scanner.js');
+
+function stateIsClean(api) {
+  assert.equal(api.numBreakpoints_(), 0,
+    'there should be no breakpoints active');
+  assert.equal(api.numListeners_(), 0,
+    'there should be no listeners active');
+  return true;
+}
+
+describe('v8debugapi', function() {
+  config.workingDirectory = process.cwd() + '/test/standalone';
+  var logger = logModule.create(config.logLevel);
+  var api = null;
+
+  beforeEach(function(done) {
+    if (!api) {
+      scanner.scan(config.workingDirectory, function(err, hash, fileStats) {
+        assert(!err);
+        api = v8debugapi.create(logger, config, fileStats);
+        assert.ok(api, 'should be able to create the api');
+        done();
+      });
+    } else {
+      assert(stateIsClean(api));
+      done();
+    }
+  });
+  afterEach(function() { assert(stateIsClean(api)); });
+
+  it('should not duplicate expressions', function(done) {
+    api.set(breakpointInFoo, function(err) {
+      assert.ifError(err);
+      api.wait(breakpointInFoo, function(err) {
+        assert.ifError(err);
+        var frames = breakpointInFoo.stackFrames[0];
+        var exprs = frames.arguments.concat(frames.locals);
+        var varTableIndicesSeen = [];
+        exprs.forEach(function(expr) {
+          assert.equal(varTableIndicesSeen.indexOf(expr.varTableIndex), -1);
+          varTableIndicesSeen.push(expr.varTableIndex);
+        });
+        api.clear(breakpointInFoo);
+        done();
+      });
+      process.nextTick(foo);
+    });
+  });
+});
