Bug 1520972 - Handle log points in the devtools server, r=jlast.

--HG--
extra : rebase_source : cbff40bc197b6a7cac66bd4698e0059860662a2c

devtools/client/debugger/new/src/client/firefox/commands.js

@@ -187,15 +187,6 @@ function removeXHRBreakpoint(path: string, method: string) {
   return threadClient.removeXHRBreakpoint(path, method);
 }
 
-// Source and breakpoint clients do not yet support an options structure, so
-// for now we transform options into condition strings when setting breakpoints.
-function transformOptionsToCondition(options) {
-  if (options.logValue) {
-    return `console.log(${options.logValue})`;
-  }
-  return options.condition;
-}
-
 function setBreakpoint(
   location: SourceActorLocation,
   options: BreakpointOptions,
@@ -210,7 +201,7 @@ function setBreakpoint(
     .setBreakpoint({
       line: location.line,
       column: location.column,
-      condition: transformOptionsToCondition(options),
+      options,
       noSliding
     })
     .then(([{ actualLocation }, bpClient]) => {
@@ -248,14 +239,18 @@ function setBreakpointOptions(
 ) {
   const id = makeBreakpointActorId(location);
   const bpClient = bpClients[id];
-  delete bpClients[id];
 
-  const sourceThreadClient = bpClient.source._activeThread;
-  return bpClient
-    .setCondition(sourceThreadClient, transformOptionsToCondition(options))
-    .then(_bpClient => {
-      bpClients[id] = _bpClient;
-    });
+  if (debuggerClient.mainRoot.traits.nativeLogpoints) {
+    bpClient.setOptions(options);
+  } else {
+    // Older server breakpoints destroy themselves when changing options.
+    delete bpClients[id];
+    bpClient
+      .setOptions(options)
+      .then(_bpClient => {
+        bpClients[id] = _bpClient;
+      });
+  }
 }
 
 async function evaluateInFrame(script: Script, options: EvaluateParam) {

devtools/client/debugger/new/src/client/firefox/types.js

@@ -398,7 +398,7 @@ export type BreakpointClient = {
     line: number,
     column: ?number
   },
-  setCondition: (ThreadClient, ?string) => Promise<BreakpointClient>,
+  setOptions: (BreakpointOptions) => Promise<BreakpointClient>,
   // request: any,
   source: SourceClient,
   options: BreakpointOptions

devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-cond.js

@@ -72,8 +72,9 @@ add_task(async function() {
   assertEditorBreakpoint(dbg, 5, true);
 
   // Edit the conditional breakpoint set above
+  const bpCondition1 = waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
   await setConditionalBreakpoint(dbg, 5, "2");
-  await waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
+  await bpCondition1;
   bp = findBreakpoint(dbg, "simple2", 5);
   is(bp.options.condition, "12", "breakpoint is created with the condition");
   assertEditorBreakpoint(dbg, 5, true);
@@ -87,19 +88,20 @@ add_task(async function() {
   // Adding a condition to a breakpoint
   clickElement(dbg, "gutter", 5);
   await waitForDispatch(dbg, "ADD_BREAKPOINT");
+  const bpCondition2 = waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
   await setConditionalBreakpoint(dbg, 5, "1");
-  await waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
+  await bpCondition2;
 
   bp = findBreakpoint(dbg, "simple2", 5);
   is(bp.options.condition, "1", "breakpoint is created with the condition");
   assertEditorBreakpoint(dbg, 5, true);
 
-  const bpCondition = waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
+  const bpCondition3 = waitForDispatch(dbg, "SET_BREAKPOINT_OPTIONS");
   //right click breakpoint in breakpoints list
   rightClickElement(dbg, "breakpointItem", 3)
   // select "remove condition";
   selectContextMenuItem(dbg, selectors.breakpointContextMenu.removeCondition);
-  await bpCondition;
+  await bpCondition3;
   bp = findBreakpoint(dbg, "simple2", 5);
   is(bp.options.condition, undefined, "breakpoint condition removed");
 });

devtools/server/actors/breakpoint.js

@@ -22,10 +22,7 @@ const { breakpointSpec } = require("devtools/shared/specs/breakpoint");
  */
 function setBreakpointAtEntryPoints(actor, entryPoints) {
   for (const { script, offsets } of entryPoints) {
-    actor.addScript(script);
-    for (const offset of offsets) {
-      script.setBreakpoint(offset, actor);
-    }
+    actor.addScript(script, offsets);
   }
 }
 
@@ -46,16 +43,25 @@ const BreakpointActor = ActorClassWithSpec(breakpointSpec, {
    *        The generated location of the breakpoint.
    */
   initialize: function(threadActor, generatedLocation) {
-    // The set of Debugger.Script instances that this breakpoint has been set
-    // upon.
-    this.scripts = new Set();
+    // A map from Debugger.Script instances to the offsets which the breakpoint
+    // has been set for in that script.
+    this.scripts = new Map();
 
     this.threadActor = threadActor;
     this.generatedLocation = generatedLocation;
-    this.condition = null;
+    this.options = null;
     this.isPending = true;
   },
 
+  // Called when new breakpoint options are received from the client.
+  setOptions(options) {
+    for (const [script, offsets] of this.scripts) {
+      this._updateOptionsForScript(script, offsets, this.options, options);
+    }
+
+    this.options = options;
+  },
+
   destroy: function() {
     this.removeScripts();
   },
@@ -70,22 +76,58 @@ const BreakpointActor = ActorClassWithSpec(breakpointSpec, {
    *
    * @param script Debugger.Script
    *        The new source script on which the breakpoint has been set.
+   * @param offsets Array
+   *        Any offsets in the script the breakpoint is associated with.
    */
-  addScript: function(script) {
-    this.scripts.add(script);
+  addScript: function(script, offsets) {
+    this.scripts.set(script, offsets.concat(this.scripts.get(offsets) || []));
+    for (const offset of offsets) {
+      script.setBreakpoint(offset, this);
+    }
+
     this.isPending = false;
+    this._updateOptionsForScript(script, offsets, null, this.options);
   },
 
   /**
    * Remove the breakpoints from associated scripts and clear the script cache.
    */
   removeScripts: function() {
-    for (const script of this.scripts) {
+    for (const [script, offsets] of this.scripts) {
+      this._updateOptionsForScript(script, offsets, this.options, null);
       script.clearBreakpoint(this);
     }
     this.scripts.clear();
   },
 
+  // Update any state affected by changing options on a script this breakpoint
+  // is associated with.
+  _updateOptionsForScript(script, offsets, oldOptions, newOptions) {
+    if (this.threadActor.dbg.replaying) {
+      // When replaying, logging breakpoints are handled using an API to get logged
+      // messages from throughout the recording.
+      const oldLogValue = oldOptions && oldOptions.logValue;
+      const newLogValue = newOptions && newOptions.logValue;
+      if (oldLogValue != newLogValue) {
+        for (const offset of offsets) {
+          const { lineNumber, columnNumber } = script.getOffsetLocation(offset);
+          script.replayVirtualConsoleLog(offset, newLogValue, (point, rv) => {
+            const packet = {
+              from: this.actorID,
+              type: "virtualConsoleLog",
+              url: script.url,
+              line: lineNumber,
+              column: columnNumber,
+              executionPoint: point,
+              message: "return" in rv ? "" + rv.return : "" + rv.throw,
+            };
+            this.conn.send(packet);
+          });
+        }
+      }
+    }
+  },
+
   /**
    * Check if this breakpoint has a condition that doesn't error and
    * evaluates to true in frame.
@@ -100,8 +142,8 @@ const BreakpointActor = ActorClassWithSpec(breakpointSpec, {
    *          - message: string
    *            If the condition throws, this is the thrown message.
    */
-  checkCondition: function(frame) {
-    const completion = frame.eval(this.condition);
+  checkCondition: function(frame, condition) {
+    const completion = frame.eval(condition);
     if (completion) {
       if (completion.throw) {
         // The evaluation failed and threw
@@ -162,15 +204,29 @@ const BreakpointActor = ActorClassWithSpec(breakpointSpec, {
     }
 
     const reason = {};
+    const { condition, logValue } = this.options || {};
 
     if (this.threadActor._hiddenBreakpoints.has(this.actorID)) {
       reason.type = "pauseOnDOMEvents";
-    } else if (!this.condition) {
+    } else if (!condition && !logValue) {
       reason.type = "breakpoint";
       // TODO: add the rest of the breakpoints on that line (bug 676602).
       reason.actors = [ this.actorID ];
     } else {
-      const { result, message } = this.checkCondition(frame);
+      // When replaying, breakpoints with log values are handled separately.
+      if (logValue && this.threadActor.dbg.replaying) {
+        return undefined;
+      }
+
+      let condstr = condition;
+      if (logValue) {
+        // In the non-replaying case, log values are handled by treating them as
+        // conditions. console.log() never returns true so we will not pause.
+        condstr = condition
+          ? `(${condition}) && console.log(${logValue})`
+          : `console.log(${logValue})`;
+      }
+      const { result, message } = this.checkCondition(frame, condstr);
 
       if (result) {
         if (!message) {

devtools/server/actors/root.js

@@ -170,6 +170,9 @@ RootActor.prototype = {
     // `front.startProfiler`. This is an optional parameter but it will throw an error if
     // the profiled Firefox doesn't accept it.
     perfActorVersion: 1,
+    // Supports native log points and modifying the condition/log of an existing
+    // breakpoints. Fx66+
+    nativeLogpoints: true,
   },
 
   /**

devtools/server/actors/source.js

@@ -559,8 +559,8 @@ const SourceActor = ActorClassWithSpec(sourceSpec, {
    *        Line to break on.
    * @param Number column
    *        Column to break on.
-   * @param String condition
-   *        A condition which must be true for breakpoint to be hit.
+   * @param Object options
+   *        Any options for the breakpoint.
    * @param Boolean noSliding
    *        If true, disables breakpoint sliding.
    *
@@ -568,11 +568,11 @@ const SourceActor = ActorClassWithSpec(sourceSpec, {
    *          A promise that resolves to a JSON object representing the
    *          response.
    */
-  setBreakpoint: function(line, column, condition, noSliding) {
+  setBreakpoint: function(line, column, options, noSliding) {
     const location = new GeneratedLocation(this, line, column);
     const actor = this._getOrCreateBreakpointActor(
       location,
-      condition,
+      options,
       noSliding
     );
 
@@ -597,16 +597,15 @@ const SourceActor = ActorClassWithSpec(sourceSpec, {
    * @param GeneratedLocation generatedLocation
    *        A GeneratedLocation representing the location of the breakpoint in
    *        the generated source.
-   * @param String condition
-   *        A string that is evaluated whenever the breakpoint is hit. If the
-   *        string evaluates to false, the breakpoint is ignored.
+   * @param Object options
+   *        Any options for the breakpoint.
    * @param Boolean noSliding
    *        If true, disables breakpoint sliding.
    *
    * @returns BreakpointActor
    *          A BreakpointActor representing the breakpoint.
    */
-  _getOrCreateBreakpointActor: function(generatedLocation, condition, noSliding) {
+  _getOrCreateBreakpointActor: function(generatedLocation, options, noSliding) {
     let actor = this.breakpointActorMap.getActor(generatedLocation);
     if (!actor) {
       actor = new BreakpointActor(this.threadActor, generatedLocation);
@@ -614,7 +613,7 @@ const SourceActor = ActorClassWithSpec(sourceSpec, {
       this.breakpointActorMap.setActor(generatedLocation, actor);
     }
 
-    actor.condition = condition;
+    actor.setOptions(options);
 
     return this._setBreakpoint(actor, noSliding);
   },

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

@@ -27,6 +27,8 @@ function run_test() {
 }
 
 function test_simple_breakpoint() {
+  let hitBreakpoint = false;
+
   gThreadClient.addOneTimeListener("paused", async function(event, packet) {
     const source = await getSourceById(
       gThreadClient,
@@ -34,9 +36,12 @@ function test_simple_breakpoint() {
     );
     source.setBreakpoint({
       line: 3,
-      condition: "a === 1",
+      options: { condition: "a === 1" },
     }).then(function([response, bpClient]) {
       gThreadClient.addOneTimeListener("paused", function(event, packet) {
+        Assert.equal(hitBreakpoint, false);
+        hitBreakpoint = true;
+
         // Check the return value.
         Assert.equal(packet.why.type, "breakpoint");
         Assert.equal(packet.frame.where.line, 3);
@@ -62,4 +67,6 @@ function test_simple_breakpoint() {
                    "test.js",
                    1);
   /* eslint-enable */
+
+  Assert.equal(hitBreakpoint, true);
 }

devtools/server/tests/unit/test_conditional_breakpoint-02.js

@@ -32,13 +32,17 @@ function test_simple_breakpoint() {
       gThreadClient,
       packet.frame.where.actor
     );
-    source.setBreakpoint({
+    await source.setBreakpoint({
       line: 3,
-      condition: "a === 2",
+      options: { condition: "a === 2" },
+    });
+    source.setBreakpoint({
+      line: 4,
+      options: { condition: "a === 1" },
     }).then(function([response, bpClient]) {
       gThreadClient.addOneTimeListener("paused", function(event, packet) {
         // Check the return value.
-        Assert.equal(packet.why.type, "debuggerStatement");
+        Assert.equal(packet.why.type, "breakpoint");
         Assert.equal(packet.frame.where.line, 4);
 
         // Remove the breakpoint.
@@ -57,7 +61,8 @@ function test_simple_breakpoint() {
   Cu.evalInSandbox("debugger;\n" +   // 1
                    "var a = 1;\n" +  // 2
                    "var b = 2;\n" +  // 3
-                   "debugger;",      // 4
+                   "b++;" +          // 4
+                   "debugger;",      // 5
                    gDebuggee,
                    "1.8",
                    "test.js",

devtools/server/tests/unit/test_conditional_breakpoint-03.js

@@ -34,7 +34,7 @@ function test_simple_breakpoint() {
     );
     source.setBreakpoint({
       line: 3,
-      condition: "throw new Error()",
+      options: { condition: "throw new Error()" },
     }).then(function([response, bpClient]) {
       gThreadClient.addOneTimeListener("paused", function(event, packet) {
         // Check the return value.

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

@@ -0,0 +1,60 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* eslint-disable no-shadow, max-nested-callbacks */
+
+"use strict";
+
+/**
+ * Check that logpoints call console.log.
+ */
+
+var gDebuggee;
+var gClient;
+var gThreadClient;
+
+function run_test() {
+  initTestDebuggerServer();
+  gDebuggee = addTestGlobal("test-logpoint");
+  gClient = new DebuggerClient(DebuggerServer.connectPipe());
+  gClient.connect().then(function() {
+    attachTestTabAndResume(gClient, "test-logpoint",
+                           function(response, targetFront, threadClient) {
+                             gThreadClient = threadClient;
+                             test_simple_breakpoint();
+                           });
+  });
+  do_test_pending();
+}
+
+function test_simple_breakpoint() {
+  gThreadClient.addOneTimeListener("paused", async function(event, packet) {
+    const source = await getSourceById(
+      gThreadClient,
+      packet.frame.where.actor
+    );
+
+    // Set a logpoint which should invoke console.log.
+    await source.setBreakpoint({
+      line: 4,
+      options: { logValue: "a" },
+    });
+
+    // Execute the rest of the code.
+    gThreadClient.resume();
+  });
+
+  // Sandboxes don't have a console available so we add our own.
+  /* eslint-disable */
+  Cu.evalInSandbox("var console = { log: v => { this.logValue = v } };\n" + // 1
+                   "debugger;\n" + // 2
+                   "var a = 'three';\n" +  // 3
+                   "var b = 2;\n", // 4
+                   gDebuggee,
+                   "1.8",
+                   "test.js",
+                   1);
+  /* eslint-enable */
+
+  Assert.equal(gDebuggee.logValue, "three");
+  finishClient(gClient);
+}

devtools/server/tests/unit/test_logpoint-02.js

@@ -0,0 +1,62 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* eslint-disable no-shadow, max-nested-callbacks */
+
+"use strict";
+
+/**
+ * Check that conditions are respected when specified in a logpoint.
+ */
+
+var gDebuggee;
+var gClient;
+var gThreadClient;
+
+function run_test() {
+  initTestDebuggerServer();
+  gDebuggee = addTestGlobal("test-logpoint");
+  gClient = new DebuggerClient(DebuggerServer.connectPipe());
+  gClient.connect().then(function() {
+    attachTestTabAndResume(gClient, "test-logpoint",
+                           function(response, targetFront, threadClient) {
+                             gThreadClient = threadClient;
+                             test_simple_breakpoint();
+                           });
+  });
+  do_test_pending();
+}
+
+function test_simple_breakpoint() {
+  gThreadClient.addOneTimeListener("paused", async function(event, packet) {
+    const source = await getSourceById(
+      gThreadClient,
+      packet.frame.where.actor
+    );
+
+    // Set a logpoint which should invoke console.log.
+    await source.setBreakpoint({
+      line: 5,
+      options: { logValue: "a", condition: "a === 5" },
+    });
+
+    // Execute the rest of the code.
+    gThreadClient.resume();
+  });
+
+  // Sandboxes don't have a console available so we add our own.
+  /* eslint-disable */
+  Cu.evalInSandbox("var console = { log: v => { this.logValue = v } };\n" + // 1
+                   "debugger;\n" + // 2
+                   "var a = 1;\n" +  // 3
+                   "while (a < 10) {\n" + // 4
+                   "  a++;\n" + // 5
+                   "}",
+                   gDebuggee,
+                   "1.8",
+                   "test.js",
+                   1);
+  /* eslint-enable */
+
+  Assert.equal(gDebuggee.logValue, 5);
+  finishClient(gClient);
+}

devtools/server/tests/unit/xpcshell.ini

@@ -134,6 +134,8 @@ reason = bug 1104838
 [test_conditional_breakpoint-01.js]
 [test_conditional_breakpoint-02.js]
 [test_conditional_breakpoint-03.js]
+[test_logpoint-01.js]
+[test_logpoint-02.js]
 [test_listsources-01.js]
 [test_listsources-02.js]
 [test_listsources-03.js]

devtools/shared/client/breakpoint-client.js

@@ -7,7 +7,7 @@
 const promise = require("devtools/shared/deprecated-sync-thenables");
 
 const eventSource = require("devtools/shared/client/event-source");
-const {DebuggerClient} = require("devtools/shared/client/debugger-client");
+const {arg, DebuggerClient} = require("devtools/shared/client/debugger-client");
 
 /**
  * Breakpoint clients are used to remove breakpoints that are no longer used.
@@ -21,10 +21,10 @@ const {DebuggerClient} = require("devtools/shared/client/debugger-client");
  * @param location object
  *        The location of the breakpoint. This is an object with two properties:
  *        url and line.
- * @param condition string
- *        The conditional expression of the breakpoint
+ * @param options object
+ *        Any options associated with the breakpoint
  */
-function BreakpointClient(client, sourceClient, actor, location, condition) {
+function BreakpointClient(client, sourceClient, actor, location, options) {
   this._client = client;
   this._actor = actor;
   this.location = location;
@@ -32,11 +32,7 @@ function BreakpointClient(client, sourceClient, actor, location, condition) {
   this.location.url = sourceClient.url;
   this.source = sourceClient;
   this.request = this._client.request;
-
-  // The condition property should only exist if it's a truthy value
-  if (condition) {
-    this.condition = condition;
-  }
+  this.options = options;
 }
 
 BreakpointClient.prototype = {
@@ -56,32 +52,46 @@ BreakpointClient.prototype = {
     type: "delete",
   }),
 
+  // Send a setOptions request to newer servers.
+  setOptionsRequester: DebuggerClient.requester({
+    type: "setOptions",
+    options: arg(0),
+  }, {
+    before(packet) {
+      this.options = packet.options;
+      return packet;
+    },
+  }),
+
   /**
-   * Set the condition of this breakpoint
+   * Set any options for this breakpoint.
    */
-  setCondition: function(gThreadClient, condition) {
-    const deferred = promise.defer();
+  setOptions: function(options) {
+    if (this._client.mainRoot.traits.nativeLogpoints) {
+      this.setOptionsRequester(options);
+    } else {
+      // Older servers need to reinstall breakpoints when the condition changes.
+      const deferred = promise.defer();
 
-    const info = {
-      line: this.location.line,
-      column: this.location.column,
-      condition: condition,
-    };
+      const info = {
+        line: this.location.line,
+        column: this.location.column,
+        options,
+      };
 
-    // Remove the current breakpoint and add a new one with the
-    // condition.
-    this.remove(response => {
-      if (response && response.error) {
-        deferred.reject(response);
-        return;
-      }
+      // Remove the current breakpoint and add a new one with the specified
+      // information.
+      this.remove(response => {
+        if (response && response.error) {
+          deferred.reject(response);
+          return;
+        }
 
-      deferred.resolve(this.source.setBreakpoint(info).then(([, newBreakpoint]) => {
-        return newBreakpoint;
-      }));
-    });
-
-    return deferred.promise;
+        deferred.resolve(this.source.setBreakpoint(info).then(([, newBreakpoint]) => {
+          return newBreakpoint;
+        }));
+      });
+    }
   },
 };
 

devtools/shared/client/source-client.js

@@ -175,10 +175,10 @@ SourceClient.prototype = {
    * Request to set a breakpoint in the specified location.
    *
    * @param object location
-   *        The location and condition of the breakpoint in
-   *        the form of { line[, column, condition] }.
+   *        The location and options of the breakpoint in
+   *        the form of { line[, column, options] }.
    */
-  setBreakpoint: function({ line, column, condition, noSliding }) {
+  setBreakpoint: function({ line, column, options, noSliding }) {
     // A helper function that sets the breakpoint.
     const doSetBreakpoint = callback => {
       const location = {
@@ -190,10 +190,23 @@ SourceClient.prototype = {
         to: this.actor,
         type: "setBreakpoint",
         location,
-        condition,
+        options,
         noSliding,
       };
 
+      // Older servers only support conditions, not a more general options
+      // object. Transform the packet to support the older format.
+      if (options && !this._client.mainRoot.traits.nativeLogpoints) {
+        delete packet.options;
+        if (options.logValue) {
+          // Emulate log points by setting a condition with a call to console.log,
+          // which always returns false so the server will never pause.
+          packet.condition = `console.log(${options.logValue})`;
+        } else {
+          packet.condition = options.condition;
+        }
+      }
+
       return this._client.request(packet).then(response => {
         // Ignoring errors, since the user may be setting a breakpoint in a
         // dead script that will reappear on a page reload.
@@ -204,7 +217,7 @@ SourceClient.prototype = {
             this,
             response.actor,
             location,
-            condition
+            options
           );
         }
         if (callback) {

devtools/shared/specs/breakpoint.js

@@ -3,13 +3,19 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-const {generateActorSpec} = require("devtools/shared/protocol");
+const {Arg, generateActorSpec} = require("devtools/shared/protocol");
 
 const breakpointSpec = generateActorSpec({
   typeName: "breakpoint",
 
   methods: {
     delete: {},
+
+    setOptions: {
+      request: {
+        options: Arg(0, "nullable:json"),
+      },
+    },
   },
 });
 

devtools/shared/specs/source.js

@@ -61,7 +61,7 @@ const sourceSpec = generateActorSpec({
           line: Arg(0, "number"),
           column: Arg(1, "nullable:number"),
         },
-        condition: Arg(2, "nullable:string"),
+        options: Arg(2, "nullable:json"),
         noSliding: Arg(3, "nullable:boolean"),
       },
       response: RetVal("json"),