818134 - Allow multiple debuggers in toolboxes to debug separate tabs

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

After the toolbox landed we lost the notification that popped up each time the user tried to open the debugger, while another instance was already open (because both could end up paused). The consensus was that we should not bring the old behavior back, but rather let the debugger know when another instance is paused, and refuse to resume in the wrong order.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

WIP, doesn't even compile.

Comment 2

[Girish Sharma [:Optimizer]]

Does this mean that I can have 2 working debuggers in 2 different tabs ?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Girish Sharma [:Optimizer] from comment #2)
> Does this mean that I can have 2 working debuggers in 2 different tabs ?

That's the plan.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

Still WIP, but it compiles now. It asserts when running though.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

Almost works. In a stack of three, only the last one is allowed to resume, but things break down in a stack of two.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

Backend fully working. Now I need to display a message to the user when not resuming the right debugger.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v5

Notification panel hooked up and working. Changed content debugging to ignore new globals, except in iframes, since the previous code expected hostAnnotations which are still not here. Browser chrome tests pass, xpcshell tests fail, due to a hack used there that has come back to bite me. More hacks coming right up!

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v6

This is a working patch, with tests for the new XPCOM component functionality. I split out the newGlobal fix, which I'll attach in a separate patch for easier reviewing.

I've used this page for testing the patch: http://jsbin.com/emosas/2
I load it in 3 tabs, start debuggers on all of them and then reload each page in turn. After they are all paused, I try to resume them in any order I can think of.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=77a013dc53d5

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Limit newGlobal events

This is the newGlobal fix that I extracted from the other patch, but I still need to work on it some more. I missed the fact that onNewGlobal parameters are of a Debugger.Object type, whereas the frames are of a Window type.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Try was green (only 3 unrelated failures).

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v7

It turns out that I was confused about the onNewGlobal code, it wasn't causing any problems after all. Precisely because hostAnnotations are not implemented yet, no new globals are added as debuggees, it's just that the "newGlobal" protocol notification was sent regardless. I've moved that code into the conditional block, so we shouldn't suffer any more head-scratching from such packets in the log.

Moreover, dynamically-added iframes are automatically connected with their parent window, so they could be debugged even before the onNewGlobal hook was implemented. It may be that there is no use in calling addDebuggee for these globals even if we could identify them, but I'll leave that code in there until hostAnnotations land and I can experiment with it.

Comment 13

[Rob Campbell [:rc] (:robcee)]

ok, I think I'll review this version. ;)

Comment 14

[Rob Campbell [:rc] (:robcee)]

I tested this in a build yesterday and couldn't get the panel to show up with two paused debuggers. Still reviewing the patches, but not sure this is working.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

I just rebased and retested the patch and it still works for me, using the testing process I described in comment 8. Can you try that, or let me know how to reproduce your testing?

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v8

Attached rebased patch, including an additional cleanup in DebuggerPanel.jsm

Comment 17

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #16)
> Created attachment 699348 [details] [diff] [review]
> Patch v8
> 
> Attached rebased patch, including an additional cleanup in DebuggerPanel.jsm

ok, tested out my rebased copy and things are working well now. I mixed up some tabs with the debugger test page just to mix things up a bit.

The panel that pops up should probably be centered on the top of the Play button instead of anchored to the top left.

I noticed that we're losing keyboard bindings on tab switch. Not just with this patch enabled.

Filed bug 828001 for that. Could be a toolbox bug.

Comment 18

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 699348 [details] [diff] [review]
Patch v8

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

pretty sweet.

It would be nice if we told the user which tab they needed to resume next in the popup. I think that would be nicer (and shouldn't be too hard to do, I don't think). Otherwise, I have a couple of minor comment nits and some wording around the panel's text.

I would like jimb to take a look at the changes to nsIJSINspector.idl and nsJSInspector.h|cpp as well.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ -226,5 @@
> -      imageURL, nbox.PRIORITY_WARNING_HIGH, buttons, null);
> -
> -    // Make sure this is not a transient notification, to avoid the automatic
> -    // transient notification removal.
> -    notification.persistence = -1;

lotsa red. Love it.

::: browser/devtools/debugger/debugger-controller.js
@@ +520,5 @@
> +        this.activeThread.resume(function (aResponse) {
> +          if (aResponse.error == "wrongOrder") {
> +            DebuggerView.Toolbar.showResumeWarning();
> +          }
> +        }.bind(this));

rather than defining this anonymous function three times, you could create it once and refer to it in each of these places with the appropriate bind(this).

::: browser/devtools/debugger/debugger-toolbar.js
@@ +89,5 @@
>    },
>  
>    /**
> +   * Display a warning when trying to resume a debuggee while another one has
> +   * been paused later.

this is a slightly confusing comment. Maybe reword it as,

"Display a warning when trying to resume a debuggee while another is paused. Debuggees must be unpaused in a Last-In-First-Out order.".

@@ +135,5 @@
> +      DebuggerController.activeThread.resume(function (aResponse) {
> +        if (aResponse.error == "wrongOrder") {
> +          this.showResumeWarning();
> +        }
> +      }.bind(this));

hey! There's that function again!

::: browser/devtools/debugger/debugger.xul
@@ +280,5 @@
>  
> +    <panel id="resumption-order-panel"
> +           type="arrow"
> +           noautofocus="true"
> +           position="before_start">

I think applying a style of "margin-left: 16px" would get us near the middle of our continue button. Either via a style attribute here (probably less desirable) or in the debugger.css files.

see, https://developer.mozilla.org/en-US/docs/XUL/PopupGuide/Positioning#Positioning_using_Coordinates

@@ +284,5 @@
> +           position="before_start">
> +      <hbox align="start">
> +        <image class="alert-icon"/>
> +        <label id="resumption-panel-desc" class="description">
> +          &debuggerUI.resumptionOrderPanelTitle;

typically use a value attribute for setting contents of the label. Does away with the need for a separate closing tag.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +135,5 @@
> +<!-- LOCALIZATION NOTE (debuggerUI.resumptionOrderPanelTitle): This is the text
> +  -  that appears as a description in the notification panel popup, when
> +  -  multiple debuggers are open in separate tabs and the user tries to resume
> +  -  them in the wrong order. -->
> +<!ENTITY debuggerUI.resumptionOrderPanelTitle "There are multiple debuggers open and this was not the last paused one. Please resume the last paused debugger first, before resuming this one.">

Wording is tricky here. How about something like:

"There are one or more paused debuggers. Please resume the most-recently paused debugger first."

::: browser/themes/gnomestripe/devtools/debugger.css
@@ +525,5 @@
>  
> +#resumption-panel-desc {
> +  width: 200px;
> +}
> +

could add 

#resumption-order-panel {
  margin-left: 16px;
}

here (and in other debugger.css files)

::: toolkit/devtools/debugger/nsIJSInspector.idl
@@ +19,4 @@
>     * @return depth Returns the number of times the event loop
>     *         has been nested using this API.
>     */
> +  unsigned long enterNestedEventLoop(in unsigned long long window);

that is a helluva type specification.

::: toolkit/devtools/debugger/nsJSInspector.cpp
@@ +105,5 @@
> +{
> +  *out = mLastRequestor;
> +  return NS_OK;
> +}
> +

I think this is ok, but I'd like a jimb or jorendorff to take a look at it.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +809,5 @@
> +    // so that xpcshell tests don't throw here.
> +    let innerID = 0;
> +    if (this.global && this.global.QueryInterface) {
> +      innerID = this.global.QueryInterface(Ci.nsIInterfaceRequestor)
> +                    .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;

whew.

Comment 19

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #18)
> 
> lotsa red. Love it.
> 

Yeah, those code paths aren't even exercised. 80% of DebuggerUI needs to be massacred, but that happens in bug 823577.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #18)
> It would be nice if we told the user which tab they needed to resume next in
> the popup. I think that would be nicer (and shouldn't be too hard to do, I
> don't think). Otherwise, I have a couple of minor comment nits and some
> wording around the panel's text.

That's what I tried to do initially, by storing the window URL instead of the ID, but I was having a hard time dealing with string arrays in C++. I went with IDs because they would be faster and lighter on memory, but also because I don't realistically expect users to have more than two debugger windows paused simultaneously, in which case the right order is obvious.

I could give in another try though, if you think we really need this.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v9

While waiting on your thoughts on comment 20, I made all the changes you suggested, except for the following two:

(In reply to Rob Campbell [:rc] (:robcee) from comment #18)
> ::: browser/devtools/debugger/debugger.xul
> @@ +280,5 @@
> >  
> > +    <panel id="resumption-order-panel"
> > +           type="arrow"
> > +           noautofocus="true"
> > +           position="before_start">
> 
> I think applying a style of "margin-left: 16px" would get us near the middle
> of our continue button. Either via a style attribute here (probably less
> desirable) or in the debugger.css files.

OK, but I got the best arrow placement with: -moz-margin-start: -8px;

> @@ +284,5 @@
> > +           position="before_start">
> > +      <hbox align="start">
> > +        <image class="alert-icon"/>
> > +        <label id="resumption-panel-desc" class="description">
> > +          &debuggerUI.resumptionOrderPanelTitle;
> 
> typically use a value attribute for setting contents of the label. Does away
> with the need for a separate closing tag.

Unfortunately that doesn't let the text wrap:
https://developer.mozilla.org/en-US/docs/XUL/label#Wrapping

Comment 22

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #20)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #18)
> > It would be nice if we told the user which tab they needed to resume next in
> > the popup. I think that would be nicer (and shouldn't be too hard to do, I
> > don't think). Otherwise, I have a couple of minor comment nits and some
> > wording around the panel's text.
> 
> That's what I tried to do initially, by storing the window URL instead of
> the ID, but I was having a hard time dealing with string arrays in C++. I
> went with IDs because they would be faster and lighter on memory, but also
> because I don't realistically expect users to have more than two debugger
> windows paused simultaneously, in which case the right order is obvious.
> 
> I could give in another try though, if you think we really need this.

you're right, it's a pretty fringe case and if people get themselves there, they should be able to figure out the correct order. I'm happy without it.

(though if we had a TabForWindowId method in tabbrowser, that would let us solve the problem)

Comment 23

[Jim Blandy :jimb]

For what it's worth, I don't think it's really going to be a fringe case. If a dev has a lot of tabs open that he's messing with, I think it could be pretty common.

When we talked about it before, the solution I suggested was that, when any tab's debugger has stopped its debuggee in mid-event-dispatch (via a breakpoint, say), that all other tabs with active debuggers become frozen. Then, if the dev wants to interact with them, it's easy to say, "You have to let this other guy go first." (I don't know how much work this is to implement.)

Ideally, they'd all have separate stacks, and be resumable in whatever order you like, but that's not going to happen. :(

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #23)
> For what it's worth, I don't think it's really going to be a fringe case. If
> a dev has a lot of tabs open that he's messing with, I think it could be
> pretty common.
> 
> When we talked about it before, the solution I suggested was that, when any
> tab's debugger has stopped its debuggee in mid-event-dispatch (via a
> breakpoint, say), that all other tabs with active debuggers become frozen.
> Then, if the dev wants to interact with them, it's easy to say, "You have to
> let this other guy go first." (I don't know how much work this is to
> implement.)

This was something that we considered, in one of our meetings a few months ago, but the consensus was to move forward with the more flexible approach. It seemed like a good idea to allow the user to pause multiple tabs simultaneously, say in order to compare states or something. All those tabs can be paused or running, with the only constraint being that resumption order must be the only one that can actually work.

I consider the multiple-debuggers-open scenario rare, because in my previous life as a web dev I never really found myself debugging more than one tab at a time, and that was not because I ran across a tool-imposed limit: Firebug allows multiple paused tabs, too, without even prompting for the right resumption order. The reason was that modern web apps are mostly single-URL sites, so I never found myself launching other windows that I needed to work on.

I can imagine that perhaps some users may be willing to multitask in some cases, say debugging Gmail in one tab and Google+ in another, even though the realities of the human brain are stacked against them. I find it hard to imagine though that they will want to debug (and pause!) 3 or more tabs at the same time, but even in that case I assert that it will be easy to remember which was the last paused tab.

In any case, I would rank the available options in the order of increased value to the user as follows:

(a) only one debugger open (what we have now)
(b) multiple debuggers open, only one paused (Jim's suggestion)
(c) multiple debuggers open, resumption in LIFO order (this patch)
(d) multiple debuggers open, resumption in LIFO order, message pointing to the last paused tab (Rob's suggestion)

I think (c) is the best tradeoff overall, and it provides a straightforward upgrade path to (d), if it ever becomes important.

Comment 25

[Jim Blandy :jimb]

Well, I'm happy to defer to your judgment on this. I do think (d) would be a big improvement.

Comment 26

[Joe Walker [:jwalker]]

Looking at the size of this, we're not uplifting to aurora, so changing the blocking status.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 699782 [details] [diff] [review]
Patch v9

I know Jim is terribly busy right now with Very Important Stuff and Dave was the original author of this code, so he may be able to review this patch sooner.

Comment 28

[Dave Camp (:dcamp)]

Comment on attachment 699782 [details] [diff] [review]
Patch v9

As discussed in irc, we probably want to uniquely identify individual event loops rather than tying them to window ids.

Comment 29

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #27)
> Comment on attachment 699782 [details] [diff] [review]
> Patch v9
> 
> I know Jim is terribly busy right now with Very Important Stuff and Dave was
> the original author of this code, so he may be able to review this patch
> sooner.

Sorry to have neglected this --- that may be for the best.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 699782 [details] [diff] [review]
Patch v9

I'll get back to this soon.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v10

Rebased.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v11

This version properly handles multiple debugger clients debugging the same tab, by storing both debuggee URLs and protocol connections every time the server spins a nested event loop. Keeping the URL allows us to make the user-facing message more useful, by including the address of the last paused tab, as previously discussed.

Mark, for the latter I am adding the page URL to the textContent of the XUL panel's label, in order to get a message like:

There are one or more paused debuggers. Please resume the most-recently paused debugger first at: http://foo/bar

I assume this is safe from a security standpoint, right? This particular change is in debugger-toolbar.js.

Comment 33

[Dave Camp (:dcamp)]

Comment on attachment 732025 [details] [diff] [review]
Patch v11

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

::: toolkit/devtools/debugger/nsIJSInspector.idl
@@ +13,5 @@
>  {
>    /**
>     * Process the thread's event queue until exit.
>     *
> +   * @param requestor An object with two properties, |connection| and |url|,

It looks like requestor is really an opaque object for the purposes of nsIJSInspector?  If I'm reading this right, its internal property layout isn't important here.

Do you think it's ok that nsIJSInspector doesn't protect itself?  A naively-written user could really mess up the event loop.

Maybe it would be better}

::: toolkit/devtools/debugger/nsJSInspector.cpp
@@ +19,5 @@
>  #define JSINSPECTOR_CONTRACTID \
>    "@mozilla.org/jsinspector;1"
>  
>  #define JSINSPECTOR_CID \
> +{ 0x9203cdd3, 0xeb7c, 0x44c5, { 0x8e, 0x8b, 0x23, 0xef, 0xe1, 0x0f, 0x54, 0xad } }

I don't think you need to change the CID - it's ok if hard-coded clients can get access to your service, because they'll only get an nsISupports that won't convert to old-iid nsIJSInspector.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +160,5 @@
> +          from: this.actorID,
> +          type: "newGlobal",
> +          // TODO: after bug 801084 lands see if we need to JSONify this.
> +          hostAnnotations: aGlobal.hostAnnotations
> +        });

This looks like an important/worthwhile fix, but did you mean it to be in this patch?

@@ +274,5 @@
> +    if (DebuggerServer.xpcInspector.eventLoopNestLevel > 1) {
> +      let lastNestRequestor = DebuggerServer.xpcInspector.lastNestRequestor;
> +      if (lastNestRequestor.url != this._hooks.url ||
> +          lastNestRequestor.connection != this.conn) {
> +        return { error: "wrongOrder",

This still seems a bit like the wrong check.  You even could be stopped twice at exactly the same location if you eval recursively.

I think we either need tokens-per-pause or to just specify that resume always resumes the most recent pause within a given connection (which I think is reasonable).

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Camp (:dcamp) from comment #33)
> ::: toolkit/devtools/debugger/nsIJSInspector.idl
> @@ +13,5 @@
> >  {
> >    /**
> >     * Process the thread's event queue until exit.
> >     *
> > +   * @param requestor An object with two properties, |connection| and |url|,
> 
> It looks like requestor is really an opaque object for the purposes of
> nsIJSInspector?  If I'm reading this right, its internal property layout
> isn't important here.

Good point, fixed.

> Do you think it's ok that nsIJSInspector doesn't protect itself?  A
> naively-written user could really mess up the event loop.
> 
> Maybe it would be better}

Since it treats the parameter as an opaque object, how do you suggest that it protects itself? Did Splinter eat the comment?

> ::: toolkit/devtools/debugger/nsJSInspector.cpp
> @@ +19,5 @@
> >  #define JSINSPECTOR_CONTRACTID \
> >    "@mozilla.org/jsinspector;1"
> >  
> >  #define JSINSPECTOR_CID \
> > +{ 0x9203cdd3, 0xeb7c, 0x44c5, { 0x8e, 0x8b, 0x23, 0xef, 0xe1, 0x0f, 0x54, 0xad } }
> 
> I don't think you need to change the CID - it's ok if hard-coded clients can
> get access to your service, because they'll only get an nsISupports that
> won't convert to old-iid nsIJSInspector.

OK, makes sense.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +160,5 @@
> > +          from: this.actorID,
> > +          type: "newGlobal",
> > +          // TODO: after bug 801084 lands see if we need to JSONify this.
> > +          hostAnnotations: aGlobal.hostAnnotations
> > +        });
> 
> This looks like an important/worthwhile fix, but did you mean it to be in
> this patch?

Moving it inside the conditional only affects my sanity when reading protocol logs (and is a slight perf win by eliminating unnecessary packets), so it didn't seem worthy of a separate bug.

> @@ +274,5 @@
> > +    if (DebuggerServer.xpcInspector.eventLoopNestLevel > 1) {
> > +      let lastNestRequestor = DebuggerServer.xpcInspector.lastNestRequestor;
> > +      if (lastNestRequestor.url != this._hooks.url ||
> > +          lastNestRequestor.connection != this.conn) {
> > +        return { error: "wrongOrder",
> 
> This still seems a bit like the wrong check.  You even could be stopped
> twice at exactly the same location if you eval recursively.
> 
> I think we either need tokens-per-pause or to just specify that resume
> always resumes the most recent pause within a given connection (which I
> think is reasonable).

You mean eval recursively from the same client connection, like in a watch expression? I assumed that in that case we always resume the most recent pause. Can you describe in a little more detail the scenario that I'm not handling well in this patch?

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v12

(In reply to Panos Astithas [:past] from comment #34)
> (In reply to Dave Camp (:dcamp) from comment #33)
> > Do you think it's ok that nsIJSInspector doesn't protect itself?  A
> > naively-written user could really mess up the event loop.
> > 
> > Maybe it would be better}
> 
> Since it treats the parameter as an opaque object, how do you suggest that
> it protects itself? Did Splinter eat the comment?

I thought about the idea to have exitNestedLoop take a token as a parameter and only proceed if it matches the token on top of the stack, but I think it's not that useful because the tokens are objects and matching isn't as cheap as if we were using literals and I don't consider this component, with its spartan feature set, as something that will receive widespread use, to warrant going to extreme lengths to guard against misuse.

> > @@ +274,5 @@
> > > +    if (DebuggerServer.xpcInspector.eventLoopNestLevel > 1) {
> > > +      let lastNestRequestor = DebuggerServer.xpcInspector.lastNestRequestor;
> > > +      if (lastNestRequestor.url != this._hooks.url ||
> > > +          lastNestRequestor.connection != this.conn) {
> > > +        return { error: "wrongOrder",
> > 
> > This still seems a bit like the wrong check.  You even could be stopped
> > twice at exactly the same location if you eval recursively.
> > 
> > I think we either need tokens-per-pause or to just specify that resume
> > always resumes the most recent pause within a given connection (which I
> > think is reasonable).
> 
> You mean eval recursively from the same client connection, like in a watch
> expression? I assumed that in that case we always resume the most recent
> pause. Can you describe in a little more detail the scenario that I'm not
> handling well in this patch?

You were right that the URL check wasn't necessary, so I removed it, and I used the thread actor instead of the connection. Then I changed it to use the actorID, because the actor object is rather big for this purpose.

Then I thought that I could combine the URL and actorID somehow into a string token, but that came with a different set of problems, so I thought let's just keep it simple (for now at least).

Comment 36

[Dave Camp (:dcamp)]

Comment on attachment 733919 [details] [diff] [review]
Patch v12

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

Fine with me, but I'm not sure the ownership situation of nsJSInspector.cpp, so I'm going to toss to jimb.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +272,5 @@
> +    // different tabs or multiple debugger clients connected to the same tab)
> +    // only allow resumption in a LIFO order.
> +    if (DebuggerServer.xpcInspector.eventLoopNestLevel > 1) {
> +      let lastNestRequestor = DebuggerServer.xpcInspector.lastNestRequestor;
> +      if (lastNestRequestor.thread != this.actorID) {

You might want to check conn too.  While actorIDs will be unique across connections for a given server, there might maybe someday be multiple servers (if you ever want to debug the debug server).  Connection objects, though, will be unique across servers.

Comment 37

[Dave Camp (:dcamp)]

Comment on attachment 733919 [details] [diff] [review]
Patch v12

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

I confused nsJSInspector, which lives in devtools, with nsIJSDebugger, which doesn't.  My review should be good enough.

Comment 38

[Mark Goodwin [:mgoodwin]]

(In reply to Panos Astithas [:past] from comment #32)
> I assume this is safe from a security standpoint, right? This particular
> change is in debugger-toolbar.js.

Yeah, this is fine.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v13

Using connection again, to be better prepared for the future.

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/5306afe4579b

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5306afe4579b