764913 - DebuggerServer doesn't work with multiple concurrent debuggers

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P2)

Description

[Jonathan Griffin (:jgriffin)]

The DebuggerServer object is currently a singleton, and the way it is implemented, it doesn't play nicely when multiple implementers attempt to use it, namely the remote debugger and Marionette. 

One issue is that only one implementer can open a listener, due to http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.js#135.

There are probably other issues.

:past, :fzzzy, and :mdas have been discussing solutions in channel, so I'll leave them to comment here.

Comment 1

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

I'll look into this.

Comment 2

[Jonathan Griffin (:jgriffin)]

This problem breaks Marionette tests in B2G, but I've proposed a temporary fix to get it working again in bug 766206.

Comment 3

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

As I told mdas, my current plan is to finish bug 753401 first and then work on making Marionette use that API. It's not my #1 priority at the moment, since jimb who will need to review that patch is on PTO (and we also have some debugger fixes that we need for Aurora).

Comment 4

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

Attachment: Convert marionette actors to use the debugger server API

So this is a draft version of what I had in mind. If we get the Marionette actors to use the server's extension API, there will be no conflicts and both a debugger and a marionette client can connect simultaneously, as well as the web console, the profiler, etc.

That would entail removing the component and registering the actors in the place where the Debugger Server is normally initialized for each product. I've used b2g for testing and there we start the server in the ContentStart listener. In case this is too late for Marionette, we can try to move it earlier.

I only made two changes in the Marionette actors: removing the createRootActor function and register the main actor with the server. I also changed the client to retrieve the actor id properly and that's pretty much it. My python experience is pretty much non-existent, so instead of learning python for this task, I figured I'd sketch the general idea for the experts, and then hopefully you can take it from there. I did confirm that the initial protocol interactions are as expected, but beyond that I can't get the tests to run properly, so I don't know if the actors or the client needs fixing.

Let me know what you think.

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 683293 [details] [diff] [review]
Convert marionette actors to use the debugger server API

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

If you've tested it with the changed client and it's working, then this looks good to me!

::: b2g/chrome/content/shell.js
@@ +754,5 @@
> +    try {
> +      withMarionette = Services.prefs.getBoolPref('marionette.defaultPrefs.enabled');
> +    } catch(e) {}
> +    if (withMarionette) {
> +      dump("Marionette enabled\n");

Should we keep the dump statement or get rid of it? I don't see much point to it, since marionette will create a log if it's been successfully kicked off

::: testing/marionette/components/marionettecomponent.js
@@ +102,5 @@
>    },
>  
>  };
>  
> +// this.NSGetFactory = XPCOMUtils.generateNSGetFactory([MarionetteComponent]);

I think it's safe to remove this line.

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 683293 [details] [diff] [review]
Convert marionette actors to use the debugger server API

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

I understand the intent here, but unfortunately, it blows things up rather badly:

#1, it breaks all the WebAPI and SpecialPowers tests.  I don't really understand now why this happens, but I suspect it's because it's being loaded from inside shell.js, rather than earlier as happens now.

#2, once Marionette runs on the device, it ends up being disabled.  This happens due to http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#764; something apparently flips this pref to off, even if it is set to true in b2g.js.  Is this because of http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.js#105?  I'm not sure.

#3, not critical, but needed to land something like this, would be to let ENABLE_MARIONETTE=1 in mozconfig set the prefs that would allow Marionette to be enabled in b2g, fennec, and firefox.

Is this all to fix bug 800138?

Comment 7

[Jonathan Griffin (:jgriffin)]

For #1, I dug a little more.  If I run an SMS test it dies with:

TEST-UNEXPECTED-FAIL | test_incoming.js | JavascriptException: TypeError: sms is null

Comment 8

[Jonathan Griffin (:jgriffin)]

My guess is that it's something like that the fact that loading Marionette this way causes it to be executed in a context (maybe the system app?) that doesn't have sufficient permissions to access SMS/telephony, etc.  If that's the case, loading it in shell.js may be a non-starter.

@philikon, do you have any idea why loading the debugger server/marionette in shell.js would break webapi tests in this way?

Comment 9

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

(In reply to Malini Das [:mdas] from comment #5)
> If you've tested it with the changed client and it's working, then this
> looks good to me!

So I haven't been able to get marionette tests to run successfully locally, without these changes even, either for desktop b2g or a device. I have only managed to get the marionette client communicating with the server and make sure that at least the initial packets were transmitted properly, but then tests just failed and I couldn't figure out the problems. And since I don't have many cycles to dedicate to this work for the next few weeks, I thought I'd better get some early feedback from you guys on the approach, to make sure this is a viable approach.

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> I understand the intent here, but unfortunately, it blows things up rather
> badly:

OK, getting the implementation idea across was my main goal here, I know that it doesn't work yet (although I could use some help in debugging this!).

> #1, it breaks all the WebAPI and SpecialPowers tests.  I don't really
> understand now why this happens, but I suspect it's because it's being
> loaded from inside shell.js, rather than earlier as happens now.

I've only tried this against desktop b2g and there I only ran the browser tests (--type=browser), but they seemed to block indefinitely while running the first test, right after the first packets were exchanged. I need to understand more about the internals of marionette to fix these problems, and my lack of python skillz makes this harder than it would otherwise have been.

> #2, once Marionette runs on the device, it ends up being disabled.  This
> happens due to
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.
> js#764; something apparently flips this pref to off, even if it is set to
> true in b2g.js.  Is this because of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/
> server/dbg-server.js#105?  I'm not sure.

It can't be because of that, since DS__defaultAllowConnection is overridden with the function parameter that we pass to init/initTransport, which always returns true no matter what. I'll try to test this patch on a device to see if I can figure this out.

> #3, not critical, but needed to land something like this, would be to let
> ENABLE_MARIONETTE=1 in mozconfig set the prefs that would allow Marionette
> to be enabled in b2g, fennec, and firefox.
> 
> Is this all to fix bug 800138?

Well, that was the reason for expediting this, because I wanted to see if resolving this larger issue would automatically fix that problem, which is proving hard to debug without deep platform knowledge. But aligning marionette better with the rest of the debugger server extensions is valuable on its own:
- it will mean that we can stop choosing what to enable (the debugger or marionette), which may even help in debugging marionette tests
- it should simplify the code in marionette that now has to deal with the low-level protocol bits, since that will be taken care of by the server, and hopefully avoid the breakage that occasionally arose when we tweaked the debugger server internals
- it will ensure that marionette can use (and perhaps more importantly) test forthcoming protocol changes like support for binary streams and multiple content process without significant effort

And at the same time you will continue to be able to modify the marionette protocol independently and without coordination with us, because that's exactly how actor-to-client communication is supposed to work in the remote protocol.

Comment 10

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

Bug 797529 will follow a different approach, effectively removing Marionette's dependency on the debugger server.