Bug 1602804 - Create WatchpointMap to keep track of objects with watchpoints r=jlast

Differential Revision: https://phabricator.services.mozilla.com/D57645

--HG--
extra : moz-landing-system : lando

devtools/client/debugger/test/mochitest/browser_dbg-watchpoints.js

@@ -2,7 +2,10 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
-// Tests adding a watchpoint
+// - Tests adding a watchpoint
+// - Tests removing a watchpoint
+// - Tests adding a watchpoint, resuming to after the youngest frame has popped,
+// then removing and adding a watchpoint during the same pause
 
 add_task(async function() {
   pushPref("devtools.debugger.features.watchpoints", true);
@@ -13,7 +16,7 @@ add_task(async function() {
   await waitForPaused(dbg);
   const sourceId = findSource(dbg, "doc-watchpoints.html").id;
 
-  info(`Add a get watchpoint at b`);
+  info("Add a get watchpoint at b");
   await toggleScopeNode(dbg, 3);
   const addedWatchpoint = waitForDispatch(dbg, "SET_WATCHPOINT");
   await rightClickScopeNode(dbg, 5);
@@ -23,26 +26,75 @@ add_task(async function() {
   pressKey(dbg, "Escape");
   await addedWatchpoint;
 
-  info(`Resume and wait to pause at the access to b on line 12`);
   resume(dbg);
   await waitForPaused(dbg);
   await waitForState(dbg, () => dbg.selectors.getSelectedInlinePreviews());
-  assertPausedAtSourceAndLine(dbg, sourceId, 11);
+  assertPausedAtSourceAndLine(dbg, sourceId, 17);
 
+  info("Resume and wait to pause at the access to b in the first `obj.b;`");
   resume(dbg);
-
   await waitForPaused(dbg);
-  assertPausedAtSourceAndLine(dbg, sourceId, 13);
-
-  const removedWatchpoint = waitForDispatch(dbg, "REMOVE_WATCHPOINT");
-  const el = await waitForElementWithSelector(dbg, ".remove-get-watchpoint");
-  el.scrollIntoView();
+  assertPausedAtSourceAndLine(dbg, sourceId, 19);
+   
+  info("Remove the get watchpoint on b");
+  const removedWatchpoint1 = waitForDispatch(dbg, "REMOVE_WATCHPOINT");
+  const el1 = await waitForElementWithSelector(dbg, ".remove-get-watchpoint");
+  el1.scrollIntoView();
   clickElementWithSelector(dbg, ".remove-get-watchpoint");
-  await removedWatchpoint;
+  await removedWatchpoint1;
 
+  info("Resume and wait to skip the second `obj.b` and pause on the debugger statement");
+  resume(dbg);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, sourceId, 21);
+   
+  info("Resume and pause on the debugger statement in getB");
+  resume(dbg);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, sourceId, 5);
+
+  info("Add a get watchpoint to b");
+  await toggleScopeNode(dbg, 4);
+  const addedWatchpoint2 = waitForDispatch(dbg, "SET_WATCHPOINT");
+  await rightClickScopeNode(dbg, 6);
+  let dummyA = selectContextMenuItem(dbg, selectors.watchpointsSubmenu);
+  const getWatchpointItem2 = document.querySelector(selectors.addGetWatchpoint);
+  getWatchpointItem2.click();
+  pressKey(dbg, "Escape");
+  await addedWatchpoint2;
+  
+  info("Resume and wait to pause at the access to b in getB");
+  resume(dbg);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, sourceId, 6);
+  
+  info("Resume and pause on the debugger statement");
+  resume(dbg);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, sourceId, 24);
+  
+  info("Remove the get watchpoint on b");
+  const removedWatchpoint2 = waitForDispatch(dbg, "REMOVE_WATCHPOINT");
+  await toggleScopeNode(dbg, 3);
+  await rightClickScopeNode(dbg, 5);
+  const el2 = await waitForElementWithSelector(dbg, ".remove-get-watchpoint");
+  el2.scrollIntoView();
+     clickElementWithSelector(dbg, ".remove-get-watchpoint");
+  await removedWatchpoint2;
+   
+  info("Add back the get watchpoint on b");
+  const addedWatchpoint3 = waitForDispatch(dbg, "SET_WATCHPOINT");
+  await rightClickScopeNode(dbg, 5);
+  selectContextMenuItem(dbg, selectors.watchpointsSubmenu);
+  const getWatchpointItem3 = document.querySelector(selectors.addGetWatchpoint);
+  getWatchpointItem3.click();
+  pressKey(dbg, "Escape");
+  await addedWatchpoint3;
+
+  info("Resume and wait to pause on the final `obj.b;`");
   resume(dbg);
 
   await waitForPaused(dbg);
-  assertPausedAtSourceAndLine(dbg, sourceId, 15);
+  assertPausedAtSourceAndLine(dbg, sourceId, 25);
   await waitForRequestsToSettle(dbg);
 });

devtools/client/debugger/test/mochitest/examples/doc-watchpoints.html

@@ -1,5 +1,11 @@
 <!DOCTYPE html>
 <html>
+  <script>
+    function getB(obj) {
+      debugger;
+      obj.b;
+    }
+  </script>
   <script>
     const obj = { a: { b: 2, c: 3 }, b: 2 };
     debugger;
@@ -13,6 +19,10 @@
     obj.b;
     obj.b;
     debugger;
+
+    getB(obj);
+    debugger;
+    obj.b;
   </script>
 
   <body>

devtools/client/shared/components/reps/reps.js

@@ -8167,26 +8167,12 @@ async function releaseActors(state, client, dispatch) {
     return;
   }
 
-  const watchpoints = getWatchpoints(state);
-  let released = false;
-
-  for (const actor of actors) {
-    // Watchpoints are stored in object actors.
-    // If we release the actor we lose the watchpoint.
-    if (!watchpoints.has(actor)) {
-      await client.releaseActor(actor);
-      released = true;
+  dispatch({
+    type: "RELEASED_ACTORS",
+    data: {
+      actors
     }
-  }
-
-  if (released) {
-    dispatch({
-      type: "RELEASED_ACTORS",
-      data: {
-        actors
-      }
-    });
-  }
+  });
 }
 
 function invokeGetter(node, receiverId) {

devtools/server/actors/object.js

@@ -108,7 +108,6 @@ const proto = {
       incrementGripDepth,
       decrementGripDepth,
     };
-    this._originalDescriptors = new Map();
   },
 
   rawValue: function() {
@@ -116,96 +115,15 @@ const proto = {
   },
 
   addWatchpoint(property, label, watchpointType) {
-    // We promote the object actor to the thread pool
-    // so that it lives for the lifetime of the watchpoint.
-    this.thread.threadObjectGrip(this);
-
-    if (this._originalDescriptors.has(property)) {
-      return;
-    }
-
-    const obj = this.rawValue();
-    const dbgDesc = this.obj.getOwnPropertyDescriptor(property);
-    const desc = Object.getOwnPropertyDescriptor(obj, property) || dbgDesc;
-
-    if (desc.set || desc.get || !desc.configurable) {
-      return;
-    }
-
-    this._originalDescriptors.set(property, { desc, dbgDesc, watchpointType });
-
-    const pauseAndRespond = type => {
-      const frame = this.thread.dbg.getNewestFrame();
-      this.thread._pauseAndRespond(frame, {
-        type: type,
-        message: label,
-      });
-    };
-
-    if (watchpointType === "get") {
-      this.obj.defineProperty(property, {
-        configurable: desc.configurable,
-        enumerable: desc.enumerable,
-        set: this.obj.makeDebuggeeValue(v => {
-          desc.value = v;
-        }),
-        get: this.obj.makeDebuggeeValue(() => {
-          const frame = this.thread.dbg.getNewestFrame();
-
-          if (!this.thread.hasMoved(frame, "getWatchpoint")) {
-            return false;
-          }
-
-          if (!this.thread.skipBreakpoints) {
-            pauseAndRespond("getWatchpoint");
-          }
-
-          return desc.value;
-        }),
-      });
-    }
-
-    if (watchpointType === "set") {
-      this.obj.defineProperty(property, {
-        configurable: desc.configurable,
-        enumerable: desc.enumerable,
-        set: this.obj.makeDebuggeeValue(v => {
-          const frame = this.thread.dbg.getNewestFrame();
-
-          if (!this.thread.hasMoved(frame, "setWatchpoint")) {
-            desc.value = v;
-            return;
-          }
-
-          if (!this.thread.skipBreakpoints) {
-            pauseAndRespond("setWatchpoint");
-          }
-
-          desc.value = v;
-        }),
-        get: this.obj.makeDebuggeeValue(() => {
-          return desc.value;
-        }),
-      });
-    }
+    this.thread.addWatchpoint(this, { property, label, watchpointType });
   },
 
   removeWatchpoint(property) {
-    if (!this._originalDescriptors.has(property)) {
-      return;
-    }
-
-    const { dbgDesc, desc } = this._originalDescriptors.get(property);
-    this._originalDescriptors.delete(property);
-    this.obj.defineProperty(property, { ...dbgDesc, value: desc.value });
-
-    this.thread.demoteObjectGrip(this);
+    this.thread.removeWatchpoint(this, property);
   },
 
   removeWatchpoints() {
-    this._originalDescriptors.forEach((_, property) =>
-      this.removeWatchpoint(property)
-    );
+    this.thread.removeWatchpoint(this);
   },
 
   /**
@@ -861,18 +779,17 @@ const proto = {
       configurable: desc.configurable,
       enumerable: desc.enumerable,
     };
+    const obj = this.rawValue();
 
     if ("value" in desc) {
       retval.writable = desc.writable;
       retval.value = this.hooks.createValueGrip(desc.value);
-    } else if (this._originalDescriptors.has(name.toString())) {
-      name = name.toString();
-      const watchpointType = this._originalDescriptors.get(name).watchpointType;
-      desc = this._originalDescriptors.get(name).desc;
+    } else if (this.thread.getWatchpoint(obj, name.toString())) {
+      const watchpoint = this.thread.getWatchpoint(obj, name.toString());
       retval.value = this.hooks.createValueGrip(
-        this.obj.makeDebuggeeValue(desc.value)
+        this.obj.makeDebuggeeValue(watchpoint.desc.value)
       );
-      retval.watchpoint = watchpointType;
+      retval.watchpoint = watchpoint.watchpointType;
     } else {
       if ("get" in desc) {
         retval.get = this.hooks.createValueGrip(desc.get);
@@ -968,9 +885,7 @@ const proto = {
    * Release the actor, when it isn't needed anymore.
    * Protocol.js uses this release method to call the destroy method.
    */
-  release: function() {
-    this.removeWatchpoints();
-  },
+  release: function() {},
 };
 
 exports.ObjectActor = protocol.ActorClassWithSpec(objectSpec, proto);

devtools/server/actors/thread.js

@@ -18,6 +18,9 @@ const {
   eventBreakpointForNotification,
   makeEventBreakpointMessage,
 } = require("devtools/server/actors/utils/event-breakpoints");
+const {
+  WatchpointMap,
+} = require("devtools/server/actors/utils/watchpoint-map");
 
 const { logEvent } = require("devtools/server/actors/utils/logEvent");
 
@@ -112,6 +115,8 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     this._pauseOverlay = null;
     this._priorPause = null;
 
+    this._watchpointsMap = new WatchpointMap(this);
+
     this._options = {
       autoBlackBox: false,
       skipBreakpoints: false,
@@ -1461,6 +1466,18 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     }
   },
 
+  addWatchpoint(objActor, data) {
+    this._watchpointsMap.add(objActor, data);
+  },
+
+  removeWatchpoint(objActor, property) {
+    this._watchpointsMap.remove(objActor, property);
+  },
+
+  getWatchpoint(obj, property) {
+    return this._watchpointsMap.get(obj, property);
+  },
+
   /**
    * Handle a protocol request to pause the debuggee.
    */
@@ -1779,17 +1796,6 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     this.threadLifetimePool.objectActors.set(actor.obj, actor);
   },
 
-  demoteObjectGrip: function(actor) {
-    // We want to reuse the existing actor ID, so we just remove it from the
-    // current pool's weak map and then let ActorPool.addActor do the rest.
-    actor.registeredPool.objectActors.delete(actor.obj);
-
-    actor.originalRegisteredPool.addActor(actor);
-    actor.originalRegisteredPool.objectActors.set(actor.obj, actor);
-
-    delete actor.originalRegisteredPool;
-  },
-
   _onWindowReady: function({ isTopLevel, isBFCache, window }) {
     if (isTopLevel && this.state != "detached") {
       this.sources.reset();

devtools/server/actors/utils/moz.build

@@ -21,4 +21,5 @@ DevToolsModules(
     'TabSources.js',
     'track-change-emitter.js',
     'walker-search.js',
+    'watchpoint-map.js',
 )

devtools/server/actors/utils/watchpoint-map.js

@@ -0,0 +1,150 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+class WatchpointMap {
+  constructor(thread) {
+    this.thread = thread;
+    this._watchpoints = new Map();
+  }
+
+  setWatchpoint(objActor, data) {
+    const { property, label, watchpointType } = data;
+    const obj = objActor.rawValue();
+
+    if (this.has(obj, property)) {
+      return;
+    }
+
+    const desc =
+      Object.getOwnPropertyDescriptor(obj, property) ||
+      objActor.obj.getOwnPropertyDescriptor(property);
+
+    if (desc.set || desc.get || !desc.configurable) {
+      return;
+    }
+
+    const pauseAndRespond = type => {
+      const frame = this.thread.dbg.getNewestFrame();
+      this.thread._pauseAndRespond(frame, {
+        type: type,
+        message: label,
+      });
+    };
+
+    if (watchpointType === "get") {
+      objActor.obj.defineProperty(property, {
+        configurable: desc.configurable,
+        enumerable: desc.enumerable,
+        set: objActor.obj.makeDebuggeeValue(v => {
+          desc.value = v;
+        }),
+        get: objActor.obj.makeDebuggeeValue(() => {
+          const frame = this.thread.dbg.getNewestFrame();
+
+          if (!this.thread.hasMoved(frame, "getWatchpoint")) {
+            return false;
+          }
+
+          if (!this.thread.skipBreakpoints) {
+            pauseAndRespond("getWatchpoint");
+          }
+
+          return desc.value;
+        }),
+      });
+    }
+
+    if (watchpointType === "set") {
+      objActor.obj.defineProperty(property, {
+        configurable: desc.configurable,
+        enumerable: desc.enumerable,
+        set: objActor.obj.makeDebuggeeValue(v => {
+          const frame = this.thread.dbg.getNewestFrame();
+
+          if (!this.thread.hasMoved(frame, "setWatchpoint")) {
+            desc.value = v;
+            return;
+          }
+
+          if (!this.thread.skipBreakpoints) {
+            pauseAndRespond("setWatchpoint");
+          }
+
+          desc.value = v;
+        }),
+        get: objActor.obj.makeDebuggeeValue(() => {
+          return desc.value;
+        }),
+      });
+    }
+  }
+
+  add(objActor, data) {
+    // Get the object's description before calling setWatchpoint,
+    // otherwise we'll get the modified property descriptor instead
+    const desc = objActor.obj.getOwnPropertyDescriptor(data.property);
+    this.setWatchpoint(objActor, data);
+
+    const objWatchpoints =
+      this._watchpoints.get(objActor.rawValue()) || new Map();
+
+    objWatchpoints.set(data.property, { ...data, desc });
+    this._watchpoints.set(objActor.rawValue(), objWatchpoints);
+  }
+
+  has(obj, property) {
+    const objWatchpoints = this._watchpoints.get(obj);
+    return objWatchpoints && objWatchpoints.has(property);
+  }
+
+  get(obj, property) {
+    const objWatchpoints = this._watchpoints.get(obj);
+    return objWatchpoints && objWatchpoints.get(property);
+  }
+
+  remove(objActor, property) {
+    const obj = objActor.rawValue();
+
+    // This should remove watchpoints on all of the object's properties if
+    // a property isn't passed in as an argument
+    if (!property) {
+      for (const objProperty in obj) {
+        this.remove(objActor, objProperty);
+      }
+    }
+
+    if (!this.has(obj, property)) {
+      return;
+    }
+
+    const objWatchpoints = this._watchpoints.get(obj);
+    const { desc } = objWatchpoints.get(property);
+
+    objWatchpoints.delete(property);
+    this._watchpoints.set(obj, objWatchpoints);
+
+    // We should stop keeping track of an object if it no longer
+    // has a watchpoint
+    if (objWatchpoints.size == 0) {
+      this._watchpoints.delete(obj);
+    }
+
+    objActor.obj.defineProperty(property, desc);
+  }
+
+  removeAll(objActor) {
+    const objWatchpoints = this._watchpoints.get(objActor.rawValue());
+    if (!objWatchpoints) {
+      return;
+    }
+
+    for (const objProperty in objWatchpoints) {
+      this.remove(objActor, objProperty);
+    }
+  }
+}
+
+exports.WatchpointMap = WatchpointMap;

devtools/server/actors/webconsole.js

@@ -550,6 +550,7 @@ const WebConsoleActor = ActorClassWithSpec(webconsoleSpec, {
     const actor = new ObjectActor(
       object,
       {
+        thread: this.parentActor.threadActor,
         getGripDepth: () => this._gripDepth,
         incrementGripDepth: () => this._gripDepth++,
         decrementGripDepth: () => this._gripDepth--,

devtools/server/tests/unit/test_watchpoint-01.js

@@ -50,10 +50,10 @@ async function testSetWatchpoint({ threadFront, debuggee, targetFront }) {
     threadFront
   );
 
-  info("Test that we paused on the debugger statement.");
+  info("Test that we paused on the debugger statement");
   Assert.equal(packet.frame.where.line, 3);
 
-  info("Add set watchpoint.");
+  info("Add set watchpoint");
   const args = packet.frame.arguments;
   const obj = args[0];
   const objClient = threadFront.pauseGrip(obj);
@@ -65,7 +65,7 @@ async function testSetWatchpoint({ threadFront, debuggee, targetFront }) {
   result = await evaluateJS("obj.a.b");
   Assert.equal(result, 1);
 
-  info("Test that watchpoint triggers pause on set.");
+  info("Test that watchpoint triggers pause on set");
   const packet2 = await resumeAndWaitForPause(threadFront);
   Assert.equal(packet2.frame.where.line, 4);
   Assert.equal(packet2.why.type, "setWatchpoint");
@@ -96,7 +96,7 @@ async function testGetWatchpoint({ threadFront, debuggee }) {
     threadFront
   );
 
-  //Test that we paused on the debugger statement.
+  info("Test that we paused on the debugger statement");
   Assert.equal(packet.frame.where.line, 3);
 
   info("Add get watchpoint.");
@@ -138,18 +138,19 @@ async function testRemoveWatchpoint({ threadFront, debuggee }) {
     threadFront
   );
 
-  //Test that we paused on the debugger statement.
+  info(`Test that we paused on the debugger statement`)
   Assert.equal(packet.frame.where.line, 3);
 
-  //Add and then remove set watchpoint.
+  info(`Add set watchpoint`)
   const args = packet.frame.arguments;
   const obj = args[0];
   const objClient = threadFront.pauseGrip(obj);
   await objClient.addWatchpoint("a", "obj.a", "set");
+  
+  info(`Remove set watchpoint`)
   await objClient.removeWatchpoint("a");
 
-  //Test that we do not pause on set.
-
+  info(`Test that we do not pause on set`)
   const packet2 = await resumeAndWaitForPause(threadFront);
   Assert.equal(packet2.frame.where.line, 5);
 
@@ -179,17 +180,17 @@ async function testRemoveWatchpoints({ threadFront, debuggee }) {
     threadFront
   );
 
-  //Test that we paused on the debugger statement.
+  info("Test that we paused on the debugger statement");
   Assert.equal(packet.frame.where.line, 3);
 
-  //Add and then remove set watchpoint.
+  info("Add and then remove set watchpoint")
   const args = packet.frame.arguments;
   const obj = args[0];
   const objClient = threadFront.pauseGrip(obj);
   await objClient.addWatchpoint("a", "obj.a", "set");
   await objClient.removeWatchpoints();
 
-  //Test that we do not pause on set.
+  info("Test that we do not pause on set")
   const packet2 = await resumeAndWaitForPause(threadFront);
   Assert.equal(packet2.frame.where.line, 5);
 

devtools/shared/specs/object.js

@@ -212,20 +212,18 @@ const objectSpec = generateActorSpec({
         label: Arg(1, "string"),
         watchpointType: Arg(2, "string"),
       },
-      response: {},
+      oneway: true,
     },
     removeWatchpoint: {
       request: {
         property: Arg(0, "string"),
       },
-      response: {},
+      oneway: true,
     },
-
     removeWatchpoints: {
       request: {},
-      response: {},
+      oneway: true,
     },
-
     release: { release: true },
     scope: {
       request: {},