Closed Bug 818134 Opened 7 years ago Closed 7 years ago

Allow multiple debuggers in toolboxes to debug separate tabs

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: past, Assigned: past)

Details

Attachments

(1 file, 13 obsolete files)

44.85 KB, patch
past
: review+
Details | Diff | Splinter Review
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.
Attached patch Patch v1 (obsolete) — Splinter Review
WIP, doesn't even compile.
Does this mean that I can have 2 working debuggers in 2 different tabs ?
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
Still WIP, but it compiles now. It asserts when running though.
Attached patch Patch v3 (obsolete) — Splinter Review
Almost works. In a stack of three, only the last one is allowed to resume, but things break down in a stack of two.
Attachment #689228 - Attachment is obsolete: true
Attachment #689352 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Backend fully working. Now I need to display a message to the user when not resuming the right debugger.
Attachment #689356 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
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!
Attachment #691455 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
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.
Attachment #691968 - Attachment is obsolete: true
Attachment #692300 - Flags: review?(rcampbell)
Attached patch Limit newGlobal events (obsolete) — Splinter Review
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.
Try was green (only 3 unrelated failures).
Attached patch Patch v7 (obsolete) — Splinter Review
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.
Attachment #692300 - Attachment is obsolete: true
Attachment #692340 - Attachment is obsolete: true
Attachment #692300 - Flags: review?(rcampbell)
Attachment #692898 - Flags: review?(rcampbell)
ok, I think I'll review this version. ;)
Whiteboard: [has-patch]
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.
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?
Attached patch Patch v8 (obsolete) — Splinter Review
Attached rebased patch, including an additional cleanup in DebuggerPanel.jsm
Attachment #692898 - Attachment is obsolete: true
Attachment #692898 - Flags: review?(rcampbell)
Attachment #699348 - Flags: review?(rcampbell)
(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 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.
Attachment #699348 - Flags: review?(rcampbell) → review?(jimb)
(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.
(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.
Attached patch Patch v9 (obsolete) — Splinter Review
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
Attachment #699348 - Attachment is obsolete: true
Attachment #699348 - Flags: review?(jimb)
Attachment #699782 - Flags: review?(jimb)
(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)
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. :(
(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.
Well, I'm happy to defer to your judgment on this. I do think (d) would be a big improvement.
Looking at the size of this, we're not uplifting to aurora, so changing the blocking status.
Blocks: DevToolsPaperCuts
No longer blocks: 816946
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.
Attachment #699782 - Flags: review?(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.
Attachment #699782 - Flags: review?(dcamp) → review-
(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 on attachment 699782 [details] [diff] [review]
Patch v9

I'll get back to this soon.
Attachment #699782 - Flags: review?(jimb)
Attached patch Patch v10 (obsolete) — Splinter Review
Rebased.
Attachment #699782 - Attachment is obsolete: true
Attached patch Patch v11 (obsolete) — Splinter Review
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.
Attachment #731792 - Attachment is obsolete: true
Attachment #732025 - Flags: review?(mgoodwin)
Attachment #732025 - Flags: review?(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).
Attachment #732025 - Flags: review?(dcamp)
(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?
Attached patch Patch v12 (obsolete) — Splinter Review
(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).
Attachment #732025 - Attachment is obsolete: true
Attachment #732025 - Flags: review?(mgoodwin)
Attachment #733919 - Flags: review?(dcamp)
Flags: sec-review?(mgoodwin)
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.
Attachment #733919 - Flags: review?(jimb)
Attachment #733919 - Flags: review?(dcamp)
Attachment #733919 - Flags: review+
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.
Attachment #733919 - Flags: review?(jimb)
(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.
Flags: sec-review?(mgoodwin) → sec-review+
Attached patch Patch v13Splinter Review
Using connection again, to be better prepared for the future.
Attachment #733919 - Attachment is obsolete: true
Attachment #735117 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5306afe4579b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.