797529 - Remove dependency on remote debugger's dbg-server

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Since we only really use the remote-debugger code for our transport layer (using remote-debugger's dbg-transport) and won't really employ the Actor pool idea, it would make sense to remove our use of dbg-server. 

Removing this dependency will reduce the number of regressions we'll see in marionette whenever the remote-debugger gets updated.

Comment 1

[Jonathan Griffin (:jgriffin)]

We'll want to continue to use the same socket protocol as we currently do.  We may be able to re-use dbg-transport.js with some jiggering, or we could roll our own.

About the only thing we get from dbg-server.js currently is message dispatching, so we'd have to implement a replacement for that.

Fixing this will allow us to add support for running the remote debugger concurrently with Marionette, and will provide a path for supporting nested processes in B2G in the future (bug 845401).

Comment 2

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

A few things to note about our current protocol. Because of our use of dbg-server:

* We have a specific way of connecting to the server we want. First the client implemented this RootActor stuff that has a specific way of starting a connection. First we have to connect, send a packet with the command 'getMarionetteID', then keep track of the ID we get and use it in every subsequent command, which leads to my next note...

* We needed to add a 'to' key in each request to the server so we could send the marionette ID to the dispatcher. This isn't part of WebDriver, and can actually break some WebDriver commands because some commands need the 'to' key for their own arguments. Also, each response from the server has a 'from' key, which should also be removed.

So when ripping out the dbg-server, we should keep in mind that we'd like to the "getMarionetteID" logic, and all the "to" and "from" keys in our json packets.

Comment 3

[William Lachance (:wlach)]

Random advice from mdas and jgriffin on solving this bug:

<mdas> wlach: I'd start at looking how it currently works. We start our server here  testing/marionette/components/marionettecomponent.js, using the DebuggerServer
<mdas> wlach: we're using the devtools debugger server which is  /Users/mdas/Code/mozilla-central/toolkit/devtools/debugger/server/dbg-server.js
<mdas> wlach: we want to stop our use of DebuggerServer, but reuse their debugger transport layer since it handles all the json packet stuff for us. That file is /Users/mdas/Code/mozilla-central/toolkit/devtools/debugger/dbg-transport.js
<mdas> ideally, we should be able to just use this transport layer without doing anything special, but I haven't looked into it
<mdas> wlach: I'd also take a look at how marionette is architected. I wrote it so it works with the debugger architecture (root actor, actor pools, etc). I'd take a look at how testing/marionette/marionette-actors.js works when considering dbg-server.js code. We'd like to get rid of our use of a 'root' actor
<mdas> it messes with stuff
<wlach> mdas: I understood that we didn't want the actor abstraction stuff at all?
<mdas> meaning, our use of this root actor relaying to another actor breaks webdriver compatibility
<mdas> wlach: exactly1
<mdas> wlach: that would be ideal, we just need the server to be marionette itself, not a relay point to a bunch of other services
<wlach> mdas: tbh I don't really understand the actor model right now, but I guess the code will reveal al...
<mdas> wlach: yeah it will, the actor model is that there's one master thing listening in on the port. You ping that master and ask it for the id of a service you want. It gives you the id, and then you send all the commands you want to that service through the master using that id.
<jgriffin> wlach: btw, for the Marionette stuff, I'd recommend working with desktop Firefox for primary development….much lighter weight than b2g and equally effective for that task

Comment 4

[William Lachance (:wlach)]

Attachment: First cut of patch to remove dependency

Here's a first pass at this; at least the basics seem to be working ok. Some concerns:

1. I had to manually define a dumpn function and reference a remote debugger preference inside marionette. We probably don't want to do this.
2. I merged all the root actor stuff into the marionettedriver actor, but didn't rename anything. I think we probably want to refactor this stuff more. Maybe stop calling this thing an "actor". Maybe merge into the new MarionetteServerConnection class. Not sure, other ideas welcome. :)

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 749370 [details] [diff] [review]
First cut of patch to remove dependency

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

This looks great!  I already love all the simplification.

I guess we probably need to keep the dumpn function, but probably want to use marionette's logger with a debug level (instead of dump); this way the messages won't appear unless you take the time to set the logger's level to debug.  (We may want a pref to control that, but that's probably a separate bug.)  We definitely don't want to use the debugger's pref to control this, I agree.

I'd like to get rid of MarionetteDriverActor and all the actor terminology (except as needed by the protocol), since it's no longer relevant, so I think merging the actor with MarionetteServerConnection makes a lot of sense.  At the same time, you could move this new code into marionette-actors.js and rename it to marionette-server.js, and we'd be nearly actor-free.

Comment 6

[William Lachance (:wlach)]

Attachment: Remove dependency on debugging server

Ok, here's my first real attempt at making this work. Made all the adjustments you suggested. I made the "dumpn" function call marionette's logger with level "trace" (the lowest level). It seems like those messages are still being output to marionette's log though, which seems like it might be more detail than we really want.

Anyway, I'm sure this could still use improvement but I'm fairly happy with how this is turning out...

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 749528 [details] [diff] [review]
Remove dependency on debugging server

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

This is awesome!  After fixing the force-local issue, we should run it through try, and I can run the gaia-ui-tests on it with an unagi, to cover all our bases.

::: testing/marionette/components/marionettecomponent.js
@@ +28,5 @@
>  function MarionetteComponent() {
>    this._loaded = false;
>    // set up the logger
>    this.logger = Log4Moz.repository.getLogger("Marionette");
>    this.logger.level = Log4Moz.Level["INFO"];

This is actually a preexisting bug which is responsible for all the trace statements appearing in the log. This should be Log4Moz.Level["Info"].

::: testing/marionette/marionette-server.js
@@ +165,5 @@
> +          e));
> +      }
> +    } else {
> +      this.conn.send({ error: "unrecognizedPacketType",
> +                       message: ('Actor ' + this.actorID + ' does not ' +

Since actorID is now rather artificial, I think we could just say 'Marionette does not '...

@@ +171,5 @@
> +                                 aPacket.type + '"') });
> +    }
> +  },
> +
> +  onClosed: function DSC_onClosed(aStatus) {

nit: should change these function prefixes from DSC (DebuggerServerConnection) to MSC

@@ +187,5 @@
> +   * in use.  If this replaces a frame-specific ChromeMessageSender, it removes the message
> +   * listeners from that sender, and then puts the corresponding frame script "to sleep",
> +   * which removes most of the message listeners from it as well.
> +   */
> +  switchToGlobalMessageManager: function MDA_switchToGlobalMM() {

nit: should change all the MDA prefixes (MarionetteDriverActor) to MSC

@@ +2359,5 @@
> + * Marionette server -- this class holds a reference to a socket and creates
> + * MarionetteServerConnection objects as needed.
> + */
> +function MarionetteServer(port) {
> +  let flags = Ci.nsIServerSocket.KeepWhenOffline;

Since we're no longer able to use the debugger server to force local-only connections, we'll have to do it here.  We already have a pref, "marionette.force-local", that we can use to do this; if that's set, we should: flags |= Ci.nsIServerSocket.LoopbackOnly;  If that pref isn't set, we should default to false for B2G or true otherwise.

See the existing logic at http://mxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionettecomponent.js#82.

@@ +2361,5 @@
> + */
> +function MarionetteServer(port) {
> +  let flags = Ci.nsIServerSocket.KeepWhenOffline;
> +  var socket = new ServerSocket(port, flags, 4);
> +  dump(">>> listening on port "+socket.port+"\n");

maybe use logger.info for this?

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 749528 [details] [diff] [review]
Remove dependency on debugging server

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

::: testing/marionette/components/marionettecomponent.js
@@ +22,5 @@
>  
> +let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +               .getService(Ci.mozIJSSubScriptLoader);
> +
> +loader.loadSubScript("chrome://marionette/content/marionette-server.js");

I just realized this will have some bad side effects, since we're loading this unconditionally, which means the bypassOffline logic in that file will get run unconditionally.  We shoud loadSubScript this file right before we use it, I think.

Comment 9

[William Lachance (:wlach)]

Attachment: Updated patch

I think this updated patch covers the issues in the review. Here's a diff of just the changes since then:

http://www.pastebin.mozilla.org/2428564

Comment 10

[William Lachance (:wlach)]

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

Comment 11

[Jonathan Griffin (:jgriffin)]

Can you make another try run for B2G with '-u marionette-webapi'?  '-u marionette' only catches the desktop cases.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 752406 [details] [diff] [review]
Updated patch

Awesome, looks great.

If you haven't, can you make sure you use 'hg mv' to rename marionette-actors.js to marionette-servers?  This lets us preserve history and makes it easier for other in-progress patches to be merged in.  I can't tell for sure whether this was done or not, but I'm guessing not.

One of us (I'm happy to volunteer) should make a local unagi or inari build with this patch and make sure Marionette still works as expected.

Comment 13

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> Comment on attachment 752406 [details] [diff] [review]
> Updated patch
> 
> Awesome, looks great.
> 
> If you haven't, can you make sure you use 'hg mv' to rename
> marionette-actors.js to marionette-servers?  This lets us preserve history
> and makes it easier for other in-progress patches to be merged in.  I can't
> tell for sure whether this was done or not, but I'm guessing not.

No, I don't think so. I'll do this before I commit.

> One of us (I'm happy to volunteer) should make a local unagi or inari build
> with this patch and make sure Marionette still works as expected.

If you could, I'd much appreciate it!

Comment 14

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #11)
> Can you make another try run for B2G with '-u marionette-webapi'?  '-u
> marionette' only catches the desktop cases.

Done: https://tbpl.mozilla.org/?tree=Try&rev=763b6c87354d

Comment 15

[Jonathan Griffin (:jgriffin)]

On an unagi, this results in:

I/Gecko   (  110): 1369242382697	Marionette	INFO	marionette initializing at final-ui-startup
I/Gecko   (  110): 1369242382753	Marionette	ERROR	exception: ReferenceError, Cc is not defined

This is likely a side effect of bug 798491.  To get around it, you can probably load marionette-server.js into a local object (rather than the default global object), and then restore the definitions of Cc, Ci, etc in marionette-server.js.

Comment 16

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #15)
> On an unagi, this results in:
> 
> I/Gecko   (  110): 1369242382697	Marionette	INFO	marionette initializing at
> final-ui-startup
> I/Gecko   (  110): 1369242382753	Marionette	ERROR	exception: ReferenceError,
> Cc is not defined
> 
> This is likely a side effect of bug 798491.  To get around it, you can
> probably load marionette-server.js into a local object (rather than the
> default global object), and then restore the definitions of Cc, Ci, etc in
> marionette-server.js.

My idea of how to fixed this did fix it, but exposed another problem:

E/GeckoConsole(  111): [JavaScript Error: "SpecialPowersObserverAPI is not defined" {file: "chrome://specialpowers/content/SpecialPowersObserver.js" line: 37}]

Somehow we're not being loaded in the correct scope.  I'll play around with this a bit more.

Comment 17

[Jonathan Griffin (:jgriffin)]

Attachment: Version that works on B2G (WIP)

This is more convoluted than I thought.  In order to get this to work on B2G, I basically had to reproduce the way the file was originally loaded in the remote debugger.  I'm sure there's a better way to do it; we might move SpecialPowers initialization out of marionette-server.js, or change that .js into a .jsm and load it via Cu.import.

Comment 18

[Jonathan Griffin (:jgriffin)]

I didn't mean to reassign this; bzexport did it for me.

Comment 19

[Jonathan Griffin (:jgriffin)]

Attachment: Version that works on B2G (WIP)

Rebased version that uses hg mv, which makes it much easier to work with.

Comment 20

[Jonathan Griffin (:jgriffin)]

splinter doesn't show this, but 'hg qdiff' shows only the lines which differ between marionette-server.js and the original marionette-actors.js.

Comment 21

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server

This works on B2G without any craziness.  It requires a 3-line patch to transport.js, which I'll flag for separate review.  Can you look over to make sure nothing looks wrong?  I'll attach a separate diff which makes reviewing the changes easier.

Comment 22

[Jonathan Griffin (:jgriffin)]

Attachment: qdiff output (easy to review)

Comment 23

[Jonathan Griffin (:jgriffin)]

Attachment: Define Cc, Ci, etc, if needed,

With this Marionette patch, we're importing transport.js into a scope without Cc, etc defined, so we check for that and define if needed.  We'll make sure all the unit tests, etc, pass before landing.

Comment 24

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b063a2ffa5fa

Comment 25

[William Lachance (:wlach)]

Comment on attachment 753978 [details] [diff] [review]
Remove dependence on debugger server

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

This looks good to me, minus a small issue with log levels. As discussed on irc, it's probably best to get someone else to double check the specialpowers stuff who is more familiar with that code.

::: testing/marionette/components/marionettecomponent.js
@@ +24,5 @@
>  function MarionetteComponent() {
>    this._loaded = false;
>    // set up the logger
>    this.logger = Log4Moz.repository.getLogger("Marionette");
> +  this.logger.level = Log4Moz.Level["Trace"];

As discussed on irc, we don't want this. :)

Comment 26

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

(In reply to Jonathan Griffin (:jgriffin) from comment #23)
> Created attachment 753981 [details] [diff] [review]
> Define Cc, Ci, etc, if needed,
> 
> With this Marionette patch, we're importing transport.js into a scope
> without Cc, etc defined, so we check for that and define if needed.  We'll
> make sure all the unit tests, etc, pass before landing.

It looks like Cc and friends are already defined in marionette-server.js, so I'm wondering why we need to redefine them in transport.js. But more importantly, there is ongoing work in bug 859372 that is about to land very soon and changes the way loading works in the debugger server. Perhaps you'd like to wait for that to land (or rebase on top of it) and start using the new loader instead, which might require different changes to marionette-server.js?

Comment 27

[Jonathan Griffin (:jgriffin)]

It has to do with compartment sharing in B2G; the Cc definition from marionette-server.js isn't inherited by subscripts there.  See bug 798491.  Without this, we get:  "ReferenceError, Cc is not defined".

But if bug 859372 is loading soon, we can wait, although in this case we'll likely have to conditionally define "require" instead.

Comment 28

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

(In reply to Jonathan Griffin (:jgriffin) from comment #27)
> It has to do with compartment sharing in B2G; the Cc definition from
> marionette-server.js isn't inherited by subscripts there.  See bug 798491. 
> Without this, we get:  "ReferenceError, Cc is not defined".

Ah, how about sticking them on |this| then?

> But if bug 859372 is loading soon, we can wait, although in this case we'll
> likely have to conditionally define "require" instead.

I think it's at the top of Dave's TODO list, but I'll let him speak for himself.

Comment 29

[Dave Camp (:dcamp)]

It's near the top of my TODO list, but I wouldn't hold up waiting for it.  The Cc setup should be ok in the short term until that lands.

Comment 30

[Jonathan Griffin (:jgriffin)]

Comment on attachment 753981 [details] [diff] [review]
Define Cc, Ci, etc, if needed,

past is right, we can use 'this' to get around having to conditionally define Cc etc in this file.

Comment 31

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server,

Define Cc et al with 'this', so it's defined for subscripts.  pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=83163799924b

Comment 32

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server,

The previous try run was bad due to some changes I missed when I rebased this using hg mv.  Fixed, and pushed to try again:  https://tbpl.mozilla.org/?tree=Try&rev=140027ee3b88

Comment 33

[Jonathan Griffin (:jgriffin)]

The B2G red is due to another shared-compartment issue I hadn't fixed, in marionette-simpletest.js.  Fixed and pushed to try again:  

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

Comment 34

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server,

Finally got a green try run:  https://tbpl.mozilla.org/?tree=Try&rev=7d2defea6904.  Broke the SpecialPowers bits into a separate patch.

Comment 35

[Jonathan Griffin (:jgriffin)]

Comment on attachment 756863 [details] [diff] [review]
Remove dependence on debugger server,

Carry r+ forward.

Comment 36

[Jonathan Griffin (:jgriffin)]

Attachment: Make SpecialPowers compatible with B2G compartment sharing,

Since the other patch in this bug changes how we load SpecialPowers in B2G, I had to make some changes to make it shared-compartment compatible, per bug 798491.

Comment 37

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adc90bff145f
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b68f9e4e04

Comment 38

[Jonathan Griffin (:jgriffin)]

Broken on osx, at least.  Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c70a04bdcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/026a5847ccd0

Comment 39

[Jonathan Griffin (:jgriffin)]

The error is:

Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: other is undefined
Call stack:
LDT_send/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/transport.js:227
@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:61

Comment 40

[Jonathan Griffin (:jgriffin)]

Whoops, that isn't the error, that was caused by Firefox's debugging tools.  The real culprit is this:

1370570782181	Marionette	ERROR	exception: ReferenceError, makeInfallible is not defined

Comment 41

[Jonathan Griffin (:jgriffin)]

Apparently the debugger has changed a bit since the last try run.  Solved by adding: 

loader.loadSubScript("resource://gre/modules/devtools/DevToolsUtils.js");

Comment 42

[Jonathan Griffin (:jgriffin)]

Attachment: Make DevToolsUtils.js compatible with B2G compartment sharing,

We now need to import DevToolsUtils.js, which needs some minor adjustments to work with B2G compartment sharing.

Comment 43

[Jonathan Griffin (:jgriffin)]

Attachment: Make SpecialPowers compatible with B2G compartment sharing,

Uploading current working patch

Comment 44

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server,

Latest working patch

Comment 45

[Jonathan Griffin (:jgriffin)]

Comment on attachment 759569 [details] [diff] [review]
Remove dependence on debugger server,

Carry r+ forward.

Comment 46

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

Comment on attachment 759566 [details] [diff] [review]
Make DevToolsUtils.js compatible with B2G compartment sharing,

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

I think you could just Cu.import the module instead:

Components.utils.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
var { makeInfallible } = DevToolsUtils;

or even:

this.makeInfallible = DevToolsUtils.makeInfallible;

Comment 47

[Jonathan Griffin (:jgriffin)]

Neither of those approaches work, Panos, they just yield a different error:

  ReferenceError, safeErrorString is not defined

Trying to fix that by, e.g., 

  this.safeErrorString = DevToolsUtils.safeErrorString;

doesn't work, since it isn't getting defined in the correct 'this'.

Comment 48

[Jonathan Griffin (:jgriffin)]

Pushed the current patches to try, to make sure they don't cause any failures in the devtools tests:  https://tbpl.mozilla.org/?tree=Try&rev=50e24e570269

Comment 49

[Jonathan Griffin (:jgriffin)]

Comment on attachment 759566 [details] [diff] [review]
Make DevToolsUtils.js compatible with B2G compartment sharing,

Also flagging Jim; we're trying to get this landed before a conference next week.

Comment 50

[Jonathan Griffin (:jgriffin)]

try job looks good:  https://tbpl.mozilla.org/?tree=Try&rev=59407140ec4e

Comment 51

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

Comment on attachment 759566 [details] [diff] [review]
Make DevToolsUtils.js compatible with B2G compartment sharing,

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

OK then!

Comment 52

[Jonathan Griffin (:jgriffin)]

This has bitrotted due to bug 870445 landing, so I'll fix it up and run it through try once more!

Comment 53

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0fa5b48dae3d

Comment 54

[Jonathan Griffin (:jgriffin)]

try was green.  pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/613d6ed5ccdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b692093c94
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d63831553f

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/613d6ed5ccdb
https://hg.mozilla.org/mozilla-central/rev/70b692093c94
https://hg.mozilla.org/mozilla-central/rev/48d63831553f

Comment 56

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server, mozilla-b2g18,

WIP: patch for mozilla-b2g18.

Comment 57

[Jonathan Griffin (:jgriffin)]

Attachment: Remove dependence on debugger server, mozilla-b2g18,

This version seems to work locally; I'll land it Monday so I can watch the tree.

Comment 58

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d80a28cf0364
https://hg.mozilla.org/releases/mozilla-b2g18/rev/263446d7506c

Will watch the tree to make sure this doesn't break.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/263446d7506c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/d80a28cf0364