933212 - Debugger can only be opened once per app per connection

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0
BuildID: 20131030030201

Steps To Reproduce:
1. Connect the App Manager to a device
2. Start an app
3. Click the Debug button for that app to open the toolbox
4. Switch to the Debugger tab
5. Close the toolbox
6. Click Debug again on the same app

Actual Results:
The Debugger tab is selected, but the actual content of the tool does not appear. "This page has no sources." is displayed on the Source list pane.

Expected Results:
The Debugger tab contains the same data as in step 4.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I can confirm that this issue is present when connecting to both the 1.2 and 1.3 simulators, so it (seems) independent of FxOS version.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I've looked into this a bit more, and it seems like the issue is we don't send a "detach" message to the script actor until connection close.  Outside of the App Manager, this is fine, because the connection is closed when the toolbox closes.

However, in the App Manager, the connection is reused across several toolboxes.  Also, the script actor doesn't appear to reset its state when it does get a detach message, as it may rely on just being destroyed when the connection goes down.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just to note that Firebug needs to re-attach (detach/attach) to the same thread-actor too. The problem is that when detaching from a thread actor its state is set to: "exited" (in disconnect method) and when attaching again, onAttach handler fails since checking the state against "exited"...

Honza

Comment 4

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

Attachment: Make the debugger frontend cope with an already connected target

This is my WIP. It fixes the debugger initialization problem and also makes the tracer and shader editor work for b2g. It's not done yet, because the debugger maintains a lot of state in the backend that the frontend didn't have to re-sync with until now, and I've got more stuff to fix there (like breakpoints). I'd also like to confirm that the shader editor works, but I don't have any WebGL b2g app to test with. The tracer works fine.

Comment 5

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

Attachment: Make the debugger frontend cope with an already connected target v2

More fixes and a test for reattaching to a previously detached thread actor. This should hopefully be all that Firebug needs. Honza, can you build with this patch to confirm?

The App Manager needs more fixes though. The original problem is fixed with this patch, but there are others that came up as I looked further into the implementation. The App Manager violates a basic assumption of the old-style fronts, which is that each toolbox corresponds to a separate protocol connection. Things like "activeThread" or "activeTab" don't play nicely with a shared client. I need to spend some more time fixing this and writing the necessary tests.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8350701 [details] [diff] [review]
Make the debugger frontend cope with an already connected target v2

> Honza, can you build with this patch to confirm?
Yes, the patch fixes the reattaching problem.

One question. Currently Firebug is using a hack to modify the state flag. 
https://github.com/firebug/firebug/blob/jsd2/extension/content/firebug/remoting/tabClient.js#L289

Any tips how Firebug can feature-detect whether this fix is already available in the current Firefox? Or do we have to check Firefox version?

Honza

Comment 7

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

Thanks Honza. About backwards compatibility, I was under the impression that Firebug cannot ship with JSDBG2/remote protocol code until some other bugs are fixed, which would mean Firefox 29+. Is that not the case?

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #7)
> Thanks Honza. About backwards compatibility, I was under the impression that
> Firebug cannot ship with JSDBG2/remote protocol code until some other bugs
> are fixed, which would mean Firefox 29+. Is that not the case?
Yes, we are is still blocked by bug 911721
But alpha/beta phase could start sooner, pre Fx29 (with missing features) to get feedback in advance.
Honza

Comment 9

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

Attachment: Make the debugger frontend cope with an already connected target v3

This seems to work as expected, but I have some mochitest failures I still need to fix.

Comment 10

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

I managed to get a green try run, but then discovered a regression in the original fix. Blech.

Comment 11

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

Attachment: Make the debugger frontend cope with an already connected target v4

So here is a complete patch that passes all tests. Notable changes:

* Made the DebuggerClient, which is actually the RootActor front, not consider one of the attached child fronts as "active". Since a single DebuggerClient (or RootFront) is kept around for the App Manager's lifetime, it makes sense to move the notion of "active" tab to the toolbox's target. As each toolbox gets destroyed, the fronts should be detaching from their actors (if they are stateful) so that the app is no longer in a debugging state. Debugging a new app (or reconnecting to a previous one) will create new fronts anyway.
* Slightly refactored the TabClient, ThreadClient, SourceClient and TracerClient towards a protocol.js-based architecture, by adding parent-child references and lifecycle management. Now a tab-scoped thread actor for instance has the tab as its parent, while a global-scoped thread actor (chrome debugger) has the DebuggerCLient (RootFront) as its parent. This lets parents reference their children, so that caching in the target object can work. It also allowed me to move some methods from the DebuggerClient to the actual front that should be responsible, like reconfigureTab, reconfigureThread and attachThread. These methods now use DebuggerClient.requester, too.
* Added some error handling in the debugger client requester around "before" and "after" callbacks, which exposed some errors in tests that are now fixed.
* Fixed the state handling in the thread actor so that merely detaching from a thread doesn't put it in the exited state. This is the part that what was necessary for Firebug's use case.
* Properly loading tracer and webgl actors now on b2g.

One thing that I didn't fix is that reopening the toolbox on an app where the inspector has been previously opened results in a completely gray toolbox. Not even a toolbox actually, it seems like the App Manager fails to create a tab, even though focus moves away from the apps tab. This is not a regression from this patch, so I think I will file a followup for that and it might be a good idea to get someone more familiar with the inspector to look at it.

Nick can look at the debugger changes and Paul at the framework bits. Do we need someone from b2g to sign off on the actor additions in shell.js or can you do that Paul?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=893a125bae42

Comment 12

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

Comment on attachment 8358388 [details] [diff] [review]
Make the debugger frontend cope with an already connected target v4

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

r=me for the dbg stuff with comments below addressed.

::: browser/devtools/debugger/test/browser_dbg_source-maps-02.js
@@ +63,5 @@
>  
>  function testSetBreakpoint() {
>    let deferred = promise.defer();
>  
> +  gDebugger.gThreadClient.interrupt(aResponse => {

Doesn't |setBreakpoint| do an |interrupt| for us? Why is this needed?

::: toolkit/devtools/client/dbg-client.jsm
@@ +229,5 @@
>    this._transport = aTransport;
>    this._transport.hooks = this;
>    this._threadClients = {};
>    this._tabClients = {};
> +  this._tracerClients = {};

Nit: I realize none of these properties have comments, but it might be nice if we just had a short comment for all of the |this._xClients = {};| that said something like "Map actor ID to client instance for each actor type" or something.

@@ +315,5 @@
>          let { from } = aResponse;
> +        try {
> +          aResponse = after.call(this, aResponse);
> +        } catch (e) {
> +          DevToolsUtils.reportException("DebuggerClient.requester after callback", e);

Should we return early when |before| or |after| callbacks fail? I imagine that we could get some really weird and hard to debug bugs down the line when one of those callbacks fails and we just keep going, pretending like it didn't.

Perhaps we can just wrap the returned function and |request| callback in |DevToolsUtils.makeInfallible| so that the error is still logged but we don't try and continue handling the packet?

@@ +417,3 @@
>        }
> +
> +      for each (let client in self._threadClients) {

Please, no more non-standardized spidermonkey specific features. Might be worth it to make |_threadClients| a |Map| so that you can iterate over it with for/of and it will better reflect the way we are using it. Up to you.

Also, can we start using arrow functions instead of closing over |self|?

@@ +417,4 @@
>        }
> +
> +      for each (let client in self._threadClients) {
> +        continuation = client.detach.bind(client, continuation);

Wouldn't it be better to detach from all of these threads concurrently? Also, we should abstract the repeated many-client-detach logic from the detaching of specific things functions. Something like this:

const detachClients = (clients, next) => {
  const keys = Object.keys(clients);
  const total = keys.length;
  let numFinished = 0;

  for (let k of keys) {
    clients[k].detach(() => {
      if (numFinished++ === total) {
        next();
      }
    });
  }
};

detachClients(this._threadClients, () => {
  detachClients(this._tabClients, closeTransport);
});

@@ +520,5 @@
>     *        - useSourceMaps: whether to use source maps or not.
>     */
>    attachThread: function (aThreadActor, aOnResponse, aOptions={}) {
> +    if (this._threadClients[aThreadActor]) {
> +      aOnResponse(null, this._threadClients[aThreadActor]);

Can we execute the callback on the next tick for consistency with when we have to actually send a packet?

@@ +549,5 @@
>     *        (which will be undefined on error).
>     */
>    attachTracer: function (aTraceActor, aOnResponse) {
> +    if (this._tracerClients[aTraceActor]) {
> +      aOnResponse(null, this._tracerClients[aTraceActor]);

ditto

@@ +1047,5 @@
> +   */
> +  reconfigure: DebuggerClient.requester({
> +    type: "reconfigure",
> +    options: args(0)
> +  }, {}),

Nit: Should probably add a telemetry probe for this request, since we have one for every other |DebuggerClient.requester|.

@@ +1121,5 @@
>   *        The actor ID for this thread.
>   */
>  function ThreadClient(aClient, aActor) {
> +  this._parent = aClient;
> +  this._client = aClient instanceof DebuggerClient ? aClient : aClient._client;

Nit: |TabClient|'s |_client| property shouldn't be underscore prefixed if we are referring to it from |ThreadClient|.

@@ +1205,5 @@
> +   */
> +  reconfigure: DebuggerClient.requester({
> +    type: "reconfigure",
> +    options: args(0)
> +  }, {}),

ditto re: telemetry probe

@@ +2029,5 @@
>    this._form = aForm;
>    this._isBlackBoxed = aForm.isBlackBoxed;
>    this._isPrettyPrinted = aForm.isPrettyPrinted;
> +  this._activeThread = aClient;
> +  this._client = aClient._client;

ditto re: _-prefixed properties being used outside their class

::: toolkit/devtools/server/tests/unit/test_objectgrips-13.js
@@ -62,5 @@
> -    obj.getDefinitionSite(() => do_check_true(false));
> -  } catch (e) {
> -    gThreadClient.resume(() => finishClient(gClient));
> -  }
> -}

Whoa, why are we removing this test? Please put it back!

Comment 13

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

I'll make all the other changes ASAP, but there are a few points I'd like to comment on.

(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> ::: browser/devtools/debugger/test/browser_dbg_source-maps-02.js
> @@ +63,5 @@
> >  
> >  function testSetBreakpoint() {
> >    let deferred = promise.defer();
> >  
> > +  gDebugger.gThreadClient.interrupt(aResponse => {
> 
> Doesn't |setBreakpoint| do an |interrupt| for us? Why is this needed?

It shouldn't be and I was actually the one who took it out before. hat happened is that I actually tried to remove the interrupt from browser_dbg_source-maps-03.js, but the problem is that I was getting leaks without it, and I didn't have time to track them down. I'll take another quick look on Monday now that all the other tests pass.

> @@ +315,5 @@
> >          let { from } = aResponse;
> > +        try {
> > +          aResponse = after.call(this, aResponse);
> > +        } catch (e) {
> > +          DevToolsUtils.reportException("DebuggerClient.requester after callback", e);
> 
> Should we return early when |before| or |after| callbacks fail? I imagine
> that we could get some really weird and hard to debug bugs down the line
> when one of those callbacks fails and we just keep going, pretending like it
> didn't.

That was my original plan too, but then I thought that only a few tests were already affected by this (timing issues mostly). It might also be a good idea to be more resilient against such failures, as the most common use of before/after functions is to handle non-critical things like caching, cleanups, etc. I will change it if you feel otherwise though.

> Perhaps we can just wrap the returned function and |request| callback in
> |DevToolsUtils.makeInfallible| so that the error is still logged but we
> don't try and continue handling the packet?

You mean wrap all |before|/|after| declarations with makeInfallible (too much churn) or wrap before |call| and avoid the try/catch (makeInfallible(after).call(...))? I'm OK with the latter as it's cleaner for logging and continuing, but it won't return early without a try/catch.

> @@ +417,3 @@
> >        }
> > +
> > +      for each (let client in self._threadClients) {
> 
> Please, no more non-standardized spidermonkey specific features. Might be
> worth it to make |_threadClients| a |Map| so that you can iterate over it
> with for/of and it will better reflect the way we are using it. Up to you.
> 
> Also, can we start using arrow functions instead of closing over |self|?

See Mihai's pre-existing comment at the top of that function for the rationale behind avoiding arrow functions in this case. I agree about for..each though.

> @@ +417,4 @@
> >        }
> > +
> > +      for each (let client in self._threadClients) {
> > +        continuation = client.detach.bind(client, continuation);
> 
> Wouldn't it be better to detach from all of these threads concurrently?
> Also, we should abstract the repeated many-client-detach logic from the
> detaching of specific things functions. Something like this:
> 
> const detachClients = (clients, next) => {
>   const keys = Object.keys(clients);
>   const total = keys.length;
>   let numFinished = 0;
> 
>   for (let k of keys) {
>     clients[k].detach(() => {
>       if (numFinished++ === total) {
>         next();
>       }
>     });
>   }
> };
> 
> detachClients(this._threadClients, () => {
>   detachClients(this._tabClients, closeTransport);
> });

I like that. I was waiting for dbg-client.jsm to become promise-aware before cleaning this up, but this looks like an improvement in the meantime.

> @@ +1047,5 @@
> > +   */
> > +  reconfigure: DebuggerClient.requester({
> > +    type: "reconfigure",
> > +    options: args(0)
> > +  }, {}),
> 
> Nit: Should probably add a telemetry probe for this request, since we have
> one for every other |DebuggerClient.requester|.

I thought about it, but this goes back to our related discussion on the team call about how useful these probes are. I'll just add it for now until we have thought about the general issue some more.

> ::: toolkit/devtools/server/tests/unit/test_objectgrips-13.js
> @@ -62,5 @@
> > -    obj.getDefinitionSite(() => do_check_true(false));
> > -  } catch (e) {
> > -    gThreadClient.resume(() => finishClient(gClient));
> > -  }
> > -}
> 
> Whoa, why are we removing this test? Please put it back!

The problem with this test is that it now causes a |before| callback to log an error, which our test harness considers fatal. I thought about adding a special flag for test use that would disable logging, but it seemed overkill for such a trivial test, since such a request is going to fail in the backend anyway.

Would you prefer it if I changed the test to use obj.client.request instead of the ObjectClient helper method?

Comment 14

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

(In reply to Panos Astithas [:past] from comment #13)
> I'll make all the other changes ASAP, but there are a few points I'd like to
> comment on.
> 
> (In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> > ::: browser/devtools/debugger/test/browser_dbg_source-maps-02.js
> > @@ +63,5 @@
> > >  
> > >  function testSetBreakpoint() {
> > >    let deferred = promise.defer();
> > >  
> > > +  gDebugger.gThreadClient.interrupt(aResponse => {
> > 
> > Doesn't |setBreakpoint| do an |interrupt| for us? Why is this needed?
> 
> It shouldn't be and I was actually the one who took it out before. hat
> happened is that I actually tried to remove the interrupt from
> browser_dbg_source-maps-03.js, but the problem is that I was getting leaks
> without it, and I didn't have time to track them down. I'll take another
> quick look on Monday now that all the other tests pass.

Ah, ok cool.

> 
> > @@ +315,5 @@
> > >          let { from } = aResponse;
> > > +        try {
> > > +          aResponse = after.call(this, aResponse);
> > > +        } catch (e) {
> > > +          DevToolsUtils.reportException("DebuggerClient.requester after callback", e);
> > 
> > Should we return early when |before| or |after| callbacks fail? I imagine
> > that we could get some really weird and hard to debug bugs down the line
> > when one of those callbacks fails and we just keep going, pretending like it
> > didn't.
> 
> That was my original plan too, but then I thought that only a few tests were
> already affected by this (timing issues mostly). It might also be a good
> idea to be more resilient against such failures, as the most common use of
> before/after functions is to handle non-critical things like caching,
> cleanups, etc. I will change it if you feel otherwise though.
> 
> > Perhaps we can just wrap the returned function and |request| callback in
> > |DevToolsUtils.makeInfallible| so that the error is still logged but we
> > don't try and continue handling the packet?
> 
> You mean wrap all |before|/|after| declarations with makeInfallible (too
> much churn) or wrap before |call| and avoid the try/catch
> (makeInfallible(after).call(...))? I'm OK with the latter as it's cleaner
> for logging and continuing, but it won't return early without a try/catch.

What I meant was this:

 DebuggerClient.requester = function DC_requester(aPacketSkeleton, { telemetry,
                                                  before, after }) {
-  return function (...args) {
+  return DevToolsUtils.makeInfallible(function (...args) {
     let histogram, startTime;

...
 
-    this.request(outgoingPacket, function (aResponse) {
+    this.request(outgoingPacket, DevToolsUtils.makeInfallible(function (aResponse) {

I defer to your judgement with regards to whether to actually make these changes or just log+continue when we hit errors.

> 
> > @@ +417,3 @@
> > >        }
> > > +
> > > +      for each (let client in self._threadClients) {
> > 
> > Please, no more non-standardized spidermonkey specific features. Might be
> > worth it to make |_threadClients| a |Map| so that you can iterate over it
> > with for/of and it will better reflect the way we are using it. Up to you.
> > 
> > Also, can we start using arrow functions instead of closing over |self|?
> 
> See Mihai's pre-existing comment at the top of that function for the
> rationale behind avoiding arrow functions in this case. I agree about
> for..each though.

I guess...

Not the way I would write it, but I'm not going to drag that into this :P

> 
> > @@ +417,4 @@
> > >        }
> > > +
> > > +      for each (let client in self._threadClients) {
> > > +        continuation = client.detach.bind(client, continuation);
> > 
> > Wouldn't it be better to detach from all of these threads concurrently?
> > Also, we should abstract the repeated many-client-detach logic from the
> > detaching of specific things functions. Something like this:
> > 
> > const detachClients = (clients, next) => {
> >   const keys = Object.keys(clients);
> >   const total = keys.length;
> >   let numFinished = 0;
> > 
> >   for (let k of keys) {
> >     clients[k].detach(() => {
> >       if (numFinished++ === total) {
> >         next();
> >       }
> >     });
> >   }
> > };
> > 
> > detachClients(this._threadClients, () => {
> >   detachClients(this._tabClients, closeTransport);
> > });
> 
> I like that. I was waiting for dbg-client.jsm to become promise-aware before
> cleaning this up, but this looks like an improvement in the meantime.

Aside: do you have a bug # for making the client promise aware?

> 
> > @@ +1047,5 @@
> > > +   */
> > > +  reconfigure: DebuggerClient.requester({
> > > +    type: "reconfigure",
> > > +    options: args(0)
> > > +  }, {}),
> > 
> > Nit: Should probably add a telemetry probe for this request, since we have
> > one for every other |DebuggerClient.requester|.
> 
> I thought about it, but this goes back to our related discussion on the team
> call about how useful these probes are. I'll just add it for now until we
> have thought about the general issue some more.

I'm personally of the opinion that we should be measuring everything (within reason). Never know when we might care about the results. I'm all for removing *deprecated* probes, however.

> 
> > ::: toolkit/devtools/server/tests/unit/test_objectgrips-13.js
> > @@ -62,5 @@
> > > -    obj.getDefinitionSite(() => do_check_true(false));
> > > -  } catch (e) {
> > > -    gThreadClient.resume(() => finishClient(gClient));
> > > -  }
> > > -}
> > 
> > Whoa, why are we removing this test? Please put it back!
> 
> The problem with this test is that it now causes a |before| callback to log
> an error, which our test harness considers fatal. I thought about adding a
> special flag for test use that would disable logging, but it seemed overkill
> for such a trivial test, since such a request is going to fail in the
> backend anyway.
> 
> Would you prefer it if I changed the test to use obj.client.request instead
> of the ObjectClient helper method?

Ah, yes ok. We've run into this problem a couple of other times and it seems like it would be nice to have a function to flip the fail-on-log behavior on/off in general, rather than taking the approach of flipping on/off the reporting in specific places (which is what we do now, IIRC). If you agree, want to file a follow up, good-first-bug?

Comment 15

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

Attachment: Make the debugger frontend cope with an already connected target v5

This version includes all the changes requested by Nick, even some that I was initially somewhat sceptical about. That's what a good reviewer can do to you! Some of the changes had broad repercussions so the patch is a little bigger, but also better!

One additional change in this version is that now tab and console clients are also reused when attaching, because why not? Reusing tab clients highlighted the fact that client.attach() callbacks may need a cached response object as well, which I also fixed.

Comment 16

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

Attachment: v4-v5 interdiff

Interdiff between v4 and v5 for glancing at the changes.

Comment 17

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #14)
> (In reply to Panos Astithas [:past] from comment #13)
> > It shouldn't be and I was actually the one who took it out before. hat
> > happened is that I actually tried to remove the interrupt from
> > browser_dbg_source-maps-03.js, but the problem is that I was getting leaks
> > without it, and I didn't have time to track them down. I'll take another
> > quick look on Monday now that all the other tests pass.
> 
> Ah, ok cool.

Fixed them both.

> I defer to your judgement with regards to whether to actually make these
> changes or just log+continue when we hit errors.

I decided to use makeInfallible, but not return early. My argument was that it's pointless unless the caller of the client method gets an actual error response (so the failure is visible and can be properly handled), and that would entail patching the server's successful response in the |after| case, which is yucky.

> > See Mihai's pre-existing comment at the top of that function for the
> > rationale behind avoiding arrow functions in this case. I agree about
> > for..each though.
> 
> I guess...
> 
> Not the way I would write it, but I'm not going to drag that into this :P

OK, fine, I admit it: yours is better! Changed :-)

> Aside: do you have a bug # for making the client promise aware?

Bug 906202.

> > > Whoa, why are we removing this test? Please put it back!
> > 
> > The problem with this test is that it now causes a |before| callback to log
> > an error, which our test harness considers fatal. I thought about adding a
> > special flag for test use that would disable logging, but it seemed overkill
> > for such a trivial test, since such a request is going to fail in the
> > backend anyway.
> > 
> > Would you prefer it if I changed the test to use obj.client.request instead
> > of the ObjectClient helper method?
> 
> Ah, yes ok. We've run into this problem a couple of other times and it seems
> like it would be nice to have a function to flip the fail-on-log behavior
> on/off in general, rather than taking the approach of flipping on/off the
> reporting in specific places (which is what we do now, IIRC). If you agree,
> want to file a follow up, good-first-bug?

I don't think we ever had to do that, or at least I can't find any such case right now. It seems we always manage to find another way to do it. In this case I chose to test that the request fails on the server instead of the client and it works out well.

Comment 18

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

(In reply to Panos Astithas [:past] from comment #17)
> > Aside: do you have a bug # for making the client promise aware?
> 
> Bug 906202.

Embarassing that I filed it :-| Thanks!

Comment 19

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=d850f2bb218e

Comment 20

[Paul Rouget [:paul]]

Could this work impact backward compatibility? (debugging fxos 1.2 with gecko 29)

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8359824 [details] [diff] [review]
Make the debugger frontend cope with an already connected target v5

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

Paul delegated his review of framework changes to me.

Everything looks fine to me!  

My only worry would be same as Paul's, about backward compatibility.  But after looking at the debugger changes, nothing struck me as causing trouble for older clients or servers.  The main issue being fixed here appears to be client-side only and doesn't involve breaking remote API changes, so fixed clients would get rid of this issue, but still be able to connect to older servers.  And the server-side changes are quite small, mostly just the issue for Firebug's use case, but that should still allow connection by older clients.  Does all this seem right Panos?

::: b2g/chrome/content/shell.js
@@ +1085,5 @@
>          DebuggerServer.registerModule("devtools/server/actors/inspector");
>          DebuggerServer.registerModule("devtools/server/actors/styleeditor");
>          DebuggerServer.registerModule("devtools/server/actors/stylesheets");
> +        DebuggerServer.registerModule("devtools/server/actors/tracer");
> +        DebuggerServer.registerModule("devtools/server/actors/webgl");

This likely collides with bug 961392, but then you reviewed that so you probably know. ;)

Comment 22

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

Yes, that's right, no protocol changes were made in this patch that would break backwards compatibility.

Comment 23

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

https://hg.mozilla.org/integration/fx-team/rev/445a480ba7d1

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/445a480ba7d1