1416711 - Convert client codebase to use DebuggerServer.registerActors (instead of addBrowserActors/addTabActors)

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Bug 1416105 highlighted a flaw in all the old codebases which is still not using the new API `DebuggerServer.registerActors`.
This new API should replace DebuggerServer.addBrowserActors and DebuggerServer.addTabActors.
And while replacing it, it is important to call it outside of all these checks we do everywhere:
  if (DebuggerServer.initialized) {
    DebuggerServer.init();
  }
It is important to ensure we always register the actors we expect, even if the server has already been initialized before.

Comment 1

[Julian Descottes [:jdescottes]]

Maybe we should have a helper in server/main.js that does the DebuggerServer init + DebuggerClient creation:

> if (!DebuggerServer.initialized) {
>   DebuggerServer.init();
>   DebuggerServer.addBrowserActors();
> }
> let client = new DebuggerClient(DebuggerServer.connectPipe());

to avoid duplicating this code all over the codebase. Most of the time, the only thing the consumer code wants is a DebuggerClient, and creating the Server is just a dependency. So it would make sense to hide this behind an API.

Should take a few options for things such as:
- which actors to register
- allowChromeProcess

Comment 2

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Sounds like a good idea as soon as you still understand you don't only spawn a client but also a server behind the scene.

Comment 3

[Alexandre Poirot [:ochameau] [PTO back August 21]]

I went through the usages of Debugger.addBrowserActors:
https://searchfox.org/mozilla-central/search?q=addBrowserActors&case=false&regexp=false&path=

And yes, in all cases where we do the "if (initialized) init()", we want to fetch a debugger client.
There is only one special case:
  https://searchfox.org/mozilla-central/source/devtools/client/webide/modules/runtimes.js#440-444
Otherwise, there is all the places where we only instanciate a server, and there, we don't check for `initialized`.

So we can refactor independantly:
* all places that fetch a debugger client should go through yet another helper,
* all places that only instanciate a server should switch to `registerActors`.


Any idea for this new helper?
  DebbugerServer.initAndConnect({ actors: {root, browser, tab}, allowXXX, ... })

At first I thought exposing it on DebuggerClient, but I think it is important to keep it on DebuggerServer as all the options we pass are related to the server.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> I went through the usages of Debugger.addBrowserActors:
> https://searchfox.org/mozilla-central/
> search?q=addBrowserActors&case=false&regexp=false&path=
> 
> And yes, in all cases where we do the "if (initialized) init()", we want to
> fetch a debugger client.
> There is only one special case:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/webide/modules/
> runtimes.js#440-444
> Otherwise, there is all the places where we only instanciate a server, and
> there, we don't check for `initialized`.
> 
> So we can refactor independantly:
> * all places that fetch a debugger client should go through yet another
> helper,
> * all places that only instanciate a server should switch to
> `registerActors`.
> 
> 
> Any idea for this new helper?
>   DebbugerServer.initAndConnect({ actors: {root, browser, tab}, allowXXX,
> ... })
> 
> At first I thought exposing it on DebuggerClient, but I think it is
> important to keep it on DebuggerServer as all the options we pass are
> related to the server.

Looks good to me! Keeping it on the DebuggerServer also makes it easier to understand that the server is involved in this.
To make sure, it would simply return the transport from connectPipe() right? So the consumer code would look like:

  let transport = DebuggerServer.initAndConnect({ actors: {root, browser, tab}, allowXXX, ... });
  let client = new DebuggerClient(transport);

> So we can refactor independantly:
> * all places that fetch a debugger client should go through yet another helper,
> * all places that only instanciate a server should switch to `registerActors`.

Maybe the initAndConnect helper does too much then? The most "complex" part of this code is the logic to initialize the server:
- checking the initialized flag
- knowing that you can safely call registerActors() even if actors have already been registered

With this in mind, we could also improve the init() code of the DebuggerServer. Would look like 

  DebuggerServer.init({ actors: {root, browser, tab}, allowXXX, ... });
  // if a client is needed ...
  let client = new DebuggerClient(DebuggerServer.connectPipe());

There's still the risk of trying to call connectPipe without having initialized the server... We can even do both: improve init() and add initAndConnect() ?

Comment 5

[Julian Descottes [:jdescottes]]

Not sure if that should be handled here or in a follow up but:

It seems that the second argument of addBrowserActors is never used (restrict privileges). As the registerActors/addBrowserActors APIs are quite confusing maybe we could get rid of it.

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #5)
> Not sure if that should be handled here or in a follow up but:
> 
> It seems that the second argument of addBrowserActors is never used
> (restrict privileges). As the registerActors/addBrowserActors APIs are quite
> confusing maybe we could get rid of it.

That attribute was only used on firefox os... That was used to ensure we could only reach tab actors and nothing else.
For example, we wanted to avoid access to the preference actor, which would have allowed to root the phone easily.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Remove unused restrictPrivileges argument in addBrowserActors;

restrictPrivileges was used for FirefoxOS and can be removed now.

Review commit: https://reviewboard.mozilla.org/r/199182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199182/

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Migrate addBrowser/TabActors to registerActors;

DebuggerServer has old APIs addBrowserActors & addTabActors that can be
replaced by calls to registerActors.

Review commit: https://reviewboard.mozilla.org/r/199184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199184/

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Remove default values for registerActors;

The default values for registerActors are never used in the codebase
and feel counter intuitive as they set all booleans to true.

Review commit: https://reviewboard.mozilla.org/r/199186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199186/

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Stop guarding consumer calls to DebuggerServer.init();

DebuggerServer.init() already bails out if it was previously initialized
so we could avoid guarding the calls to init() with it everywhere.

Registering an actor module several times is also a noop as the Server
keeps a map of all the already registered modules and will bail out if
the module is already known.

Review commit: https://reviewboard.mozilla.org/r/199188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199188/

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Remove windowType argument from registerActors;

The windowType option of registerActors has nothing to do with
registering actors. It is only used in one spot in the codebase, so
switch this one to DebuggerServer.chromeWindowType = "..."; and
remove the option from registerActors.

Review commit: https://reviewboard.mozilla.org/r/199190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199190/

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1416711 - Add registerAllActors API;

Most of the codebase that needs to create a debugger server
can use a server with all actors registered.

Define an additional method registerAllActors to do that.

By previous implementations, all the call sites that were
using browser: true were indirectly using tab & root: true
as well. So all the call sites using browser: true have been
migrated to registerAllActors and the specific behavior of
the browser: true case has been removed. Passing browser:true
to registerActors now only registers browser specific actors.

Review commit: https://reviewboard.mozilla.org/r/199192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199192/

Comment 13

[Julian Descottes [:jdescottes]]

Alex: that's a lot of small reviews, I tried to refactor step by step. Let me know which ones you find useful or not. I didn't add a new helper in the end. Simplifying the call sites, and removing the the (!initialized) guards felt sufficient.

Comment 14

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927978 [details]
Bug 1416711 - Remove unused restrictPrivileges argument in addBrowserActors;

https://reviewboard.mozilla.org/r/199182/#review204926

Looks

Comment 15

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927979 [details]
Bug 1416711 - Migrate addBrowser/TabActors to registerActors;

https://reviewboard.mozilla.org/r/199184/#review204914

good

::: devtools/server/main.js:284
(Diff revision 1)
>        this.registerModule("devtools/server/actors/webbrowser");
>      }
>  
> -    if (tab) {
> -      this.addTabActors();
> +    if (browser || tab) {
> +      this._addTabActors();
>      }

EDIT: looks like you fixed that in last changeset...

Now that you do this cleanup, I would ignore the old behavior/naming and make each flag load distinct things.
So root would only load the root,
browser would only load the global actors,
and tab would only load the tab actors.
Otherwise, there is no point in being so explicit in all call sites, you could just do registerActors({browser:true}) from most places.
(But I think being explicit everywhere is good)

if (browser) {
  this._addBrowserActors();
}

if (root) {
  this.registerModule("devtools/server/actors/webbrowser");
}

if (tab) {
  this._addTabActors();
}

Then... may be `browser` would be better called `global`. But to be honest global isn't the best either, it just happen to be the historical way to designate these actors.

Comment 16

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927980 [details]
Bug 1416711 - Remove default values for registerActors;

https://reviewboard.mozilla.org/r/199186/#review204930

thanks

Comment 17

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927981 [details]
Bug 1416711 - Stop guarding consumer calls to DebuggerServer.init();

https://reviewboard.mozilla.org/r/199188/#review204916

for

::: devtools/client/scratchpad/scratchpad.js:2191
(Diff revision 1)
>    _attach: function SW__attach()
>    {
> -    if (!DebuggerServer.initialized) {
> -      DebuggerServer.init();
> +    DebuggerServer.init();
> -      DebuggerServer.registerActors({ browser: true, root: true, tab: true });
> +    DebuggerServer.registerActors({ browser: true, root: true, tab: true });
> -    }
> +  DebuggerServer.registerActors({ browser: true, root: true, tab: true });

Something weird happened in this patch here?

::: devtools/client/webide/modules/runtimes.js:442
(Diff revision 1)
>    type: RuntimeTypes.LOCAL,
>    connect: function (connection) {
> -    if (!DebuggerServer.initialized) {
> -      DebuggerServer.init();
> +    DebuggerServer.init();
> -      DebuggerServer.registerActors({ browser: true, root: true, tab: true });
> +    DebuggerServer.registerActors({ browser: true, root: true, tab: true });
> -    }
> +  DebuggerServer.registerActors({ browser: true, root: true, tab: true });

Same here.

Comment 18

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927982 [details]
Bug 1416711 - Remove windowType argument from registerActors;

https://reviewboard.mozilla.org/r/199190/#review204932

this

Comment 19

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8927983 [details]
Bug 1416711 - Add registerAllActors API;

https://reviewboard.mozilla.org/r/199192/#review204920

cleanup!

::: devtools/client/framework/target.js:414
(Diff revision 1)
>        // When connecting to a local tab, we only need the root actor.
>        // Then we are going to call DebuggerServer.connectToChild and talk
>        // directly with actors living in the child process.
>        // We also need browser actors for actor registry which enabled addons
>        // to register custom actors.
> -      DebuggerServer.registerActors({ root: true, browser: true, tab: false });
> +      DebuggerServer.registerAllActors();

/!\ Not from here /!\

::: devtools/server/child.js:25
(Diff revision 1)
>  
>      DebuggerServer.init();
>      // We want a special server without any root actor and only tab actors.
>      // We are going to spawn a ContentActor instance in the next few lines,
>      // it is going to act like a root actor without being one.
> -    DebuggerServer.registerActors({ root: false, browser: false, tab: true });
> +    DebuggerServer.registerActors({ tab: true });

/!\ Nor from here /!\

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8927981 [details]
Bug 1416711 - Stop guarding consumer calls to DebuggerServer.init();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199188/diff/1-2/

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8927982 [details]
Bug 1416711 - Remove windowType argument from registerActors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199190/diff/1-2/

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8927983 [details]
Bug 1416711 - Add registerAllActors API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199192/diff/1-2/

Comment 23

[Julian Descottes [:jdescottes]]

Thanks for the reviews Alex! 

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Comment on attachment 8927983 [details]
> Bug 1416711 - Add registerAllActors API;
> 
> https://reviewboard.mozilla.org/r/199192/#review204920
> 
> cleanup!
> 
> ::: devtools/client/framework/target.js:414
> (Diff revision 1)
> >        // When connecting to a local tab, we only need the root actor.
> >        // Then we are going to call DebuggerServer.connectToChild and talk
> >        // directly with actors living in the child process.
> >        // We also need browser actors for actor registry which enabled addons
> >        // to register custom actors.
> > -      DebuggerServer.registerActors({ root: true, browser: true, tab: false });
> > +      DebuggerServer.registerAllActors();
> 
> /!\ Not from here /!\
> 
> ::: devtools/server/child.js:25
> (Diff revision 1)
> >  
> >      DebuggerServer.init();
> >      // We want a special server without any root actor and only tab actors.
> >      // We are going to spawn a ContentActor instance in the next few lines,
> >      // it is going to act like a root actor without being one.
> > -    DebuggerServer.registerActors({ root: false, browser: false, tab: true });
> > +    DebuggerServer.registerActors({ tab: true });
> 
> /!\ Nor from here /!\

Not sure what is wrong with these two? 
The first one contained "browser: true", so I considered it as equivalent to "all".
The second one only had tab: true, so passing only {tab: true} should be the same.

Comment 24

[Julian Descottes [:jdescottes]]

ni? for my question above

Comment 25

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #23)
> Thanks for the reviews Alex! 
> 
> (In reply to Alexandre Poirot [:ochameau] from comment #19)
> > Comment on attachment 8927983 [details]
> > Bug 1416711 - Add registerAllActors API;
> > 
> > https://reviewboard.mozilla.org/r/199192/#review204920
> > 
> > cleanup!
> > 
> > ::: devtools/client/framework/target.js:414
> > (Diff revision 1)
> > >        // When connecting to a local tab, we only need the root actor.
> > >        // Then we are going to call DebuggerServer.connectToChild and talk
> > >        // directly with actors living in the child process.
> > >        // We also need browser actors for actor registry which enabled addons
> > >        // to register custom actors.
> > > -      DebuggerServer.registerActors({ root: true, browser: true, tab: false });
> > > +      DebuggerServer.registerAllActors();
> > 
> > /!\ Not from here /!\
> > 
> > ::: devtools/server/child.js:25
> > (Diff revision 1)
> > >  
> > >      DebuggerServer.init();
> > >      // We want a special server without any root actor and only tab actors.
> > >      // We are going to spawn a ContentActor instance in the next few lines,
> > >      // it is going to act like a root actor without being one.
> > > -    DebuggerServer.registerActors({ root: false, browser: false, tab: true });
> > > +    DebuggerServer.registerActors({ tab: true });
> > 
> > /!\ Nor from here /!\
> 
> Not sure what is wrong with these two? 

So, discussing about that is disturbing as you change the behavior of registerActors in the second patch (browser:true imply all actors),
and revert back to original behavior in the last patch (each flag [root, browser, tab] is independant).
Let assume here we consider only the final behavior of it.

> The first one contained "browser: true", so I considered it as equivalent to
> "all".

We only want the browser actors here. See the comment before this line.
  DebuggerServer.registerActors({ root: true, browser: true });
I imagine it may have required special modification in second patch.

> The second one only had tab: true, so passing only {tab: true} should be the
> same.
  Yes, I must have been confused here. It looks good. It matches the existing comment.

Comment 26

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #23)
> > Thanks for the reviews Alex! 
> > 
> > (In reply to Alexandre Poirot [:ochameau] from comment #19)
> > > Comment on attachment 8927983 [details]
> > > Bug 1416711 - Add registerAllActors API;
> > > 
> > > https://reviewboard.mozilla.org/r/199192/#review204920
> > > 
> > > cleanup!
> > > 
> > > ::: devtools/client/framework/target.js:414
> > > (Diff revision 1)
> > > >        // When connecting to a local tab, we only need the root actor.
> > > >        // Then we are going to call DebuggerServer.connectToChild and talk
> > > >        // directly with actors living in the child process.
> > > >        // We also need browser actors for actor registry which enabled addons
> > > >        // to register custom actors.
> > > > -      DebuggerServer.registerActors({ root: true, browser: true, tab: false });
> > > > +      DebuggerServer.registerAllActors();
> > > 
> > > /!\ Not from here /!\
> > > 
> > > ::: devtools/server/child.js:25
> > > (Diff revision 1)
> > > >  
> > > >      DebuggerServer.init();
> > > >      // We want a special server without any root actor and only tab actors.
> > > >      // We are going to spawn a ContentActor instance in the next few lines,
> > > >      // it is going to act like a root actor without being one.
> > > > -    DebuggerServer.registerActors({ root: false, browser: false, tab: true });
> > > > +    DebuggerServer.registerActors({ tab: true });
> > > 
> > > /!\ Nor from here /!\
> > 
> > Not sure what is wrong with these two? 
> 
> So, discussing about that is disturbing as you change the behavior of
> registerActors in the second patch (browser:true imply all actors),
> and revert back to original behavior in the last patch (each flag [root,
> browser, tab] is independant).

I don't think I changed the behavior. Even though it is not explicit, today browser: true already implies all actors.
If browser: true, we call addBrowserActors. If we look at the code in details:
- https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0//devtools/server/main.js#429
- https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0//devtools/server/main.js#432

These two lines from addBrowserActors are the exact same code as what is executed when root: true and tab: true.
Based on this, today, browser: true already implies tab: true and root: true, it's just not explicit.

> Let assume here we consider only the final behavior of it.
> 
> > The first one contained "browser: true", so I considered it as equivalent to
> > "all".
> 
> We only want the browser actors here. See the comment before this line.
>   DebuggerServer.registerActors({ root: true, browser: true });
> I imagine it may have required special modification in second patch.
> 

I was mostly concerned about keeping the implementation identical. The comment meant {root: true, browser: true}, but the implementation was equivalent to {root: true, browser: true, tab: true}. Should I update the comment then? Or are we super confident we don't need tab actors in this case.

Comment 27

[Alexandre Poirot [:ochameau] [PTO back August 21]]

The implementation of registerActors from Bug 1323466 was misleading. Browser:true registering everything is misleading. 
This doesn't match was I meant to do in that bug. We should fix the behavior rather than updating the comment. 
But feel free to open a follow-up as it isn't strictly related to your work here.

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8927978 [details]
Bug 1416711 - Remove unused restrictPrivileges argument in addBrowserActors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199182/diff/1-2/

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8927979 [details]
Bug 1416711 - Migrate addBrowser/TabActors to registerActors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199184/diff/1-2/

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8927980 [details]
Bug 1416711 - Remove default values for registerActors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199186/diff/1-2/

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8927981 [details]
Bug 1416711 - Stop guarding consumer calls to DebuggerServer.init();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199188/diff/2-3/

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8927982 [details]
Bug 1416711 - Remove windowType argument from registerActors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199190/diff/2-3/

Comment 33

[Julian Descottes [:jdescottes]]

Comment on attachment 8927983 [details]
Bug 1416711 - Add registerAllActors API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199192/diff/2-3/

Comment 34

[Julian Descottes [:jdescottes]]

I logged Bug 1420134 as a follow up and mentioned it in the comment of makeRemote.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9b7d5086d37
Remove unused restrictPrivileges argument in addBrowserActors;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e2993f4377fa
Migrate addBrowser/TabActors to registerActors;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/71cc1ff4febe
Remove default values for registerActors;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6e58664da16e
Stop guarding consumer calls to DebuggerServer.init();r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6075d6f91ae7
Remove windowType argument from registerActors;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a2b9ac473749
Add registerAllActors API;r=ochameau

Comment 36

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/b9b7d5086d37
https://hg.mozilla.org/mozilla-central/rev/e2993f4377fa
https://hg.mozilla.org/mozilla-central/rev/71cc1ff4febe
https://hg.mozilla.org/mozilla-central/rev/6e58664da16e
https://hg.mozilla.org/mozilla-central/rev/6075d6f91ae7
https://hg.mozilla.org/mozilla-central/rev/a2b9ac473749