Bug 900268 - Trace actor collects too much information when serializing objects; r=fitzgen
authorJake Bailey <rjbailey@mozilla.com>
Fri, 02 Aug 2013 17:26:31 -0700
changeset 141223 f98860de7350c11ce4b3f211e25dfab6f53f99e2
parent 141222 24446c5f75c1c42fab6cc1840c7651e0d33fcb40
child 141224 290389a3cbe7e2f3f7c506018a9449e17c7b9cc1
push id2023
push usernfitzgerald@mozilla.com
push dateSat, 03 Aug 2013 00:26:53 +0000
treeherderfx-team@290389a3cbe7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen
bugs900268
milestone25.0a1
Bug 900268 - Trace actor collects too much information when serializing objects; r=fitzgen
toolkit/devtools/server/actors/tracer.js
toolkit/devtools/server/tests/unit/test_trace_actor-05.js
toolkit/devtools/server/tests/unit/test_trace_actor-06.js
toolkit/devtools/server/tests/unit/test_trace_actor-07.js
toolkit/devtools/server/tests/unit/xpcshell.ini
--- a/toolkit/devtools/server/actors/tracer.js
+++ b/toolkit/devtools/server/actors/tracer.js
@@ -459,21 +459,18 @@ TraceTypes.register("time", TraceTypes.E
 TraceTypes.register("parameterNames", TraceTypes.Events.enterFrame, function({ frame }) {
   return frame.callee ? frame.callee.parameterNames : undefined;
 });
 
 TraceTypes.register("arguments", TraceTypes.Events.enterFrame, function({ frame }) {
   if (!frame.arguments) {
     return undefined;
   }
-  let objectPool = [];
-  let objToId = new Map();
   let args = Array.prototype.slice.call(frame.arguments);
-  let values = args.map(arg => createValueGrip(arg, objectPool, objToId));
-  return { values: values, objectPool: objectPool };
+  return args.map(arg => createValueGrip(arg, true));
 });
 
 TraceTypes.register("return", TraceTypes.Events.exitFrame,
                     serializeCompletionValue.bind(null, "return"));
 
 TraceTypes.register("throw", TraceTypes.Events.exitFrame,
                     serializeCompletionValue.bind(null, "throw"));
 
@@ -510,53 +507,46 @@ function getOffsetColumn(aOffset, aScrip
 /**
  * Returns elapsed time since the given start time.
  */
 function timeSinceTraceStarted({ startTime }) {
   return +new Date - startTime;
 }
 
 /**
- * Creates a pool of object descriptors and a value grip for the given
- * completion value, to be serialized by JSON.stringify.
+ * Creates a value grip for the given completion value, to be
+ * serialized by JSON.stringify.
  *
  * @param aType string
  *        The type of completion value to serialize (return, throw, or yield).
  */
 function serializeCompletionValue(aType, { value }) {
   if (typeof value[aType] === "undefined") {
     return undefined;
   }
-  let objectPool = [];
-  let objToId = new Map();
-  let valueGrip = createValueGrip(value[aType], objectPool, objToId);
-  return { value: valueGrip, objectPool: objectPool };
+  return createValueGrip(value[aType], true);
 }
 
 
 // Serialization helper functions. Largely copied from script.js and modified
 // for use in serialization rather than object actor requests.
 
 /**
- * Create a grip for the given debuggee value. If the value is an object, will
- * create an object descriptor in the given object pool.
+ * Create a grip for the given debuggee value.
  *
  * @param aValue Debugger.Object|primitive
  *        The value to describe with the created grip.
  *
- * @param aPool [ObjectDescriptor]
- *        The pool of objects that may be referenced by |aValue|.
- *
- * @param aObjectToId Map
- *        A map from Debugger.Object instances to indices into the pool.
+ * @param aUseDescriptor boolean
+ *        If true, creates descriptors for objects rather than grips.
  *
  * @return ValueGrip
  *        A primitive value or a grip object.
  */
-function createValueGrip(aValue, aPool, aObjectToId) {
+function createValueGrip(aValue, aUseDescriptor) {
   let type = typeof aValue;
 
   if (type === "string" && aValue.length >= DebuggerServer.LONG_STRING_LENGTH) {
     return {
       type: "longString",
       initial: aValue.substring(0, DebuggerServer.LONG_STRING_INITIAL_LENGTH),
       length: aValue.length
     };
@@ -570,165 +560,163 @@ function createValueGrip(aValue, aPool, 
     return { type: "null" };
   }
 
   if (aValue === undefined) {
     return { type: "undefined" };
   }
 
   if (typeof(aValue) === "object") {
-    createObjectDescriptor(aValue, aPool, aObjectToId);
-    return { type: "object", objectId: aObjectToId.get(aValue) };
+    return aUseDescriptor ? objectDescriptor(aValue) : objectGrip(aValue);
   }
 
   reportException("TraceActor",
                   new Error("Failed to provide a grip for: " + aValue));
   return null;
 }
 
 /**
- * Create an object descriptor in the object pool for the given debuggee object,
- * if it is not already present.
+ * Create a grip for the given debuggee object.
  *
  * @param aObject Debugger.Object
- *        The object to describe with the created descriptor.
- *
- * @param aPool [ObjectDescriptor]
- *        The pool of objects that may be referenced by |aObject|.
- *
- * @param aObjectToId Map
- *        A map from Debugger.Object instances to indices into the pool.
+ *        The object to describe with the created grip.
  */
-function createObjectDescriptor(aObject, aPool, aObjectToId) {
-  if (aObjectToId.has(aObject)) {
-    return;
-  }
-
-  aObjectToId.set(aObject, aPool.length);
-  let desc = Object.create(null);
-  aPool.push(desc);
-
-  // Add properties which object actors include in object grips.
-  desc.class = aObject.class;
-  desc.extensible = aObject.isExtensible();
-  desc.frozen = aObject.isFrozen();
-  desc.sealed = aObject.isSealed();
+function objectGrip(aObject) {
+  let g = {
+    "type": "object",
+    "class": aObject.class,
+    "extensible": aObject.isExtensible(),
+    "frozen": aObject.isFrozen(),
+    "sealed": aObject.isSealed()
+  };
 
   // Add additional properties for functions.
   if (aObject.class === "Function") {
     if (aObject.name) {
-      desc.name = aObject.name;
+      g.name = aObject.name;
     }
     if (aObject.displayName) {
-      desc.displayName = aObject.displayName;
+      g.displayName = aObject.displayName;
     }
 
     // Check if the developer has added a de-facto standard displayName
     // property for us to use.
     let name = aObject.getOwnPropertyDescriptor("displayName");
     if (name && name.value && typeof name.value == "string") {
-      desc.userDisplayName = createValueGrip(name.value, aObject, aPool, aObjectToId);
+      g.userDisplayName = createValueGrip(name.value, aObject);
+    }
+
+    // Add source location information.
+    if (aObject.script) {
+      g.url = aObject.script.url;
+      g.line = aObject.script.startLine;
     }
   }
 
+  return g;
+}
+
+/**
+ * Create a descriptor for the given debuggee object. Descriptors are
+ * identical to grips, with the addition of the prototype,
+ * ownProperties, and safeGetterValues properties.
+ *
+ * @param aObject Debugger.Object
+ *        The object to describe with the created descriptor.
+ */
+function objectDescriptor(aObject) {
+  let desc = objectGrip(aObject);
   let ownProperties = Object.create(null);
-  let propNames;
+  let names;
   try {
-    propNames = aObject.getOwnPropertyNames();
+    names = aObject.getOwnPropertyNames();
   } catch(ex) {
     // The above can throw if aObject points to a dead object.
     // TODO: we should use Cu.isDeadWrapper() - see bug 885800.
     desc.prototype = createValueGrip(null);
     desc.ownProperties = ownProperties;
     desc.safeGetterValues = Object.create(null);
-    return;
+    return desc;
+  }
+  for (let name of names) {
+    ownProperties[name] = propertyDescriptor(name, aObject);
   }
 
-  for (let name of propNames) {
-    ownProperties[name] = createPropertyDescriptor(name, aObject, aPool, aObjectToId);
-  }
+  desc.prototype = createValueGrip(aObject.proto);
+  desc.ownProperties = ownProperties;
+  desc.safeGetterValues = findSafeGetterValues(ownProperties, aObject);
 
-  desc.prototype = createValueGrip(aObject.proto, aPool, aObjectToId);
-  desc.ownProperties = ownProperties;
-  desc.safeGetterValues = findSafeGetterValues(ownProperties, aObject, aPool, aObjectToId);
+  return desc;
 }
 
 /**
  * A helper method that creates a property descriptor for the provided object,
  * properly formatted for sending in a protocol response.
  *
  * @param aName string
  *        The property that the descriptor is generated for.
  *
  * @param aObject Debugger.Object
  *        The object whose property the descriptor is generated for.
  *
- * @param aPool [ObjectDescriptor]
- *        The pool of objects that may be referenced by this property.
- *
- * @param aObjectToId Map
- *        A map from Debugger.Object instances to indices into the pool.
- *
  * @return object
  *         The property descriptor for the property |aName| in |aObject|.
  */
-function createPropertyDescriptor(aName, aObject, aPool, aObjectToId) {
+function propertyDescriptor(aName, aObject) {
   let desc;
   try {
     desc = aObject.getOwnPropertyDescriptor(aName);
   } catch (e) {
     // Calling getOwnPropertyDescriptor on wrapped native prototypes is not
     // allowed (bug 560072). Inform the user with a bogus, but hopefully
     // explanatory, descriptor.
     return {
       configurable: false,
       writable: false,
       enumerable: false,
       value: e.name
     };
   }
 
+  if (!desc) {
+    return undefined;
+  }
+
   let retval = {
     configurable: desc.configurable,
     enumerable: desc.enumerable
   };
 
   if ("value" in desc) {
     retval.writable = desc.writable;
-    retval.value = createValueGrip(desc.value, aPool, aObjectToId);
+    retval.value = createValueGrip(desc.value);
   } else {
     if ("get" in desc) {
-      retval.get = createValueGrip(desc.get, aPool, aObjectToId);
+      retval.get = createValueGrip(desc.get);
     }
     if ("set" in desc) {
-      retval.set = createValueGrip(desc.set, aPool, aObjectToId);
+      retval.set = createValueGrip(desc.set);
     }
   }
   return retval;
 }
 
 /**
  * Find the safe getter values for the given Debugger.Object.
  *
  * @param aOwnProperties object
  *        The object that holds the list of known ownProperties for |aObject|.
  *
  * @param Debugger.Object object
  *        The object to find safe getter values for.
  *
- * @param aPool [ObjectDescriptor]
- *        The pool of objects that may be referenced by |aObject| getters.
- *
- * @param aObjectToId Map
- *        A map from Debugger.Object instances to indices into the pool.
- *
  * @return object
  *         An object that maps property names to safe getter descriptors.
  */
-function findSafeGetterValues(aOwnProperties, aObject, aPool, aObjectToId) {
+function findSafeGetterValues(aOwnProperties, aObject) {
   let safeGetterValues = Object.create(null);
   let obj = aObject;
   let level = 0;
 
   while (obj) {
     let getters = findSafeGetters(obj);
     for (let name of getters) {
       // Avoid overwriting properties from prototypes closer to this.obj. Also
@@ -757,17 +745,17 @@ function findSafeGetterValues(aOwnProper
           getterValue = result.return;
         } else if ("yield" in result) {
           getterValue = result.yield;
         }
         // WebIDL attributes specified with the LenientThis extended attribute
         // return undefined and should be ignored.
         if (getterValue !== undefined) {
           safeGetterValues[name] = {
-            getterValue: createValueGrip(getterValue, aPool, aObjectToId),
+            getterValue: createValueGrip(getterValue),
             getterPrototypeLevel: level,
             enumerable: desc.enumerable,
             writable: level == 0 ? desc.writable : true,
           };
         }
       }
     }
 
--- a/toolkit/devtools/server/tests/unit/test_trace_actor-05.js
+++ b/toolkit/devtools/server/tests/unit/test_trace_actor-05.js
@@ -75,26 +75,26 @@ function test_enter_exit_frame()
                   'foo entry packet should have parameterNames');
       do_check_eq(packets[2].parameterNames.length, 1,
                   'foo should have only one formal parameter');
       do_check_eq(packets[2].parameterNames[0], "x",
                   'foo should have formal parameter "x"');
 
       do_check_eq(typeof packets[2].arguments, "object",
                   'foo entry packet should have arguments');
-      do_check_eq(typeof packets[2].arguments.values, "object",
-                  'foo arguments object should have values array');
-      do_check_eq(packets[2].arguments.values.length, 1,
+      do_check_true(Array.isArray(packets[2].arguments),
+                    'foo entry packet arguments should be an array');
+      do_check_eq(packets[2].arguments.length, 1,
                   'foo should have only one actual parameter');
-      do_check_eq(packets[2].arguments.values[0], 42,
+      do_check_eq(packets[2].arguments[0], 42,
                   'foo should have actual parameter 42');
 
-      do_check_eq(typeof packets[3].return, "object",
+      do_check_eq(typeof packets[3].return, "string",
                   'Fourth packet in sequence should be exit from "foo" frame');
-      do_check_eq(packets[3].return.value, "bar",
+      do_check_eq(packets[3].return, "bar",
                   'foo should return "bar"');
 
       finishClient(gClient);
     });
 }
 
 function start_trace()
 {
--- a/toolkit/devtools/server/tests/unit/test_trace_actor-06.js
+++ b/toolkit/devtools/server/tests/unit/test_trace_actor-06.js
@@ -1,14 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /**
- * Tests that objects, nested objects, and circular references are
- * correctly serialized and sent in exitedFrame packets.
+ * Tests that objects are correctly serialized and sent in exitedFrame
+ * packets.
  */
 
 let {defer} = devtools.require("sdk/core/promise");
 
 var gDebuggee;
 var gClient;
 var gTraceClient;
 
@@ -25,34 +25,61 @@ function run_test()
       });
     });
   });
   do_test_pending();
 }
 
 function test_enter_exit_frame()
 {
-  let packetsSeen = 0;
-
   gTraceClient.addListener("exitedFrame", function(aEvent, aPacket) {
     if (aPacket.sequence === 3) {
-      do_check_eq(typeof aPacket.return, "object",
+      let obj = aPacket.return;
+      do_check_eq(typeof obj, "object",
                   'exitedFrame response should have return value');
-
-      let objPool = aPacket.return.objectPool;
-      let retval = objPool[aPacket.return.value.objectId];
-      let obj = objPool[retval.ownProperties.obj.value.objectId];
+      do_check_eq(typeof obj.prototype, "object",
+                  'return value should have prototype');
+      do_check_eq(typeof obj.ownProperties, "object",
+                  'return value should have ownProperties list');
+      do_check_eq(typeof obj.safeGetterValues, "object",
+                  'return value should have safeGetterValues');
 
-      do_check_eq(retval.ownProperties.num.value, 25);
-      do_check_eq(retval.ownProperties.str.value, "foo");
-      do_check_eq(retval.ownProperties.bool.value, false);
-      do_check_eq(retval.ownProperties.undef.value.type, "undefined");
-      do_check_eq(retval.ownProperties.nil.value.type, "null");
-      do_check_eq(obj.ownProperties.self.value.objectId,
-                  retval.ownProperties.obj.value.objectId);
+      do_check_eq(typeof obj.ownProperties.num, "object",
+                  'return value should have property "num"');
+      do_check_eq(typeof obj.ownProperties.str, "object",
+                  'return value should have property "str"');
+      do_check_eq(typeof obj.ownProperties.bool, "object",
+                  'return value should have property "bool"');
+      do_check_eq(typeof obj.ownProperties.undef, "object",
+                  'return value should have property "undef"');
+      do_check_eq(typeof obj.ownProperties.undef.value, "object",
+                  'return value property "undef" should be a grip');
+      do_check_eq(typeof obj.ownProperties.nil, "object",
+                  'return value should have property "nil"');
+      do_check_eq(typeof obj.ownProperties.nil.value, "object",
+                  'return value property "nil" should be a grip');
+      do_check_eq(typeof obj.ownProperties.obj, "object",
+                  'return value should have property "obj"');
+      do_check_eq(typeof obj.ownProperties.obj.value, "object",
+                  'return value property "obj" should be a grip');
+      do_check_eq(typeof obj.ownProperties.arr, "object",
+                  'return value should have property "arr"');
+      do_check_eq(typeof obj.ownProperties.arr.value, "object",
+                  'return value property "arr" should be a grip');
+
+      do_check_eq(obj.prototype.type, "object");
+      do_check_eq(obj.ownProperties.num.value, 25);
+      do_check_eq(obj.ownProperties.str.value, "foo");
+      do_check_eq(obj.ownProperties.bool.value, false);
+      do_check_eq(obj.ownProperties.undef.value.type, "undefined");
+      do_check_eq(obj.ownProperties.nil.value.type, "null");
+      do_check_eq(obj.ownProperties.obj.value.type, "object");
+      do_check_eq(obj.ownProperties.obj.value.class, "Object");
+      do_check_eq(obj.ownProperties.arr.value.type, "object");
+      do_check_eq(obj.ownProperties.arr.value.class, "Array");
     }
   });
 
   start_trace()
     .then(eval_code)
     .then(stop_trace)
     .then(function() {
       finishClient(gClient);
@@ -65,28 +92,28 @@ function start_trace()
   gTraceClient.startTrace(["return"], null, function() { deferred.resolve(); });
   return deferred.promise;
 }
 
 function eval_code()
 {
   gDebuggee.eval("(" + function() {
     function foo() {
-      let obj = Object.create(null);
+      let obj = {};
       obj.self = obj;
 
-      let retval = Object.create(null);
-      retval.num = 25;
-      retval.str = "foo";
-      retval.bool = false;
-      retval.undef = undefined;
-      retval.nil = null;
-      retval.obj = obj;
-
-      return retval;
+      return {
+        num: 25,
+        str: "foo",
+        bool: false,
+        undef: undefined,
+        nil: null,
+        obj: obj,
+        arr: [1,2,3,4,5]
+      };
     }
     foo();
   } + ")()");
 }
 
 function stop_trace()
 {
   let deferred = defer();
deleted file mode 100644
--- a/toolkit/devtools/server/tests/unit/test_trace_actor-07.js
+++ /dev/null
@@ -1,99 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- http://creativecommons.org/publicdomain/zero/1.0/ */
-
-/**
- * Tests that chained object prototypes are correctly serialized and
- * sent in exitedFrame packets.
- */
-
-let {defer} = devtools.require("sdk/core/promise");
-
-var gDebuggee;
-var gClient;
-var gTraceClient;
-
-function run_test()
-{
-  initTestTracerServer();
-  gDebuggee = addTestGlobal("test-tracer-actor");
-  gClient = new DebuggerClient(DebuggerServer.connectPipe());
-  gClient.connect(function() {
-    attachTestTab(gClient, "test-tracer-actor", function(aResponse, aTabClient) {
-      gClient.attachTracer(aResponse.traceActor, function(aResponse, aTraceClient) {
-        gTraceClient = aTraceClient;
-        test_enter_exit_frame();
-      });
-    });
-  });
-  do_test_pending();
-}
-
-function test_enter_exit_frame()
-{
-  let packetsSeen = 0;
-
-  gTraceClient.addListener("exitedFrame", function(aEvent, aPacket) {
-    if (aPacket.sequence === 3) {
-      do_check_eq(typeof aPacket.return, "object",
-                  'exitedFrame response should have return value');
-
-      let objPool = aPacket.return.objectPool;
-      let obj = objPool[aPacket.return.value.objectId];
-      let propObj = objPool[obj.ownProperties.b.value.objectId];
-      let proto = objPool[obj.prototype.objectId];
-      let protoProto = objPool[proto.prototype.objectId];
-
-      do_check_eq(obj.ownProperties.a.value, 1);
-      do_check_eq(propObj.ownProperties.c.value, "c");
-      do_check_eq(proto.ownProperties.d.value, "foo");
-      do_check_eq(proto.ownProperties.e.value, 42);
-      do_check_eq(protoProto.ownProperties.f.value, 2);
-    }
-  });
-
-  start_trace()
-    .then(eval_code)
-    .then(stop_trace)
-    .then(function() {
-      finishClient(gClient);
-    });
-}
-
-function start_trace()
-{
-  let deferred = defer();
-  gTraceClient.startTrace(["return"], null, function() { deferred.resolve(); });
-  return deferred.promise;
-}
-
-function eval_code()
-{
-  gDebuggee.eval("(" + function() {
-    function foo() {
-      let protoProto = Object.create(null);
-      protoProto.f = 2;
-
-      let proto = Object.create(protoProto);
-      proto.d = "foo";
-      proto.e = 42;
-
-      let obj = Object.create(proto);
-      obj.a = 1;
-
-      let propObj = Object.create(null);
-      propObj.c = "c";
-
-      obj.b = propObj;
-
-      return obj;
-    }
-    foo();
-  } + ")()");
-}
-
-function stop_trace()
-{
-  let deferred = defer();
-  gTraceClient.stopTrace(null, function() { deferred.resolve(); });
-  return deferred.promise;
-}
--- a/toolkit/devtools/server/tests/unit/xpcshell.ini
+++ b/toolkit/devtools/server/tests/unit/xpcshell.ini
@@ -152,9 +152,8 @@ reason = bug 820380
 [test_unsafeDereference.js]
 [test_add_actors.js]
 [test_trace_actor-01.js]
 [test_trace_actor-02.js]
 [test_trace_actor-03.js]
 [test_trace_actor-04.js]
 [test_trace_actor-05.js]
 [test_trace_actor-06.js]
-[test_trace_actor-07.js]