Bug 900268 - Trace actor collects too much information when serializing objects; r=fitzgen

toolkit/devtools/server/actors/tracer.js

@@ -464,11 +464,8 @@ 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,
@@ -515,8 +512,8 @@ function timeSinceTraceStarted({ 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).
@@ -525,10 +522,7 @@ 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);
 }
 
 
@@ -536,22 +530,18 @@ function serializeCompletionValue(aType, { value }) {
 // 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) {
@@ -575,8 +565,7 @@ function createValueGrip(aValue, aPool, aObjectToId) {
   }
 
   if (typeof(aValue) === "object") {
-    createObjectDescriptor(aValue, aPool, aObjectToId);
-    return { type: "object", objectId: aObjectToId.get(aValue) };
+    return aUseDescriptor ? objectDescriptor(aValue) : objectGrip(aValue);
   }
 
   reportException("TraceActor",
@@ -585,70 +574,77 @@ function createValueGrip(aValue, aPool, aObjectToId) {
 }
 
 /**
- * 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, aPool, aObjectToId);
+  desc.prototype = createValueGrip(aObject.proto);
   desc.ownProperties = ownProperties;
-  desc.safeGetterValues = findSafeGetterValues(ownProperties, aObject, aPool, aObjectToId);
+  desc.safeGetterValues = findSafeGetterValues(ownProperties, aObject);
+
+  return desc;
 }
 
 /**
@@ -661,16 +657,10 @@ function createObjectDescriptor(aObject, aPool, aObjectToId) {
  * @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);
@@ -686,6 +676,10 @@ function createPropertyDescriptor(aName, aObject, aPool, aObjectToId) {
     };
   }
 
+  if (!desc) {
+    return undefined;
+  }
+
   let retval = {
     configurable: desc.configurable,
     enumerable: desc.enumerable
@@ -693,13 +687,13 @@ function createPropertyDescriptor(aName, aObject, aPool, aObjectToId) {
 
   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;
@@ -714,16 +708,10 @@ function createPropertyDescriptor(aName, aObject, aPool, aObjectToId) {
  * @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;
@@ -762,7 +750,7 @@ function findSafeGetterValues(aOwnProperties, aObject, aPool, aObjectToId) {
         // 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,

toolkit/devtools/server/tests/unit/test_trace_actor-05.js

@@ -80,16 +80,16 @@ function test_enter_exit_frame()
 
       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);

toolkit/devtools/server/tests/unit/test_trace_actor-06.js

@@ -2,8 +2,8 @@
  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");
@@ -30,24 +30,51 @@ function run_test()
 
 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');
+      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');
 
-      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.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(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(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");
     }
   });
 
@@ -70,18 +97,18 @@ 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();
   } + ")()");

toolkit/devtools/server/tests/unit/test_trace_actor-07.js

@@ -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;
-}

toolkit/devtools/server/tests/unit/xpcshell.ini

@@ -157,4 +157,3 @@ reason = bug 820380
 [test_trace_actor-04.js]
 [test_trace_actor-05.js]
 [test_trace_actor-06.js]
-[test_trace_actor-07.js]