Closed Bug 977443 Opened 10 years ago Closed 10 years ago

Implement an actor that defines new actors

Categories

(DevTools :: General, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: fitzgen, Assigned: Honza)

References

Details

Attachments

(1 file, 11 obsolete files)

30.08 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Attached patch wip (obsolete) — Splinter Review
Addons should be able to define new actors.
Assignee: nobody → nfitzgerald
Priority: -- → P3
Status: NEW → ASSIGNED
This will be an awesome feature! I can help testing the result API (from extension's perspective) as soon as there is working patch.

Honza
It would be good to get a sec-review of this approach.
Agreed, will flag Mark once I get something working. FWIW, addons can already do whatever they want, AFAIK.
(In reply to Nick Fitzgerald [:fitzgen] [Ðoge:D6jomNp59N9TVfgc538HU3RswwmwZwcrd7] from comment #3)
> Agreed, will flag Mark once I get something working. FWIW, addons can
> already do whatever they want, AFAIK.

Yep, for sure.  I am more wondering how we feel about it in the case of remote debugging, since it is basically a remote code installation API that an attacker could potentially abuse.  Of course, if you allow a debugging connection at all, you can already do many things like this anyway via the console actor and others...

Anyway, just good to have Mark double check our thinking here. :)
Attached patch wip (obsolete) — Splinter Review
New WIP

Dynamically adds actors, but fails to remove them when asked to do so.
Attachment #8382734 - Attachment is obsolete: true
Comment on attachment 8392396 [details] [diff] [review]
wip

Hey Mark!

Wanted to see if you think this is ok to ship / do a sec-review.

This patch allows addons (or anyone connected via the RDP) to eval chrome code on the server (aka the device). The point is so that addons wouldn't have to be installed on both the device and the desktop. Instead, they would be installed on the desktop only and then they could create new actors on the device.

See toolkit/devtools/server/actors/actor-registry.js for the interesting parts.

The patch isn't quite finished yet, but the potentially security sensitive parts won't be changing anymore.
Attachment #8392396 - Flags: sec-approval?
Attachment #8392396 - Flags: review?(mgoodwin)
Comment on attachment 8392396 [details] [diff] [review]
wip

sec-approval? is used to get approval to check security issues into trunk. Clearing flag. :-)
Attachment #8392396 - Flags: sec-approval?
Attached patch actor-actor.patch (obsolete) — Splinter Review
Alright, should be ready for a proper review (and still sec-review) now.

https://tbpl.mozilla.org/?tree=Try&rev=d28a37f3e1b1
Attachment #8392396 - Attachment is obsolete: true
Attachment #8392396 - Flags: review?(mgoodwin)
Attachment #8392542 - Flags: review?(mgoodwin)
Attachment #8392542 - Flags: review?(dcamp)
Comment on attachment 8392542 [details] [diff] [review]
actor-actor.patch

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

I'm happy with this provided my comment on actor-registry.js is addressed. Useful feature, by the way.

::: toolkit/devtools/server/actors/actor-registry.js
@@ +105,5 @@
> +    client.addActorPool(this);
> +    this.manage(this);
> +  },
> +
> +  registerActor: custom(function (uri) {

We should check the URI is sane (chrome?) before running stuff from it as the system principal (though, granted we have no way of verifying this on the server; just don't want to encourage addon authors to do dangerous things).
Attachment #8392542 - Flags: review?(mgoodwin) → review+
This is interesting for FxOS; Paul, please could you take a look?
Flags: needinfo?(ptheriault)
So my main concern would be if this patch allowed someone connected to a FxOS device to bypass the devtools.debugger.forbid-certified-apps restrictions (no parent process debugging, no debugging certified apps). [1] However as far as I can tell from this patch, you need chrome privileges in order to call the code which adds actors. If "devtools.debugger.forbid-certified-apps" is true, then I am not aware of anyway to run arbitrary chrome code, even with remote debugging (either app-manager or remote debug client). So I _think_ this code doesn't change things on Firefox OS. Would love for someone on the devtools team to confirm this assumption though.

I also am keen to understand the implications (if any) for compromised child processes (could they use this API to escalate privileges?) Again not something that I have looked at, but probably should asap.

[1]https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1127
Flags: needinfo?(ptheriault)
We could disable functionality if we are on fxos and devtools.debugger.forbid-certified-apps is true.
Yes, I think we'd have to keep this disabled when "devtools.debugger.forbid-certified-apps" is true (which I believe should now happen by default, if you don't list it explicitly in b2g's "safe" global actor list in the shell.js file Paul linked to).

It's unfortunate, since this could potentially have been an avenue to upgrade server support on older devices.  But, my understanding is that this would accept and run arbitrary chrome code, so it would have to be blocked by default on FxOS for now.  CCing Alex in case he has more thoughts on this.
(In reply to Paul Theriault [:pauljt] from comment #11)
> So my main concern would be if this patch allowed someone connected to a
> FxOS device to bypass the devtools.debugger.forbid-certified-apps
> restrictions (no parent process debugging, no debugging certified apps). [1]
> However as far as I can tell from this patch, you need chrome privileges in
> order to call the code which adds actors. If
> "devtools.debugger.forbid-certified-apps" is true, then I am not aware of
> anyway to run arbitrary chrome code, even with remote debugging (either
> app-manager or remote debug client). So I _think_ this code doesn't change
> things on Firefox OS. Would love for someone on the devtools team to confirm
> this assumption though.

The registerActor method is made to be called remotely from the client (firefox desktop),
so that it will work on FxOS. Then, correct me if I'm wrong but the fact that this actor is only global (addGlobalActor) will make it only appear for the parent process (global actors aren't created as tab actors, right?). If that the case, we won't have this actor-registry in child processes as we only create tab actors for them (there is no root actor in child processes).


> I also am keen to understand the implications (if any) for compromised child
> processes (could they use this API to escalate privileges?) Again not
> something that I have looked at, but probably should asap.

Hum, any actor, even living in the child process are living in system principal. Does compromised child means having chrome priviledges in the child?
If that's the case, I wouldn't be surprise if we can communicate with this actor in the parent process.
I can help you look into this, but I would prefer releasing the "how to hack out protocol" on non-public channels!

Having said that, I think this actor isn't really designed for FxOS in its current state. It will only be usefull if it is going to work in child processes, which would most likely needs some dedicated efforts.
So +1 on preventing registering the whole actor if the forbid pref is around.
Comment on attachment 8392542 [details] [diff] [review]
actor-actor.patch

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

::: toolkit/devtools/server/actors/root.js
@@ +405,5 @@
> +      }
> +      if (this._tabActorPool) {
> +        for (let tab of this._tabActorPool) {
> +          dump("FITZGEN: calling tab.removeActor\n");
> +          dump("FITZGEN:   prefix = " + tab.actorPrefix + "\n");

Leftover dumps.

::: toolkit/devtools/server/tests/unit/testactors.js
@@ +46,5 @@
>  };
>  
>  function createRootActor(aConnection)
>  {
> +  dump("FITZGEN: creating root actor with DS.globalActorFactories");

More debug dump.
Attachment #8392542 - Flags: review?(dcamp) → review+
Attached patch actor-actor.patch (obsolete) — Splinter Review
Rebased and updated based on comments.

For some reason one random xpcshell test is failing now and I don't really know what is going on.
Attachment #8392542 - Attachment is obsolete: true
No time to work on this now.
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Any estimate when this API could be ready?

I think this API are invaluable for any extension that wants to implement a new actor.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> Any estimate when this API could be ready?
> 
> I think this API are invaluable for any extension that wants to implement a
> new actor.
> 
> Honza

The jetpack team took this over, but it seems they aren't working on it in this bug. I don't know what's going on with it anymore.
Flags: needinfo?(rFobic)
(In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> (In reply to Jan Honza Odvarko [:Honza] from comment #18)
> > Any estimate when this API could be ready?
> > 
> > I think this API are invaluable for any extension that wants to implement a
> > new actor.
> > 
> > Honza
> 
> The jetpack team took this over, but it seems they aren't working on it in
> this bug. I don't know what's going on with it anymore.

Perhaps bug 980481 is what you are looking for.
(In reply to J. Ryan Stinnett [:jryans] from comment #20)
> Perhaps bug 980481 is what you are looking for.
Yeah, I know about this one, but the main difference is that
"director actor" is not supposed to have chrome privileges
(but I'll ask around yet).

Thanks
Honza
General approach behind director is to dinamically setup a same privileged sandbox as a debuggee and provide hooks for document related events. That being said we could add some option to create a chrome privileged sandbox instead. Please note that scope of work for director is a lot larger than this though & in initial cut we won't even have an API to define actors, instead "debug-script" will have a message port instance to exchange JSON messages with a toolbox/panel document. In a next cut we will add something like volcan for the server part that will allow defining actors pretty much the same way as it's done with protocol.js but again without dependencies on the chrome privileges.
Flags: needinfo?(rFobic)
Attached patch bug977443.patch (obsolete) — Splinter Review
@Alex: we have done good progress on fixing the original (Nick's) patch and I am attaching new updated version.

As we also discussed, this approach is nicely fitting into exiting actor world and could be later used as a base for higher level concepts (like e.g. Bug 980481 aka the Director you'll be reviewing). The other goal here is having API for extensions, so they can use the existing concept of actors and dynamically install them on remote devices.

Can you please also push to try?

Honza
Assignee: nobody → odvarko
Attachment #8400967 - Attachment is obsolete: true
Attachment #8499529 - Flags: review?(poirot.alex)
I flushed most of my ni?f?r? queues, but I can't imagine reviewing any more patches today.
I'll get back to this one tomorrow.

But the good news is that I reviewed luca's patches for Director and I think we can share some pieces regarding e10s. See bug 980481 comment 38 about DebuggerServer.parentMessageManager and connect-to-child event.

In the meantime, here is a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59ff53898a2d
Comment on attachment 8499529 [details] [diff] [review]
bug977443.patch

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

Thanks honza for reviving this patch!!

::: toolkit/devtools/server/actors/actor-registry.js
@@ +12,5 @@
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> +const Services = require("Services");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> +loader.lazyImporter(this, "DebuggerServer", "resource://gre/modules/devtools/dbg-server.jsm");
> +loader.lazyImporter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm");

Where does `loader` comes from?!

DebuggerServer should be loaded like this:
  const { DebuggerServer } = require("devtools/server/main");

There is no need to use `.js` in require path, you could just do:
  const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");

@@ +30,5 @@
> +};
> +
> +exports.unregister = function (handle) {
> +  handle.removeGlobalActor(ActorRegistryActor, "actorRegistryActor");
> +};

register/unregister is now deprecated due to lazy loading of actors.
We no longer need to define these method in actors, DebuggerServer handle the registration on its own.
See comment in main.js for forbid-certified-apps.

@@ +45,5 @@
> +    this.exports = exports;
> +  },
> +
> +  unregister: method(function () {
> +    this.exports.unregister(DebuggerServer.ModuleAPI());

This would have to be tuned to call DebuggerServer.removeXXXActor.

@@ +76,5 @@
> +    const sandbox = Cu.Sandbox(principal);
> +    const exports = sandbox.exports = {};
> +    sandbox.require = require;
> +    Cu.evalInSandbox(sourceText, sandbox, "1.8", filename, 1);
> +    exports.register(DebuggerServer.ModuleAPI());

Instead of using the now deprecated API, involving register/unregister methods within actor modules,
I think it would be better to use same pattern than lazy loading of actors.
registerActor would receive same additional argument than DebuggerServer.registerModule:
- is tab actor?
- is global actor?
- actor constructor name?
- actor prefix?
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#300

Then, instead of calling exports.register, you would call DebuggerServer.addTabActor/addGlobalActor from here.
if ("tab" in type)
  DebuggerServer.addTabActor(sandbox[constructorName], actorPrefix);
if ("global" in type)
  DebuggerServer.addGlobalActor(sandbox[constructorName], actorPrefix);

You may even do some lazy loading to prevent creating the sandbox until we receive a request for the related actor.
In order to do so you would pass a function as first argument to addXXXActor that would lazily create the sandbox and return the actor constructor.

@@ +126,5 @@
> +  initialize: function (client, actorID) {
> +    protocol.Front.prototype.initialize.call(this, client);
> +    this.actorID = actorID;
> +    client.addActorPool(this);
> +    this.manage(this);

I think you only need to call:
  protocol.Front.prototype.initialize.call(this, client, form);
and:
  this.manage(this);
(But instead of having `actorID` as second argument, you will take `form` and pass it to initialize)
`form` is the result of listTabs.

@@ +136,5 @@
> +        return this._registerActor(sourceText, uri);
> +      })
> +      .then(null, e => {
> +        DevToolsUtils.reportException("registerActor", e);
> +      });

Wouldn't it be better to just let the promise reject?
Wouldn't it allow to client to receive the exception?

::: toolkit/devtools/server/main.js
@@ +449,5 @@
> +    this.registerModule("devtools/server/actors/actor-registry", {
> +      prefix: "actorRegistry",
> +      constructor: "ActorRegistryActor",
> +      type: { global: true }
> +    });

We shouldn't even register the actor if the forbid-certifed-app isn't available.
Please move this within the if (!restrictPrivileges) condition, few lines before.

@@ +936,5 @@
>            (handler.id && handler.id == aActor.id)) {
>          delete DebuggerServer.tabActorFactories[name];
> +        for (let { rootActor } of values(this._connections)) {
> +          rootActor.removeActor(name);
> +        }

It feels hacky.
What about calling:
  for (let conn of this._connections) {
    conn.removeActor(name);
  }
and then tune DebuggerServerConnection.removeActor
to not only call removeActor of _actorPool but also
on each pool of _extraPools?
(Then we wouldn't need to add a removeActor method on RootActor nor BrowserTabActor)

::: toolkit/devtools/server/tests/unit/hello-actor.js
@@ +8,5 @@
> +};
> +
> +exports.unregister = function (handle) {
> +  handle.removeGlobalActor(HelloActor, "helloActor");
> +};

To follow previous comment about lazy loading of actor, register/unregister is now deprecated.
To keep the actor story simple, please avoid using this pattern for dynamically registered actors.

::: toolkit/devtools/server/tests/unit/test_actor-registry-actor.js
@@ +27,5 @@
> +}
> +
> +function getRegistry() {
> +  gClient.listTabs(({ actorRegistryActor }) => {
> +    gRegistryFront = ActorRegistryFront(gClient, actorRegistryActor);

Front used to pass the "form" as second argument to front,
here, it would be the first argument of listTabs callback.
Attachment #8499529 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Comment on attachment 8499529 [details] [diff] [review]
> bug977443.patch
> 
> Review of attachment 8499529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks honza for reviving this patch!!
> 
> ::: toolkit/devtools/server/actors/actor-registry.js
> @@ +12,5 @@
> > +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> > +const Services = require("Services");
> > +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
> > +loader.lazyImporter(this, "DebuggerServer", "resource://gre/modules/devtools/dbg-server.jsm");
> > +loader.lazyImporter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm");
> 
> Where does `loader` comes from?!

Loader.jsm adds it as a global to every common js module.

> 
> DebuggerServer should be loaded like this:
>   const { DebuggerServer } = require("devtools/server/main");

This would undo the lazy loading of the module, but I suppose in this specific case the server has already started if we are loading actors.
Attached patch bug977443-2.patch (obsolete) — Splinter Review
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Comment on attachment 8499529 [details] [diff] [review]
> bug977443.patch
> 
> Review of attachment 8499529 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks honza for reviving this patch!!
> 
> DebuggerServer should be loaded like this:
>   const { DebuggerServer } = require("devtools/server/main");
Done

> There is no need to use `.js` in require path, you could just do:
>   const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js");
Done

> register/unregister is now deprecated due to lazy loading of actors.
> We no longer need to define these method in actors, DebuggerServer handle
> the registration on its own.
> See comment in main.js for forbid-certified-apps.
Done

> > +  unregister: method(function () {
> > +    this.exports.unregister(DebuggerServer.ModuleAPI());
> 
> This would have to be tuned to call DebuggerServer.removeXXXActor.
Done

> > +    const sandbox = Cu.Sandbox(principal);
> > +    const exports = sandbox.exports = {};
> > +    sandbox.require = require;
> > +    Cu.evalInSandbox(sourceText, sandbox, "1.8", filename, 1);
> > +    exports.register(DebuggerServer.ModuleAPI());
> 
> Instead of using the now deprecated API, involving register/unregister
> methods within actor modules,
> I think it would be better to use same pattern than lazy loading of actors.
> registerActor would receive same additional argument than
> DebuggerServer.registerModule:
> - is tab actor?
> - is global actor?
> - actor constructor name?
> - actor prefix?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.
> js#300
Done

> You may even do some lazy loading to prevent creating the sandbox until we
> receive a request for the related actor.
> In order to do so you would pass a function as first argument to addXXXActor
> that would lazily create the sandbox and return the actor constructor.
I don't understand the suggestion. The function passed into addXXXActor
is supposed to be a constructor and is used together with 'new' operand to create an instance of the actor. if I pass a callback that returns an instance of the actor the code in: ObservedActorFactory.prototype.createActor wouldn't work. 

// c is supposed to be actor ctor not a callback
let instance = new c(this._conn, this._parentActor);

Or am I missing something?

> @@ +126,5 @@
> > +  initialize: function (client, actorID) {
> > +    protocol.Front.prototype.initialize.call(this, client);
> > +    this.actorID = actorID;
> > +    client.addActorPool(this);
> > +    this.manage(this);
> 
> I think you only need to call:
>   protocol.Front.prototype.initialize.call(this, client, form);
> and:
>   this.manage(this);
> (But instead of having `actorID` as second argument, you will take `form`
> and pass it to initialize)
> `form` is the result of listTabs.
Done

> @@ +136,5 @@
> > +        return this._registerActor(sourceText, uri);
> > +      })
> > +      .then(null, e => {
> > +        DevToolsUtils.reportException("registerActor", e);
> > +      });
> 
> Wouldn't it be better to just let the promise reject?
> Wouldn't it allow to client to receive the exception?
Done

> ::: toolkit/devtools/server/main.js
> @@ +449,5 @@
> > +    this.registerModule("devtools/server/actors/actor-registry", {
> > +      prefix: "actorRegistry",
> > +      constructor: "ActorRegistryActor",
> > +      type: { global: true }
> > +    });
> 
> We shouldn't even register the actor if the forbid-certifed-app isn't
> available.
> Please move this within the if (!restrictPrivileges) condition, few lines
> before.
Done 

> @@ +936,5 @@
> >            (handler.id && handler.id == aActor.id)) {
> >          delete DebuggerServer.tabActorFactories[name];
> > +        for (let { rootActor } of values(this._connections)) {
> > +          rootActor.removeActor(name);
> > +        }
> 
> It feels hacky.
> What about calling:
>   for (let conn of this._connections) {
>     conn.removeActor(name);
>   }
> and then tune DebuggerServerConnection.removeActor
> to not only call removeActor of _actorPool but also
> on each pool of _extraPools?
> (Then we wouldn't need to add a removeActor method on RootActor nor
> BrowserTabActor)
I don't understand what you mean. The current way the actor is removes expects actor's name (prefix) to look for the right actor(s) to remove it. ActorPool.removeActor expects actor ID, which is not known in this case.

> ::: toolkit/devtools/server/tests/unit/hello-actor.js
> @@ +8,5 @@
> > +};
> > +
> > +exports.unregister = function (handle) {
> > +  handle.removeGlobalActor(HelloActor, "helloActor");
> > +};
> 
> To follow previous comment about lazy loading of actor, register/unregister
> is now deprecated.
> To keep the actor story simple, please avoid using this pattern for
> dynamically registered actors.
Done 

> ::: toolkit/devtools/server/tests/unit/test_actor-registry-actor.js
> @@ +27,5 @@
> > +}
> > +
> > +function getRegistry() {
> > +  gClient.listTabs(({ actorRegistryActor }) => {
> > +    gRegistryFront = ActorRegistryFront(gClient, actorRegistryActor);
> 
> Front used to pass the "form" as second argument to front,
> here, it would be the first argument of listTabs callback.
Done

Thanks for the review Alex!

Honza
Attachment #8499529 - Attachment is obsolete: true
Attachment #8502659 - Flags: review?(poirot.alex)
(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> Created attachment 8502659 [details] [diff] [review]
>
> > You may even do some lazy loading to prevent creating the sandbox until we
> > receive a request for the related actor.
> > In order to do so you would pass a function as first argument to addXXXActor
> > that would lazily create the sandbox and return the actor constructor.
> I don't understand the suggestion. The function passed into addXXXActor
> is supposed to be a constructor and is used together with 'new' operand to
> create an instance of the actor. if I pass a callback that returns an
> instance of the actor the code in:
> ObservedActorFactory.prototype.createActor wouldn't work. 
> 
> // c is supposed to be actor ctor not a callback
> let instance = new c(this._conn, this._parentActor);
> 
> Or am I missing something?

I'm not sure it is that much important to delay the sandbox creation
as it looks like we will always end up loading it if we already are in this codepath.
But you should be able to delay it with something like this:

let { prefix, constructor, type } = options;
let ctor = (conn, parentActor) => 
  const principal = CC("@mozilla.org/systemprincipal;1", "nsIPrincipal")();
  const sandbox = Cu.Sandbox(principal);
  const exports = sandbox.exports = {};
  sandbox.require = require;
  Cu.evalInSandbox(sourceText, sandbox, "1.8", fileName, 1);
  return new sandbox[constructor](conn, parentActor);
}

I think the double usage of new() is fine here, but I may be wrong...

If you want, you can also tweak RegisteredActorFactory in order to support 3 cases:
- lazy loaded actors using modules: `if (typeof(options) != "function") {`,
- old deprecated synchronously loaded actors using a constructor function: `} else {`,
- and a new kind, your case, where you have an actor source string.
You would have to come up with a new set of if/elseif/else conditions in RegisteredActorFactory(),
but then you will be able to define a custom _getConstructor function that will create the sandbox.


> > @@ +936,5 @@
> > >            (handler.id && handler.id == aActor.id)) {
> > >          delete DebuggerServer.tabActorFactories[name];
> > > +        for (let { rootActor } of values(this._connections)) {
> > > +          rootActor.removeActor(name);
> > > +        }
> > 
> > It feels hacky.
> > What about calling:
> >   for (let conn of this._connections) {
> >     conn.removeActor(name);
> >   }
> > and then tune DebuggerServerConnection.removeActor
> > to not only call removeActor of _actorPool but also
> > on each pool of _extraPools?
> > (Then we wouldn't need to add a removeActor method on RootActor nor
> > BrowserTabActor)
> I don't understand what you mean. The current way the actor is removes
> expects actor's name (prefix) to look for the right actor(s) to remove it.
> ActorPool.removeActor expects actor ID, which is not known in this case.

Oh right I totally missed that point when suggesting this alternative...
Still it is very misleading to have these methods on the root actors, especially called removeActor, like all the existing ones that have a very different behavior.
I think it would be better to name it `removeExtraActor` and put its implementation in actors/common.js,
next to the other methods playing with _extraActors array.
Then If I'm not wrong again, I would still suggest to call DebuggerServerConnection.removeActor from this removeExtraActor method. It looks like it expect an actor as argument.
removeExtraActor = (name) {
  let actor = extraActors[name];
  if (actor) {
    this.conn.removeActor(actor);
    delete extraActors[name];
  }
}
Finally as said before, you would have to tweak DebuggerServerConnection.removeActor to not only remove actors from _actorPool but also from _extraPools.

> Thanks for the review Alex!

Thanks for the patch!!
Comment on attachment 8502659 [details] [diff] [review]
bug977443-2.patch

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

r+ with the changes made to removeActor described in previous comment.

::: toolkit/devtools/server/actors/actor-registry.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { on, once, off, emit } = require("sdk/event/core");

It doesn't seem to be used?

@@ +10,5 @@
> +
> +const { Cu, CC, components } = require("chrome");
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> +const Services = require("Services");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");

Doesn't seem to be used either.

@@ +12,5 @@
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> +const Services = require("Services");
> +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +const { DebuggerServer } = require("devtools/server/main");
> +loader.lazyImporter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm");

I still feel bad with using this magic `loader` global.
Consider this as a nit:
  const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
  XPCOMUtils.defineLazyModuleGetter(this, "NetUtils", "resource://gre/modules/NetUtil.jsm");

@@ +139,5 @@
> +
> +  registerActor: custom(function (uri, options) {
> +    return request(uri, options)
> +      .then(sourceText => {
> +        return this._registerActor(sourceText, uri, options);

Note that for performance reason, you may want to use bulk data to send the sourceText to the server.
Especially if you want to have actor of a significant size!
The following test may help:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/transport/tests/unit/test_client_server_bulk.js

That can be done as a followup if we end up seeing performances issues,
but note that doing such thing later may complexify the support of this feature
as it we would have to support both ways if we end up switching to bulk later.

::: toolkit/devtools/server/actors/common.js
@@ +61,5 @@
> +
> +    // Using the type name is preferred since actors defined using ActorClass
> +    // use the same default constructor labeled as 'constructor'.
> +    if (options.prototype && options.prototype.typeName)
> +      this.name = options.prototype.typeName;

Are you sure it won't break removeTab/GlobalActor if typeName is different than constructor name?

@@ +286,5 @@
> +  "@@iterator": function* () {
> +    for (let name in this._actors) {
> +      yield this._actors[name];
> +    }
> +   }

nit: Is this really needed/used somewhere?

::: toolkit/devtools/server/tests/unit/test_actor-registry-actor.js
@@ +10,5 @@
> +var gRegistryFront;
> +var gActorFront;
> +var gOldPref;
> +
> +Components.utils.import("resource:///modules/devtools/SourceMap.jsm");

Is it necessary?
Attachment #8502659 - Flags: review?(poirot.alex) → review+
Attached patch bug977443-3.patch (obsolete) — Splinter Review
(In reply to Alexandre Poirot [:ochameau] from comment #29)

> I'm not sure it is that much important to delay the sandbox
> creation as it looks like we will always end up loading it if
> we already are in this codepath.
Agree, so let's keep the code simple.

> Still it is very misleading to have these methods on the root actors,
> especially called removeActor, like all the existing ones that have
> a very different behavior.
After our email conversation I am keeping this as it is.

The only thing I changed in the end is the name of the method:
removeActor -> removeActorByName. To make it a bit clearer.


> > +const { on, once, off, emit } = require("sdk/event/core");
> 
> It doesn't seem to be used?
Fixed

> > +const { Cu, CC, components } = require("chrome");
> > +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
> > +const Services = require("Services");
> > +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> 
> Doesn't seem to be used either.
Fixed

> > +const { DebuggerServer } = require("devtools/server/main");
> > +loader.lazyImporter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm");
> 
> I still feel bad with using this magic `loader` global.
Fixed

> > +  registerActor: custom(function (uri, options) {
> > +    return request(uri, options)
> > +      .then(sourceText => {
> > +        return this._registerActor(sourceText, uri, options);
> 
> Note that for performance reason, you may want to use bulk data to send the
> sourceText to the server.
> That can be done as a followup if we end up seeing performances issues,
Let's do this as a follow up. I think this could be also solved generally by
JSON packet (auto) compression done in the future (talking quickly with Panos on IRC about this).

> Are you sure it won't break removeTab/GlobalActor if typeName is different
> than constructor name?
Good call. I changed that and used "prefix" passed into RegisteredActorFactory(). The same argument is used (as 'name') for:

DebuggerServer.tabActorFactories[name] = new RegisteredActorFactory(aActor, name);

So, iteration over tabActorFactories in removeXXXActor uses the same 'name'.

> > +  "@@iterator": function* () {
> > +    for (let name in this._actors) {
> > +      yield this._actors[name];
> > +    }
> > +   }
> 
> nit: Is this really needed/used somewhere?
Yes, it's for iteration of _tabActorPoolin RootActor.removeActorByName()


> ::: toolkit/devtools/server/tests/unit/test_actor-registry-actor.js
> @@ +10,5 @@
> > +var gRegistryFront;
> > +var gActorFront;
> > +var gOldPref;
> > +
> > +Components.utils.import("resource:///modules/devtools/SourceMap.jsm");
> 
> Is it necessary?
Fixed

Thanks!
Honza
Attachment #8502659 - Attachment is obsolete: true
Attachment #8505510 - Flags: review?(poirot.alex)
(In reply to Jan Honza Odvarko [:Honza] from comment #30)
> Created attachment 8505510 [details] [diff] [review]
>
> > > +  registerActor: custom(function (uri, options) {
> > > +    return request(uri, options)
> > > +      .then(sourceText => {
> > > +        return this._registerActor(sourceText, uri, options);
> > 
> > Note that for performance reason, you may want to use bulk data to send the
> > sourceText to the server.
> > That can be done as a followup if we end up seeing performances issues,
> Let's do this as a follow up. I think this could be also solved generally by
> JSON packet (auto) compression done in the future (talking quickly with
> Panos on IRC about this).

The simple fact of converting a file data to a base64 string takes significant ressources :/

 
> > Are you sure it won't break removeTab/GlobalActor if typeName is different
> > than constructor name?
> Good call. I changed that and used "prefix" passed into
> RegisteredActorFactory(). The same argument is used (as 'name') for:
> 
> DebuggerServer.tabActorFactories[name] = new RegisteredActorFactory(aActor,
> name);
> 
> So, iteration over tabActorFactories in removeXXXActor uses the same 'name'.

I still it is still broken.
I don't think we have any test covering removal of protocol.js actor with the deprecated api.
The issue will be that here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#929
aActor.name will be "constructor", whereas handler.name will be prefix value.
Comment on attachment 8505510 [details] [diff] [review]
bug977443-3.patch

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

Looks almost good to me, but there is a failing xpcshell test:
TEST-UNEXPECTED-FAIL | /mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/unit/head_dbg.js | false == true - See following stack:
/mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/unit/head_dbg.js:do_check_eq:390
/mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_register_actor.js:check_actors:9
/mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/unit/test_register_actor.js:test_deprecated_api:44
/mnt/desktop/gecko/testing/xpcshell/head.js:_run_next_test:1315
/mnt/desktop/gecko/testing/xpcshell/head.js:do_execute_soon/<.run:570
/mnt/desktop/gecko/testing/xpcshell/head.js:_do_main:191
/mnt/desktop/gecko/testing/xpcshell/head.js:_execute_test:405

Could you fix that before I push your patch to try?

::: toolkit/devtools/server/actors/common.js
@@ +62,5 @@
> +    // Using the prefix is preferred since actors defined using ActorClass
> +    // use the same default constructor labeled as 'constructor'.
> +    if (prefix) {
> +      this.name = prefix;
> +    }

See my previous comment, I think it still breaks DebuggerServer.removeXXXActor

::: toolkit/devtools/server/actors/root.js
@@ +429,5 @@
> +      if (this._globalActorPool.has(actor)) {
> +        this._globalActorPool.removeActor(actor);
> +      }
> +      if (this._tabActorPool) {
> +        for (let tab of this._tabActorPool) {

A comment may be useful. Something like "Iterate over TabActor instances to also remove tab actors created during listTabs for each document"
(tab isn't a pool, nor just a random actor living in a pool, but a TabActor)

::: toolkit/devtools/server/tests/unit/testactors.js
@@ +128,5 @@
> +    if (this._tabActorPool) {
> +      this._tabActorPool.removeActor(actor);
> +    }
> +    delete this._extraActors[aName];
> +  },

Shouldn't this be named removeExtraActor now?
Attachment #8505510 - Flags: review?(poirot.alex)
Attached patch bug977443-4.patch (obsolete) — Splinter Review
(In reply to Alexandre Poirot [:ochameau] from comment #31)
> The simple fact of converting a file data to a base64 string takes
> significant ressources :/
True, ok, I am working on it. I think I'll attach this as separate patch to this bug, so the review is easier.

> I still it is still broken.
> I don't think we have any test covering removal of protocol.js actor with
> the deprecated api.
> The issue will be that here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.
> js#929
> aActor.name will be "constructor", whereas handler.name will be prefix value.
I see, should be fixed now.


(In reply to Alexandre Poirot [:ochameau] from comment #32)
> Comment on attachment 8505510 [details] [diff] [review]
> Looks almost good to me, but there is a failing xpcshell test:
> TEST-UNEXPECTED-FAIL |
> /mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/
> unit/head_dbg.js | false == true - See following stack:
> /mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/
> unit/head_dbg.js:do_check_eq:390
> /mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/
> unit/test_register_actor.js:check_actors:9
> /mnt/desktop/gecko/obj-firefox/_tests/xpcshell/toolkit/devtools/server/tests/
> unit/test_register_actor.js:test_deprecated_api:44
> /mnt/desktop/gecko/testing/xpcshell/head.js:_run_next_test:1315
> /mnt/desktop/gecko/testing/xpcshell/head.js:do_execute_soon/<.run:570
> /mnt/desktop/gecko/testing/xpcshell/head.js:_do_main:191
> /mnt/desktop/gecko/testing/xpcshell/head.js:_execute_test:405
> 
> Could you fix that before I push your patch to try?
Fixed (one bug in the test also fixed).


> A comment may be useful. Something like "Iterate over TabActor instances to
> also remove tab actors created during listTabs for each document"
> (tab isn't a pool, nor just a random actor living in a pool, but a TabActor)
Done

> Shouldn't this be named removeExtraActor now?
Fixed (it's removeActorByName)

Honza
Attachment #8505510 - Attachment is obsolete: true
Attachment #8507659 - Flags: review?(poirot.alex)
Comment on attachment 8507659 [details] [diff] [review]
bug977443-4.patch

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

If you think you can land this patch without addressing the protocoljs actor removing.
I'm ok to address that in a followup...

try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b808edd03049

::: toolkit/devtools/server/main.js
@@ +135,4 @@
>      },
>      // See DebuggerServer.removeTabActor for a description.
>      removeTabActor: function(factory) {
> +      DebuggerServer.removeTabActor(activeTabActors.get(factory));

It is hard to follow why we are doing such thing in ModuleAPI.
removeTabActor is used to receive an actor constructor or the lazy actor object. Here we are faking an actor constructor by just passing `{name}` object.
To be honest I would prefer fixing the issue with constructor name within Heritage, or somehow identify Heritage actors differently.
Instead of introducing workaround in unrelated code.
I think in a previous patch you identified protocoljs actors by typeName or something, we can do that. We will just have to also identify actors by typeName in DebuggerServer.remove{Tab,Global}Actor.

I'm really sorry to keep being unhappy about this, but I wish we could keep stuff simple and not introduce complex and fragile workarounds for an edge case.
Attachment #8507659 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #34)
> Comment on attachment 8507659 [details] [diff] [review]
> bug977443-4.patch
> 
> Review of attachment 8507659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you think you can land this patch without addressing the protocoljs actor
> removing.
> I'm ok to address that in a followup...
I have created the followup (bug 1086613)
Let's see, perhaps there is a simple solution to the ctor problem.

> I'm really sorry to keep being unhappy about this, but I wish we could keep
> stuff simple and not introduce complex and fragile workarounds for an edge
> case.
Not a problem, you are great reviewer Alex!

Honza
How about this proposed fix to the removeXXXActor issue (decribed in the followup issue 1086613):

- devtools/server/main.js: reverted changes to the ModuleAPI

- devtools/server/actors/common.js: moved the management of actors registered from ActorRegistryActor out of the obsolete "else" branch

- devtools/server/actors/actor-registry.js: use new "factory format" (similar to the lazy loaded actors, instead of the obsolete one):
   {
      constructorName: "...",
      constructorFun: function () { ... }
   }
Attachment #8513219 - Flags: feedback?(poirot.alex)
Attachment #8513219 - Flags: feedback?(odvarko)
Comment on attachment 8513219 [details] [diff] [review]
bug977443-4_tweak_RegisteredActorFactory.patch

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

I think I would prefer identifying protocoljs actors by typeName rather than introducing yet another option type in RegisteredActorFactory.
Attachment #8513219 - Flags: feedback?(poirot.alex)
Honza, Please do not hold your patch too much on that. I don't want to see your patch rot just because of the removal/constructor-name issue!
(In reply to Alexandre Poirot [:ochameau] from comment #37)
> Comment on attachment 8513219 [details] [diff] [review]
> bug977443-4_tweak_RegisteredActorFactory.patch
> I think I would prefer identifying protocoljs actors by typeName rather than
> introducing yet another option type in RegisteredActorFactory.
I don't actually see the new option as bad thing (it's just an argument). It should be clear what is it for and can be later safely refactored together with the entire "constructor story". Otherwise, I don't know what are the other options. From the feedback I have, I don't feel that a patch for heritage.js is something easy to get done.

> Honza, Please do not hold your patch too much on that. I don't want to see
> your patch rot just because of the removal/constructor-name issue!
Yeah, I'd be so happy if we can finally lend this!

Btw. sending the source as bulk data is also not doable at this moment, see bug1092196 (the protocol is just not ready)

Honza
Comment on attachment 8513219 [details] [diff] [review]
bug977443-4_tweak_RegisteredActorFactory.patch

See my comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=977443#c39

Honza
Attachment #8513219 - Flags: feedback?(odvarko) → feedback+
Attached patch bug977443-5.patch (obsolete) — Splinter Review
Attaching new patch that includes changes suggested by Luca.

(the bulk transport needs to be done as a followup since the protocol.js has not yet been updated to work with bulk data, see bug 949595)

Honza
Attachment #8507659 - Attachment is obsolete: true
Attachment #8513219 - Attachment is obsolete: true
Attachment #8518840 - Flags: review?(poirot.alex)
Attached patch bug977443-6.patch (obsolete) — Splinter Review
Yet rebased on HEAD + iterator made clean.

Honza
Attachment #8518840 - Attachment is obsolete: true
Attachment #8518840 - Flags: review?(poirot.alex)
Attachment #8518879 - Flags: review?(poirot.alex)
Comment on attachment 8518879 [details] [diff] [review]
bug977443-6.patch

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

Tested on desktop and b2g devices.
Looks good, thanks Honza for this patch!

Here is a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d04283ac54f

::: toolkit/devtools/server/main.js
@@ +935,5 @@
>            (handler.id && handler.id == aActor.id)) {
>          delete DebuggerServer.tabActorFactories[name];
> +        for (let connID of Object.getOwnPropertyNames(this._connections)) {
> +          this._connections[connID].rootActor.removeActorByName(name);
> +        }

A comment could be useful. We are not only removing factories, but also instances now.

@@ +992,5 @@
>            (handler.id && handler.id == aActor.id)) {
>          delete DebuggerServer.globalActorFactories[name];
> +        for (let connID of Object.getOwnPropertyNames(this._connections)) {
> +          this._connections[connID].rootActor.removeActorByName(name);
> +        }

Same thing here.
Attachment #8518879 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #43)
> Comment on attachment 8518879 [details] [diff] [review]
> A comment could be useful. We are not only removing factories, but also
> instances now.
Done

Thanks for the review Alex!

Honza
Attachment #8518879 - Attachment is obsolete: true
Attachment #8522074 - Flags: review?(poirot.alex)
Comment on attachment 8522074 [details] [diff] [review]
bug977443-7.patch

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

There is no need to re-request the review for nits.
Here is a try for yesterday's patch:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d04283ac54f
There is some failures in mochitest-devtools, I just launched a new run to see if that's intermittents.
Attachment #8522074 - Flags: review?(poirot.alex) → review+
Honza, it looks like mochitest-devtools are broken.
Can you take a look?
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d04283ac54f

(I might have pushed the try run on a broken m-c changeset. Tell me if tests are all green locally, I'll push another try.)
I just ran the tests and all green for me.

mach mochitest-chrome browser/devtools/
mach mochitest-chrome toolkit/devtools/

Tested on both Nightly and DevEdition

Honza
This is failing on mochitest-devtools.
So you have to run: mach mochitest-devtools.
I see, so I ran tests again.

mach mochitest-devtools browser/devtools/debugger/ -> 9755 passed (no failed tests)
mach mochitest-devtools browser/devtools/styleeditor/ -> 332 passed (no failed tests)
mach mochitest-devtools browser/devtools/framework/ -> 932 passed (1 failed)


The one failing tests I am experiencing is the following one, but it fails
even if the patch isn't applied.

1572 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_devtools_api.js | A promise chain failed to handlea rejection:  - at resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1096 - TypeError:this.doc is undefined


Honza
Btw. tested on Win7

Honza
Ok, let's see if a new try, based on today's master is green:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bffdcc113362
Looks green, honza, can you rebase one last time? I had to resolve some conflict before pushing to try.
> Looks green, honza, can you rebase one last time?
> I had to resolve some conflict before pushing to try.
(based on our IRC conversation)

bug977443-7.patch applies cleanly, so just checkin-needed set.

Honza
https://hg.mozilla.org/mozilla-central/rev/e0ac11eb4bf4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.