Remove dependency on remote debugger's dbg-server

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mdas, Assigned: jgriffin)

Tracking

Trunk
mozilla24
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(4 attachments, 13 obsolete attachments)

1.55 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.42 KB, patch
past
: review+
jimb
: review+
Details | Diff | Splinter Review
171.89 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
172.29 KB, patch
Details | Diff | Splinter Review
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.
Assignee

Comment 1

6 years ago
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).
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.
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
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. :)
Assignee: nobody → wlachance
Attachment #749370 - Flags: feedback?(jgriffin)
Assignee

Comment 5

6 years ago
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.
Attachment #749370 - Flags: feedback?(jgriffin) → feedback+
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...
Attachment #749370 - Attachment is obsolete: true
Attachment #749528 - Flags: review?(jgriffin)
Assignee

Comment 7

6 years ago
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?
Attachment #749528 - Flags: review?(jgriffin) → review-
Assignee

Comment 8

6 years ago
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.
Posted patch Updated patch (obsolete) — Splinter Review
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
Attachment #749528 - Attachment is obsolete: true
Attachment #752406 - Flags: review?(jgriffin)
Assignee

Comment 11

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

Comment 12

6 years ago
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.
Attachment #752406 - Flags: review?(jgriffin) → review+
(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!
(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
Assignee

Comment 15

6 years ago
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.
Assignee

Comment 16

6 years ago
(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.
Assignee

Comment 17

6 years ago
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.
Assignee

Updated

6 years ago
Assignee: wlachance → jgriffin
Assignee

Comment 18

6 years ago
I didn't mean to reassign this; bzexport did it for me.
Assignee: jgriffin → wlachance
Assignee

Comment 19

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

Updated

6 years ago
Attachment #753365 - Attachment is obsolete: true
Assignee

Comment 20

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

Comment 21

6 years ago
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.
Attachment #753978 - Flags: review?(wlachance)
Assignee

Updated

6 years ago
Attachment #753390 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Assignee: wlachance → jgriffin
Assignee

Updated

6 years ago
Attachment #752406 - Attachment is obsolete: true
Assignee

Comment 22

6 years ago
Posted file qdiff output (easy to review) (obsolete) —
Assignee

Comment 23

6 years ago
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.
Attachment #753981 - Flags: review?(past)
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. :)
Attachment #753978 - Flags: review?(wlachance) → review+
(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?
Assignee

Comment 27

6 years ago
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.
(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.
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.
Assignee

Comment 30

6 years ago
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.
Attachment #753981 - Attachment is obsolete: true
Attachment #753981 - Flags: review?(past)
Assignee

Comment 31

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

Updated

6 years ago
Attachment #753978 - Attachment is obsolete: true
Assignee

Comment 32

6 years ago
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
Assignee

Updated

6 years ago
Attachment #754990 - Attachment is obsolete: true
Assignee

Comment 33

6 years ago
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
Assignee

Comment 34

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

Updated

6 years ago
Attachment #755103 - Attachment is obsolete: true
Assignee

Comment 35

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

Carry r+ forward.
Attachment #756863 - Flags: review+
Assignee

Updated

6 years ago
Attachment #753979 - Attachment is obsolete: true
Assignee

Comment 36

6 years ago
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.
Attachment #756864 - Flags: review?(khuey)
Assignee

Comment 39

6 years ago
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
Assignee

Comment 40

6 years ago
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
Assignee

Comment 41

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

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

Comment 42

6 years ago
We now need to import DevToolsUtils.js, which needs some minor adjustments to work with B2G compartment sharing.
Attachment #759566 - Flags: review?(past)
Assignee

Comment 43

6 years ago
Uploading current working patch
Assignee

Updated

6 years ago
Attachment #759567 - Attachment is obsolete: true
Assignee

Comment 44

6 years ago
Latest working patch
Assignee

Updated

6 years ago
Attachment #756863 - Attachment is obsolete: true
Assignee

Comment 45

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

Carry r+ forward.
Attachment #759569 - Flags: review+
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;
Assignee

Comment 47

6 years ago
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'.
Assignee

Comment 48

6 years ago
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
Assignee

Comment 49

6 years ago
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.
Attachment #759566 - Flags: review?(jimb)

Updated

6 years ago
Attachment #759566 - Flags: review?(jimb) → review+
Comment on attachment 759566 [details] [diff] [review]
Make DevToolsUtils.js compatible with B2G compartment sharing,

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

OK then!
Attachment #759566 - Flags: review?(past) → review+
Assignee

Comment 52

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

Comment 56

6 years ago
WIP: patch for mozilla-b2g18.
Assignee

Comment 57

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

Updated

6 years ago
Attachment #766103 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.