1164880 - Implement Firebug's debug/undebug commands

Status: NEW

(DevTools :: Console, enhancement, P3)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150508094354

Steps to reproduce:

See:
- https://getfirebug.com/wiki/index.php/Debug
- https://getfirebug.com/wiki/index.php/Undebug

Florent

Comment 1

[Florent Fayolle]

I mark this bug as dependent of bug 1050691. We would need to get the location of a function to put a breakpoint on it.

Florent

Comment 2

[Florent Fayolle]

Nick, I see this error returned if we attempt to set a breakpoint while the debuggee is not paused:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L2684-L2687

Also, it looks like the debuggee state is actually set to "running" when evaluating a console command (so when entering "debug(someFunc);" in the WebConsole).

Is it safe to bypass this condition or is there another way to add a breakpoint properly?

Florent

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Is the `debug` special helper function implemented in the server or on the client? Based on our irc chat the other day, I'm assuming server. If so, I think this is a safe operation in the sense that the debuggee is "paused" while we are doing some chrome things even if we don't actually spin up a nested event loop. We clearly aren't running any debuggee JS here.

Are you just making your own fake RDP packet that you pass into onSetBreakpoint? If so, we could perhaps add a second parameter to this function to suppress the state check and just go ahead and set the breakpoint. We don't want to actually pause the debuggee and send pause notifications or else the debugger tab will be switched to on the frontend. The parameter would only be usable by callers already on the server (and presumably who understand what they're doing) since you can't pass a second parameter via the RDP.

Comment 4

[Florent Fayolle]

Attachment: 1164880.patch (WIP)

James, here is my work in progress. Everything almost works fine, I would need some feedback as the patch is already big. 

Here is a list of what is implemented:
 * The breakpoint is set to the back-end, at the line and the column of the first instruction
 * The back-end sends a "breakpointAdded" event to the front-end on |debug()| command
 * The front-end listens to it and just displays a breakpoint at the specified location (no further requests)
 * |DebuggerView.editor.moveBreakpoint| doesn't raise events anymore when called from DebuggerController._onBreakpointAdded (FIXME fixed)
 * When the breakpoint is set, the editor prints the source at the line and the column of the first instruction

I still have this TODO list:
 * Support for SourceMapped sources (question asked below)
 * Feedback in the console output indicating whether the operation succeeded or not (question asked below)
 * undebug() command
 * Tests

Also here is a quick overview of the feature: https://pbs.twimg.com/tweet_video/CG0zBvlXAAAFczD.mp4

Questions
---------

 * SourceMaps: I would need to understand the |OriginalLocation| role here: 
   https://github.com/mozilla/gecko-dev/blob/b83b4f3197c8dc735e47551f33d8f14d505e1524/toolkit/devtools/server/actors/script.js#L2581
   If I set a breakpoint in a PrettyPrinted source (so it is SourceMapped, 
   right?), the function is called with the generated line and column numbers 
   (i.e. the ones that the user see in their Debugger view), and we pass them 
   to the |OriginalLocation| constructor. So I don't understand what this 
   object represents.

 * Feedback in WebConsole: If the user is in the WebConsole panel (and assuming 
   they have previously opened the Debugger panel), I think they would expect 
   some feedback message (for success or error). That's what Firebug does.
   If we want to implement that, how would you localize the messages printed
   in the WebConsole?

Thanks in advance! :)

> Are you just making your own fake RDP packet that you pass into onSetBreakpoint?

Oops, really sorry, Nick to have missed your question. Not really, I rather create a public setBreakpoint method in which we don't check the state of the thread. The onSetBreakpoint do the check and calls setBreakpoint.

BTW, thanks for your answer Nick!

Florent

Comment 5

[Florent Fayolle]

Oh, forgot to mention: it requires the patch of bug 1050691 to be applied.

Florent

Comment 6

[Florent Fayolle]

> I still have this TODO list:

In addition, I noticed a bug I have to fix:
1. Set a breakpoint at the first line of some function (referenced as |someFunc| below)
2. run: |debug(someFunc)| (visually no breakpoint is added, and the breakpoint previously set remains)
3. Trigger someFunc
   => you must resume the debugger twice, as there is two breakpoints for the same line

I suggest that the breakpoint that covers the more instructions wins (i.e. rejects the other one or overrides it). In the example above, the first one would win. 

To do so, probably some modifications should be brought here:
- either at the call site: https://github.com/mozilla/gecko-dev/blob/b83b4f3197c8dc735e47551f33d8f14d505e1524/toolkit/devtools/server/actors/script.js#L2616
- and/or in this method: https://github.com/mozilla/gecko-dev/blob/b83b4f3197c8dc735e47551f33d8f14d505e1524/toolkit/devtools/server/actors/script.js#L61

Florent

Comment 7

[James Long (:jlongster)]

Comment on attachment 8616399 [details] [diff] [review]
1164880.patch (WIP)

Review of attachment 8616399 [details] [diff] [review]:
-----------------------------------------------------------------

This code it pretty fragile (not yours, the existing breakpoint code) so I want to make sure we don't break anything here. If you answer my question below I'll go through it thoroughly.

::: browser/devtools/webconsole/webconsole.js
@@ +3341,5 @@
> +          break;
> +        case "undebugFunction":
> +          // TODO same.
> +          throw new Error("TODO");
> +          break;

I don't understand, where is the code that invokes the breakpoint setting code from the console? You do a lot of work in the debugger to set it up for it but I don't see where it's actually used.

Comment 8

[James Long (:jlongster)]

I'm also in a middle of refactoring all the frontend breakpoint code. I'm not sure if you want to wait (it might be about a month). But I also want to slow down changes in the frontend so it's not hopeless trying to land my refactor. How important is this?

Comment 9

[Florent Fayolle]

> I don't understand, where is the code that invokes the breakpoint setting code from the console?

I am not very sure about what you mean. Here is a draw of what the code basically does:
http://ur1.ca/mrrx5

So there is a step where the webconsoleActor gets the request and evaluates a debug command: the webconsoleActor calls the debuggerActor to set a breakpoint in the server-side.
When it is setup server-side, the webconsoleActor sends an RDP event to notify the existence of the newly created BP to the client, so it updates its view accordingly.

> But I also want to slow down changes in the frontend so it's not hopeless trying to land my refactor. 
> How important is this?

If by important you mean "how big your patch is in the front-end part", here are the stats: 

 % hg qdiff --stat
 browser/devtools/debugger/debugger-controller.js |  222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------
 browser/devtools/debugger/views/sources-view.js  |    3 +-
 browser/devtools/sourceeditor/debugger.js        |   18 ++++++++++------
 browser/devtools/webconsole/test/head.js         |    2 +-
 browser/devtools/webconsole/webconsole.js        |   11 ++++++++++
 toolkit/devtools/client/dbg-client.jsm           |   40 ++++++++++++++++++++++++++++---------
 toolkit/devtools/server/actors/script.js         |   64 +++++++++++++++++++++++++++++++++++++++++++------------------
 toolkit/devtools/webconsole/utils.js             |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Note that I move a lots of code in debugger-controller.js (the callback for |source.setBreakpoint| in |addBreakpoint|) and adapt it a bit. I also need to do the same for |removeBreakpoint|), and listen for the future |breakpointRemoved| RDP event.

Florent

Comment 10

[Florent Fayolle]

Don't hesitate to ping me on IRC if it is easier for you to ask for clarifications.

Florent

Comment 11

[Florent Fayolle]

Attachment: 1164880.patch

WIP with locales support.

TODO: 
- twice-debugger-resumption bug (above)
- source maps (needs understanding code)
- undebug
- tests

Not working (though no exception):
- Browser Toolbox / Browser Content Toolbox (probably TODO in a follow-up)

Florent

Comment 12

[Florent Fayolle]

Comment on attachment 8616399 [details] [diff] [review]
1164880.patch (WIP)

># HG changeset patch
># User Florent Fayolle <[e-mail]>
># Parent  b3ffd646075d816a78254304b668a518e2ba309d
>Bug 1164880 - Implement Firebug's debug/undebug commands
>
>diff --git a/browser/devtools/debugger/debugger-controller.js b/browser/devtools/debugger/debugger-controller.js
>--- a/browser/devtools/debugger/debugger-controller.js
>+++ b/browser/devtools/debugger/debugger-controller.js
>@@ -1,27 +1,31 @@
> /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> /* 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/. */
>+/* global DebuggerView: true, gThreadClient: true, window: true */
> "use strict";
> 
> const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> 
> const DBG_STRINGS_URI = "chrome://browser/locale/devtools/debugger.properties";
> const NEW_SOURCE_IGNORED_URLS = ["debugger eval code", "XStringBundle"];
> const NEW_SOURCE_DISPLAY_DELAY = 200; // ms
> const FETCH_SOURCE_RESPONSE_DELAY = 200; // ms
> const FETCH_EVENT_LISTENERS_DELAY = 200; // ms
> const FRAME_STEP_CLEAR_DELAY = 100; // ms
> const CALL_STACK_PAGE_SIZE = 25; // frames
> 
> // The panel's window global is an EventEmitter firing the following events:
> const EVENTS = {
>+  // When the controller is connected and the thread client available.
>+  CONTROLLER_CONNECTED: "Debugger:ControllerConnected",
>+
>   // When the debugger's source editor instance finishes loading or unloading.
>   EDITOR_LOADED: "Debugger:EditorLoaded",
>   EDITOR_UNLOADED: "Debugger:EditorUnoaded",
> 
>   // When new sources are received from the debugger server.
>   NEW_SOURCE: "Debugger:NewSource",
>   SOURCES_ADDED: "Debugger:SourcesAdded",
> 
>@@ -216,16 +220,17 @@ let DebuggerController = {
>     } else {
>       yield this._startDebuggingTab();
> 
>       if (Prefs.tracerEnabled && traceActor) {
>         yield this._startTracingTab(traceActor);
>       }
>     }
> 
>+    window.emit(EVENTS.CONTROLLER_CONNECTED);
>     this._hideUnsupportedFeatures();
>   }),
> 
>   /**
>    * Disconnects the debugger client and removes event handlers as necessary.
>    */
>   disconnect: function() {
>     // Return early if the client didn't even have a chance to instantiate.
>@@ -1850,57 +1855,78 @@ EventListeners.prototype = {
> };
> 
> /**
>  * Handles all the breakpoints in the current debugger.
>  */
> function Breakpoints() {
>   this._onEditorBreakpointAdd = this._onEditorBreakpointAdd.bind(this);
>   this._onEditorBreakpointRemove = this._onEditorBreakpointRemove.bind(this);
>+  this._onControllerConnected = this._onControllerConnected.bind(this);
>+  this._onBackEndBreakpointAddedEvent = this._onBackEndBreakpointAddedEvent.bind(this);
>   this.addBreakpoint = this.addBreakpoint.bind(this);
>   this.removeBreakpoint = this.removeBreakpoint.bind(this);
> }
> 
> Breakpoints.prototype = {
>   /**
>    * A map of breakpoint promises as tracked by the debugger frontend.
>    * The keys consist of a string representation of the breakpoint location.
>    */
>   _added: new Map(),
>   _removing: new Map(),
>   _disabled: new Map(),
> 
>+  get debuggerClient() {
>+    return DebuggerController.client;
>+  },
>+
>   /**
>    * Adds the source editor breakpoint handlers.
>    *
>    * @return object
>    *         A promise that is resolved when the breakpoints finishes initializing.
>    */
>   initialize: function() {
>     DebuggerView.editor.on("breakpointAdded", this._onEditorBreakpointAdd);
>     DebuggerView.editor.on("breakpointRemoved", this._onEditorBreakpointRemove);
> 
>+    window.once(EVENTS.CONTROLLER_CONNECTED, this._onControllerConnected);
>+
>     // Initialization is synchronous, for now.
>     return promise.resolve(null);
>   },
> 
>   /**
>    * Removes the source editor breakpoint handlers & all the added breakpoints.
>    *
>    * @return object
>    *         A promise that is resolved when the breakpoints finishes destroying.
>    */
>   destroy: function() {
>     DebuggerView.editor.off("breakpointAdded", this._onEditorBreakpointAdd);
>     DebuggerView.editor.off("breakpointRemoved", this._onEditorBreakpointRemove);
> 
>+    if (this.debuggerClient) {
>+      this.debuggerClient.removeListener("breakpointAdded",
>+          this._onBackEndBreakpointAddedEvent);
>+    }
>+
>     return this.removeAllBreakpoints();
>   },
> 
>   /**
>+   * Event handler for when the DebuggerController is connected.
>+   */
>+  _onControllerConnected: function() {
>+    this.debuggerClient.addListener("breakpointAdded",
>+        this._onBackEndBreakpointAddedEvent);
>+  },
>+
>+  /**
>    * Event handler for new breakpoints that come from the editor.
>    *
>    * @param number aLine
>    *        Line number where breakpoint was set.
>    */
>   _onEditorBreakpointAdd: Task.async(function*(_, aLine) {
>     let actor = DebuggerView.Sources.selectedValue;
>     let location = { actor: actor, line: aLine + 1 };
>@@ -1974,17 +2000,17 @@ Breakpoints.prototype = {
>    *
>    * @param object aLocation
>    *        The location where you want the breakpoint.
>    *        This object must have two properties:
>    *          - url: the breakpoint's source location.
>    *          - line: the breakpoint's line number.
>    *        It can also have the following optional properties:
>    *          - condition: only pause if this condition evaluates truthy
>-   * @param object aOptions [optional]
>+   * @param object [aOptions]
>    *        Additional options or flags supported by this operation:
>    *          - openPopup: tells if the expression popup should be shown.
>    *          - noEditorUpdate: tells if you want to skip editor updates.
>    *          - noPaneUpdate: tells if you want to skip breakpoint pane updates.
>    * @return object
>    *         A promise that is resolved after the breakpoint is added, or
>    *         rejected if there was an error.
>    */
>@@ -2012,92 +2038,144 @@ Breakpoints.prototype = {
>     // Remember the breakpoint initialization promise in the store.
>     let identifier = this.getIdentifier(aLocation);
>     this._added.set(identifier, deferred.promise);
> 
>     let source = gThreadClient.source(
>       DebuggerView.Sources.getItemByValue(aLocation.actor).attachment.source
>     );
> 
>-    source.setBreakpoint(aLocation, Task.async(function*(aResponse, aBreakpointClient) {
>-      // If the breakpoint response has an "actualLocation" attached, then
>-      // the original requested placement for the breakpoint wasn't accepted.
>-      let actualLocation = aResponse.actualLocation;
>-      if (actualLocation) {
>-        // Update the editor to reflect the new location of the breakpoint. We
>-        // always need to do this, even when we already have a breakpoint for
>-        // the actual location, because the editor already as already shown the
>-        // breakpoint at the original location at this point. Calling
>-        // moveBreakpoint will hide the breakpoint at the original location, and
>-        // show it at the actual location, if necessary.
>-        //
>-        // FIXME: The call to moveBreakpoint triggers another call to remove-
>-        // and addBreakpoint, respectively. These calls do not have any effect,
>-        // because there is no breakpoint to remove at the old location, and
>-        // the breakpoint is already being added at the new location, but they
>-        // are redundant and confusing.
>-        DebuggerView.editor.moveBreakpoint(
>-          aBreakpointClient.location.line - 1,
>-          actualLocation.line - 1
>-        );
>-
>-        aBreakpointClient.location = actualLocation;
>-        aBreakpointClient.location.actor = actualLocation.source
>-                                         ? actualLocation.source.actor
>-                                         : null;
>-
>-        let oldIdentifier = identifier;
>-        this._added.delete(oldIdentifier);
>-
>-        if ((addedPromise = this._getAdded(actualLocation))) {
>-          deferred.resolve(addedPromise);
>-          return;
>-        }
>-
>-        // Remember the initialization promise for the new location instead.
>-        let newIdentifier = identifier = this.getIdentifier(actualLocation);
>-        this._added.set(newIdentifier, deferred.promise);
>-      }
>-
>-      // By default, new breakpoints are always enabled. Disabled breakpoints
>-      // are, in fact, removed from the server but preserved in the frontend,
>-      // so that they may not be forgotten across target navigations.
>-      let disabledPromise = this._disabled.get(identifier);
>-      if (disabledPromise) {
>-        let aPrevBreakpointClient = yield disabledPromise;
>-        let condition = aPrevBreakpointClient.getCondition();
>-        this._disabled.delete(identifier);
>-
>-        if (condition) {
>-          aBreakpointClient = yield aBreakpointClient.setCondition(
>-            gThreadClient,
>-            condition
>-          );
>-        }
>-      }
>-
>-      // Preserve information about the breakpoint's line text, to display it
>-      // in the sources pane without requiring fetching the source (for example,
>-      // after the target navigated). Note that this will get out of sync
>-      // if the source text contents change.
>-      let line = aBreakpointClient.location.line - 1;
>-      aBreakpointClient.text = DebuggerView.editor.getText(line).trim();
>-
>-      // Show the breakpoint in the breakpoints pane, and resolve.
>-      yield this._showBreakpoint(aBreakpointClient, aOptions);
>-
>-      // Notify that we've added a breakpoint.
>-      window.emit(EVENTS.BREAKPOINT_ADDED, aBreakpointClient);
>-      deferred.resolve(aBreakpointClient);
>-    }.bind(this)));
>+    source.setBreakpoint(aLocation, (...args) =>
>+      this._onBreakpointAdded(...args, deferred, aOptions));
> 
>     return deferred.promise;
>   }),
> 
>   /**
>+   * Callback when the Back-End achieved in setting a breakpoint.
>+   *
>+   * @param {object} aResponse
>+   * @param {BreakpointClient} breakpointClient
>+   * @param {object} [deferred] A deferred promise, resolved representing
>+   *                 the breakpoint and resolved at the end of this callback.
>+   * @param object [aOptions]
>+   *        Additional options or flags supported by this operation:
>+   *          - openPopup: tells if the expression popup should be shown.
>+   *          - noEditorUpdate: tells if you want to skip editor updates.
>+   *          - noPaneUpdate: tells if you want to skip breakpoint pane updates.
>+   */
>+  _onBreakpointAdded: Task.async(function*(response, breakpointClient, deferred, options = {}) {
>+    let identifier = this.getIdentifier(breakpointClient.location);
>+    // If the breakpoint response has an "actualLocation" attached, then
>+    // the original requested placement for the breakpoint wasn't accepted.
>+    let actualLocation = response.actualLocation;
>+    if (actualLocation) {
>+      // Update the editor to reflect the new location of the breakpoint. We
>+      // always need to do this, even when we already have a breakpoint for
>+      // the actual location, because the editor already as already shown the
>+      // breakpoint at the original location at this point. Calling
>+      // moveBreakpoint will hide the breakpoint at the original location, and
>+      // show it at the actual location, if necessary.
>+
>+      DebuggerView.editor.moveBreakpoint(
>+        breakpointClient.location.line - 1,
>+        actualLocation.line - 1,
>+        { noEvent: true }
>+      );
>+
>+      breakpointClient.location = actualLocation;
>+      breakpointClient.location.actor = actualLocation.source
>+                                       ? actualLocation.source.actor
>+                                       : null;
>+
>+      let oldIdentifier = identifier;
>+      this._added.delete(oldIdentifier);
>+
>+      let addedPromise = this._getAdded(actualLocation);
>+      if (addedPromise) {
>+        if (deferred) {
>+          deferred.resolve(addedPromise);
>+        }
>+        return;
>+      }
>+
>+      // Remember the initialization promise for the new location instead.
>+      let newIdentifier = identifier = this.getIdentifier(actualLocation);
>+      this._added.set(newIdentifier, promise.resolve(breakpointClient));
>+    }
>+
>+    // By default, new breakpoints are always enabled. Disabled breakpoints
>+    // are, in fact, removed from the server but preserved in the frontend,
>+    // so that they may not be forgotten across target navigations.
>+    let disabledPromise = this._disabled.get(identifier);
>+    if (disabledPromise) {
>+      let aPrevBreakpointClient = yield disabledPromise;
>+      let condition = aPrevBreakpointClient.getCondition();
>+      this._disabled.delete(identifier);
>+
>+      if (condition) {
>+        breakpointClient = yield breakpointClient.setCondition(
>+          gThreadClient,
>+          condition
>+        );
>+      }
>+    }
>+
>+    // Preserve information about the breakpoint's line text, to display it
>+    // in the sources pane without requiring fetching the source (for example,
>+    // after the target navigated). Note that this will get out of sync
>+    // if the source text contents change.
>+    let line = breakpointClient.location.line - 1;
>+    breakpointClient.text = DebuggerView.editor.getText(line).trim();
>+
>+    // Show the breakpoint in the breakpoints pane, and resolve.
>+    yield this._showBreakpoint(breakpointClient, options);
>+
>+    // Notify that we've added a breakpoint.
>+    window.emit(EVENTS.BREAKPOINT_ADDED, breakpointClient);
>+    if (deferred) {
>+      deferred.resolve(breakpointClient);
>+    }
>+  }),
>+
>+  /**
>+   * Listener for the "breakpointAdded" event from the backend.
>+   * The "breakpointAdded" event indicates that the back-end has setup a
>+   * breakpoint by its own initiative (e.g. after the "debug"
>+   * WebConsole command has been called).
>+   *
>+   * @param {string} eventName
>+   * @param {object} response
>+   */
>+  _onBackEndBreakpointAddedEvent: function(eventName, response) {
>+    let sourceActor = response.actualLocation.source.actor;
>+    let location = response.actualLocation;
>+    let source = gThreadClient.source(
>+      DebuggerView.Sources.getItemByValue(sourceActor).attachment.source
>+    );
>+
>+    // Ignoring errors, since the user may be setting a breakpoint in a
>+    // dead script that will reappear on a page reload.
>+    let bpClient;
>+    if (response.actor) {
>+      bpClient =
>+        source.makeBreakpointClient(response.actor, location);
>+    }
>+
>+    // For some reasons, in order to place a breakpoint visually, the editor
>+    // sometimes needs to open the source where it has to be placed.
>+    // Since |DebuggerView.setEditorLocation| also needs to be called later (in
>+    // order to display the breakpoint), so just open the source.
>+    DebuggerView.setEditorLocation(sourceActor, null,
>+      { noDebug: true }).then(() => {
>+        this._onBreakpointAdded(response, bpClient);
>+      });
>+  },
>+
>+  /**
>    * Remove a breakpoint.
>    *
>    * @param object aLocation
>    *        @see DebuggerController.Breakpoints.addBreakpoint
>    * @param object aOptions [optional]
>    *        @see DebuggerController.Breakpoints.addBreakpoint
>    * @return object
>    *         A promise that is resolved after the breakpoint is removed, or
>diff --git a/browser/devtools/debugger/views/sources-view.js b/browser/devtools/debugger/views/sources-view.js
>--- a/browser/devtools/debugger/views/sources-view.js
>+++ b/browser/devtools/debugger/views/sources-view.js
>@@ -454,17 +454,18 @@ SourcesView.prototype = Heritage.extend(
>       return;
>     }
> 
>     // Breakpoint will now be selected.
>     this._selectBreakpoint(breakpointItem);
> 
>     // Update the editor location if necessary.
>     if (!aOptions.noEditorUpdate) {
>-      this.DebuggerView.setEditorLocation(aLocation.actor, aLocation.line, { noDebug: true });
>+      this.DebuggerView.setEditorLocation(aLocation.actor, aLocation.line,
>+        { noDebug: true, columnOffset: aLocation.column });
>     }
> 
>     // If the breakpoint requires a new conditional expression, display
>     // the panel to input the corresponding expression.
>     if (aOptions.openPopup) {
>       this._openConditionalPopup();
>     } else {
>       this._hideConditionalPopup();
>diff --git a/browser/devtools/sourceeditor/debugger.js b/browser/devtools/sourceeditor/debugger.js
>--- a/browser/devtools/sourceeditor/debugger.js
>+++ b/browser/devtools/sourceeditor/debugger.js
>@@ -121,31 +121,33 @@ function hasBreakpoint(ctx, line) {
> 
> /**
>  * Adds a visual breakpoint for a specified line. Third
>  * parameter 'cond' can hold any object.
>  *
>  * After adding a breakpoint, this function makes Editor to
>  * emit a breakpointAdded event.
>  */
>-function addBreakpoint(ctx, line, cond) {
>+function addBreakpoint(ctx, line, cond, options = {}) {
>   function _addBreakpoint(ctx, line, cond) {
>     let { ed, cm } = ctx;
>     let meta = dbginfo.get(ed);
>     let info = cm.lineInfo(line);
> 
>     ed.addMarker(line, "breakpoints", "breakpoint");
>     meta.breakpoints[line] = { condition: cond };
> 
>     info.handle.on("delete", function onDelete() {
>       info.handle.off("delete", onDelete);
>       meta.breakpoints[info.line] = null;
>     });
> 
>-    ed.emit("breakpointAdded", line);
>+    if (!options.noEvent) {
>+      ed.emit("breakpointAdded", line);
>+    }
>     deferred.resolve();
>   }
> 
>   if (hasBreakpoint(ctx, line))
>     return;
> 
>   let deferred = promise.defer();
>   // If lineInfo() returns null, wait a tick to give the editor a chance to
>@@ -157,43 +159,45 @@ function addBreakpoint(ctx, line, cond) 
>   }
>   return deferred.promise;
> }
> 
> /**
>  * Removes a visual breakpoint from a specified line and
>  * makes Editor to emit a breakpointRemoved event.
>  */
>-function removeBreakpoint(ctx, line) {
>+function removeBreakpoint(ctx, line, options = {}) {
>   if (!hasBreakpoint(ctx, line))
>     return;
> 
>   let { ed, cm } = ctx;
>   let meta = dbginfo.get(ed);
>   let info = cm.lineInfo(line);
> 
>   meta.breakpoints[info.line] = null;
>   ed.removeMarker(info.line, "breakpoints", "breakpoint");
>-  ed.emit("breakpointRemoved", line);
>+  if (!options.noEvent) {
>+    ed.emit("breakpointRemoved", line);
>+  }
> }
> 
>-function moveBreakpoint(ctx, fromLine, toLine) {
>+function moveBreakpoint(ctx, fromLine, toLine, options = {}) {
>   let { ed, cm } = ctx;
>   let info = cm.lineInfo(fromLine);
> 
>   var fromTop = cm.cursorCoords({ line: fromLine }).top;
>   var toTop = cm.cursorCoords({ line: toLine }).top;
> 
>   var marker = ed.getMarker(info.line, "breakpoints", "breakpoint");
>   if (marker) {
>     marker.setAttribute("adding", "");
>     marker.style.transform = "translateY(" + (toTop - fromTop) + "px)";
>     marker.addEventListener('transitionend', function(e) {
>-      ed.removeBreakpoint(info.line);
>-      ed.addBreakpoint(toLine);
>+      ed.removeBreakpoint(info.line, options);
>+      ed.addBreakpoint(toLine, null, options);
> 
>       // For some reason, we have to reset the styles after the marker
>       // is already removed, not before.
>       e.target.removeAttribute("adding");
>       e.target.style.transform = "none";
>     });
>   }
> }
>diff --git a/browser/devtools/webconsole/test/head.js b/browser/devtools/webconsole/test/head.js
>--- a/browser/devtools/webconsole/test/head.js
>+++ b/browser/devtools/webconsole/test/head.js
>@@ -245,17 +245,17 @@ function waitForContextMenu(aPopup, aBut
>     aOnHidden && aOnHidden();
> 
>     deferred.resolve(aPopup);
>   }
> 
>   aPopup.addEventListener("popupshown", onPopupShown);
> 
>   info("wait for the context menu to open");
>-  let eventDetails = {type: "contextmenu", button: 2};
>+  let eventDetails = { type: "contextmenu", button: 2 };
>   EventUtils.synthesizeMouse(aButton, 2, 2, eventDetails,
>                              aButton.ownerDocument.defaultView);
>   return deferred.promise;
> }
> 
> /**
>  * Listen for a new tab to open and return a promise that resolves when one
>  * does and completes the load event.
>diff --git a/browser/devtools/webconsole/webconsole.js b/browser/devtools/webconsole/webconsole.js
>--- a/browser/devtools/webconsole/webconsole.js
>+++ b/browser/devtools/webconsole/webconsole.js
>@@ -3327,16 +3327,27 @@ JSTerm.prototype = {
>           }
>           break;
>         case "help":
>           this.hud.owner.openLink(HELP_URL);
>           break;
>         case "copyValueToClipboard":
>           clipboardHelper.copyString(helperResult.value);
>           break;
>+        case "debugFunction":
>+          // TODO print a message into the Console so we give the user a
>+          // feedback like in Firebug (if they had switched back to the WebConsole)
>+          //
>+          // Also I wonder how to localize messages here.
>+          throw new Error("TODO");
>+          break;
>+        case "undebugFunction":
>+          // TODO same.
>+          throw new Error("TODO");
>+          break;
>       }
>     }
> 
>     // Hide undefined results coming from JSTerm helper functions.
>     if (!errorMessage && result && typeof result == "object" &&
>         result.type == "undefined" &&
>         helperResult && !helperHasRawOutput) {
>       aCallback && aCallback();
>diff --git a/toolkit/devtools/client/dbg-client.jsm b/toolkit/devtools/client/dbg-client.jsm
>--- a/toolkit/devtools/client/dbg-client.jsm
>+++ b/toolkit/devtools/client/dbg-client.jsm
>@@ -235,16 +235,17 @@ const UnsolicitedNotifications = {
>   "documentLoad": "documentLoad",
>   "enteredFrame": "enteredFrame",
>   "exitedFrame": "exitedFrame",
>   "appOpen": "appOpen",
>   "appClose": "appClose",
>   "appInstall": "appInstall",
>   "appUninstall": "appUninstall",
>   "evaluationResult": "evaluationResult",
>+  "breakpointadded": "breakpointadded",
> };
> 
> /**
>  * Set of pause types that are sent by the server and not as an immediate
>  * response to a client request.
>  */
> const UnsolicitedPauses = {
>   "resumeLimit": "resumeLimit",
>@@ -2559,16 +2560,19 @@ SourceClient.prototype = {
>     return this._form.actor;
>   },
>   get request() {
>     return this._client.request;
>   },
>   get url() {
>     return this._form.url;
>   },
>+  get root() {
>+    return this._client.mainRoot;
>+  },
> 
>   /**
>    * Black box this SourceClient's source.
>    *
>    * @param aCallback Function
>    *        The callback function called when we receive the response from the server.
>    */
>   blackBox: DebuggerClient.requester({
>@@ -2707,48 +2711,42 @@ SourceClient.prototype = {
>    *        The location and condition of the breakpoint in
>    *        the form of { line[, column, condition] }.
>    * @param function aOnResponse
>    *        Called with the thread's response.
>    */
>   setBreakpoint: function ({ line, column, condition }, aOnResponse = noop) {
>     // A helper function that sets the breakpoint.
>     let doSetBreakpoint = aCallback => {
>-      let root = this._client.mainRoot;
>       let location = {
>         line: line,
>         column: column
>       };
> 
>       let packet = {
>         to: this.actor,
>         type: "setBreakpoint",
>         location: location,
>         condition: condition
>       };
> 
>       // Backwards compatibility: send the breakpoint request to the
>       // thread if the server doesn't support Debugger.Source actors.
>-      if (!root.traits.debuggerSourceActors) {
>+      if (!this.root.traits.debuggerSourceActors) {
>         packet.to = this._activeThread.actor;
>         packet.location.url = this.url;
>       }
> 
>       this._client.request(packet, aResponse => {
>         // Ignoring errors, since the user may be setting a breakpoint in a
>         // dead script that will reappear on a page reload.
>         let bpClient;
>         if (aResponse.actor) {
>-          bpClient = new BreakpointClient(
>-            this._client,
>-            this,
>-            aResponse.actor,
>-            location,
>-            root.traits.conditionalBreakpoints ? condition : undefined
>-          );
>+          bpClient =
>+            this.makeBreakpointClient(aResponse.actor, location, condition);
>         }
>         aOnResponse(aResponse, bpClient);
>         if (aCallback) {
>           aCallback();
>         }
>       });
>     };
> 
>@@ -2766,17 +2764,39 @@ SourceClient.prototype = {
>       }
> 
>       const { type, why } = aResponse;
>       const cleanUp = type == "paused" && why.type == "interrupted"
>             ? () => this._activeThread.resume()
>             : noop;
> 
>       doSetBreakpoint(cleanUp);
>-    })
>+    });
>+  },
>+
>+  /**
>+   * Instanciates a BreakpointClient object.
>+   * @param {string} actor
>+   *        The actor ID for this breakpoint.
>+   * @param {object} location
>+   *        The location of the breakpoint. This is an object with two properties:
>+   *        url and line.
>+   * @param {string} condition
>+   *        The conditional expression of the breakpoint
>+   *
>+   * @return {BreakpointClient} The instance of BreakpointClient
>+   */
>+  makeBreakpointClient: function (actor, location, condition) {
>+    return new BreakpointClient(
>+      this._client,
>+      this,
>+      actor,
>+      location,
>+      this.root.traits.conditionalBreakpoints ? condition : undefined
>+    );
>   }
> };
> 
> /**
>  * Breakpoint clients are used to remove breakpoints that are no longer used.
>  *
>  * @param aClient DebuggerClient
>  *        The debugger client parent.
>diff --git a/toolkit/devtools/server/actors/script.js b/toolkit/devtools/server/actors/script.js
>--- a/toolkit/devtools/server/actors/script.js
>+++ b/toolkit/devtools/server/actors/script.js
>@@ -2574,61 +2574,85 @@ SourceActor.prototype = {
>     if (this.threadActor.state !== "paused") {
>       return {
>         error: "wrongState",
>         message: "Cannot set breakpoint while debuggee is running."
>       };
>     }
> 
>     let { location: { line, column }, condition } = request;
>+    return this.setBreakpoint(line, column, condition)
>+      .then(({actor, expectedLocation}) => {
>+      return this.createBreakpointSetResponse(actor, expectedLocation);
>+    });
>+  },
>+
>+  /**
>+   * Set a breakpoint at the specified location.
>+   *
>+   * @param {number} line The line on which to set a breakpoint.
>+   * @param {number} [column] The column on which to set the breakpoint.
>+   * @param {string} [condition] The condition of the breakpoint.
>+   *
>+   * @returns BreakpointActor
>+   *          A BreakpointActor representing the breakpoint.
>+   */
>+  setBreakpoint: function (line, column, condition) {
>     let location = new OriginalLocation(this, line, column);
>-    return this._getOrCreateBreakpointActor(
>-      location,
>-      condition
>-    ).then((actor) => {
>-      let response = {
>-        actor: actor.actorID,
>-        isPending: actor.isPending
>-      };
>-
>-      let actualLocation = actor.originalLocation;
>-      if (!actualLocation.equals(location)) {
>-        response.actualLocation = actualLocation.toJSON();
>-      }
>-
>-      return response;
>-    });
>+
>+    return this._getOrCreateBreakpointActor(location, condition);
>+  },
>+
>+  /**
>+   * Creates a response to the Front-End indicating that the breakpoint is set.
>+   * @param {string} actor The actor ID for the breakpoint
>+   * @param {originalLocation}
>+   */
>+  createBreakpointSetResponse: function (actor, expectedLocation) {
>+    let response = {
>+      actor: actor.actorID,
>+      isPending: actor.isPending
>+    };
>+
>+    let actualLocation = actor.originalLocation;
>+    if (!expectedLocation || !actualLocation.equals(expectedLocation)) {
>+      response.actualLocation = actualLocation.toJSON();
>+    }
>+
>+    return response;
>   },
> 
>   /**
>    * Get or create a BreakpointActor for the given location in the original
>    * source, and ensure it is set as a breakpoint handler on all scripts that
>    * match the given location.
>    *
>    * @param OriginalLocation originalLocation
>    *        An OriginalLocation representing the location of the breakpoint in
>    *        the original source.
>-   * @param String condition
>+   * @param String [condition]
>    *        A string that is evaluated whenever the breakpoint is hit. If the
>    *        string evaluates to false, the breakpoint is ignored.
>    *
>    * @returns BreakpointActor
>    *          A BreakpointActor representing the breakpoint.
>    */
>   _getOrCreateBreakpointActor: function (originalLocation, condition) {
>     let actor = this.breakpointActorMap.getActor(originalLocation);
>     if (!actor) {
>       actor = new BreakpointActor(this.threadActor, originalLocation);
>       this.threadActor.threadLifetimePool.addActor(actor);
>       this.breakpointActorMap.setActor(originalLocation, actor);
>     }
> 
>     actor.condition = condition;
> 
>-    return this._setBreakpoint(actor);
>+    return this._setBreakpoint(actor).then(function (actor) {
>+      return { actor, expectedLocation: originalLocation };
>+    });
>   },
> 
>   /*
>    * Ensure the given BreakpointActor is set as a breakpoint handler on all
>    * scripts that match its location in the original source.
>    *
>    * If there are no scripts that match the location of the BreakpointActor,
>    * we slide its location to the next closest line (for line breakpoints) or
>diff --git a/toolkit/devtools/webconsole/utils.js b/toolkit/devtools/webconsole/utils.js
>--- a/toolkit/devtools/webconsole/utils.js
>+++ b/toolkit/devtools/webconsole/utils.js
>@@ -1869,16 +1869,66 @@ WebConsoleCommands._registerOriginal("pr
>   // Waiving Xrays here allows us to see a closer representation of the
>   // underlying object. This may execute arbitrary content code, but that
>   // code will run with content privileges, and the result will be rendered
>   // inert by coercing it to a String.
>   return String(Cu.waiveXrays(aValue));
> });
> 
> /**
>+ * Place a breakpoint at the first line of a function.
>+ *
>+ * @param {Function} func The function to debug
>+ */
>+WebConsoleCommands._registerOriginal("debug", function(owner, func) {
>+  // TODO Handle SourceMaps.
>+  // TODO Send message to the front-end to mean whether the command
>+  //      succeeded or not (and why if it doesn't).
>+  let { globalDebugObject, parentActor } = owner.consoleActor;
>+  // Don't do anything if the debugger is not active
>+  // (in which case globalDebugObject is undefined).
>+  if (!globalDebugObject) {
>+    return;
>+  }
>+  let script = globalDebugObject.makeDebuggeeValue(func).script;
>+  if (!script) {
>+    return;
>+  }
>+
>+  let source = parentActor.sources.getSourceActor(script.source);
>+  let offsets = script.getAllColumnOffsets();
>+  if (!offsets.length) {
>+    return;
>+  }
>+
>+  let { lineNumber, columnNumber } = offsets[0];
>+
>+  source.setBreakpoint(lineNumber, columnNumber).then(({actor}) => {
>+    // Don't send the expected location so we force to send the actual location.
>+    let resp = source.createBreakpointSetResponse(actor);
>+    source.conn.sendActorEvent(source.actorID, "breakpointAdded", resp);
>+  });
>+});
>+
>+/**
>+ * Remove a breakpoint at the first line of a function.
>+ *
>+ * @param {Function} func The function for which we remove the breakpoint.
>+ */
>+WebConsoleCommands._registerOriginal("undebug", function(owner, func) {
>+  let dbgObj = owner.makeDebuggeeValue(func);
>+  let grip = owner.createValueGrip(dbgObj);
>+  owner.helperResult = {
>+    type: "undebugFunction",
>+    function: grip
>+  };
>+});
>+
>+
>+/**
>  * Copy the String representation of a value to the clipboard.
>  *
>  * @param any aValue
>  *        A value you want to copy as a string.
>  * @return void
>  */
> WebConsoleCommands._registerOriginal("copy", function JSTH_copy(aOwner, aValue)
> {

Comment 13

[Florent Fayolle]

Attachment: 1164880.patch (WIP)

Sorry for the previous thread pollution... 

I fixed the "twice-debugger-resumption bug" simply: if a breakpoint exist at the first line of the instruction (not considering the column), it reports an error.

TODO:
 - sourcemaps (would need help) 
 - undebug
 - tests

Not working (though no exception):
 - Browser Toolbox / Browser Content Toolbox (probably TODO in a follow-up)

Florent

Comment 14

[Florent Fayolle]

James, do you have the answer of this question, by chance?

> * SourceMaps: I would need to understand the |OriginalLocation| role here: 
>   https://github.com/mozilla/gecko-dev/blob/b83b4f3197c8dc735e47551f33d8f14d505e1524/toolkit/devtools/server/actors/script.js#L2581
>   If I set a breakpoint in a PrettyPrinted source (so it is SourceMapped, 
>   right?), the function is called with the generated line and column numbers 
>   (i.e. the ones that the user see in their Debugger view), and we pass them 
>   to the |OriginalLocation| constructor. So I don't understand what this 
>   object represents.

Florent

Comment 15

[James Long (:jlongster)]

You might be confused by the terms "original" and "generated". They aren't clear what they represent.

* original: the line numbers that you see in the editor in the debugger. This location represents what the user interacts with. If it's a non-sourcemapped source, it's the line numbers in the real file. If it's a sourcemapped source, it's the line number in the "original" file that is being sourcemapped (like the JSX, CoffeeScript, ClojureScript, etc file).

* generated: the line numbers that the debugger uses in low-level calls. This is what we use to actually interact with the scripts in the system. For non-sourcemapped sources, it's the line number in the real file. For sourcemapped sources, it's the line numbers in the "generated" files from the compiler that the debugger actually runs.

You pass original locations to setBreakpoint because that's what the user interacts with, and for sourcemapped files will be the locations of the file being sourcemapped.

Comment 16

[Florent Fayolle]

I see, thanks for the clear explanation James.

Florent

Comment 17

[Florent Fayolle]

Attachment: 1164880.patch (WIP)

Fixed: Support Source Map

TODO: 
 - undebug
 - tests

Not working (though no exception):
 - Browser Toolbox / Browser Content Toolbox (probably TODO in a follow-up)

Florent

Comment 18

[Sebastian Zartner [:sebo]]

Florent, what's the status of this? Are you still working on it?

Sebastian

Comment 19

[Florent Fayolle]

Currently not, sorry.

Florent