Open Bug 1119790 Opened 9 years ago Updated 2 years ago

A global actor is instantiated for every toolbox

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Platform:
x86_64
Windows 7
Type:
defect

Tracking

(Not tracked)

People

(Reporter: Honza, Unassigned)

Details

(Whiteboard: [firebug]) 

STR:
1. Install extension: https://github.com/firebug/devtools-extension-examples/tree/master/GlobalActor
2. Open a browser tab and toolbox in it. Watch the browser console, you should see: "myActor.initialize" (log from within the actor's ctor)
3. Open another browser tab and toolbox in it. Watch the browser console, you should see another log "myActor.initialize" indicating that new instance of the 'global' actor has been created, but it should not -> BUG

---

DebuggerServerConnection._getOrCreateActor() creates a new actor instead of using an existing one (the actor variable is set to ObservedActorFactory)

Honza
Whiteboard: [firebug]
Btw. I can see that non-pool instance is passed into DebuggerServerConnection.addActorPool, which doesn't feel right.

It's caused by __poolMap from Pool object (protocol.js)

  /**
   * Pool is the base class for all actors, even leaf nodes.
   * If the child map is actually referenced, go ahead and create
   * the stuff needed by the pool.
   */
  __poolMap: null,
  get _poolMap() {
    if (this.__poolMap) return this.__poolMap;
    this.__poolMap = new Map();
    this.conn.addActorPool(this);
    return this.__poolMap;
  },

Could this be related or should I create another report?

Honza
One more comment. The logic I am seeing is as follows:

1. A new instance of DebuggerServerConnection is created for every tab where the Toolbox is opened.
2. Every connection instance has its own set of actor pools.
3. Connections share actor factories
4. Every connection maintains its own instances of registered global actors. So, global actor is instantiated for every tab (where toolbox is active).

I thought that there should be one instance of Global actor (in the parent process) shared among all tabs, no?

Honza
Flags: needinfo?(poirot.alex)
Hum I'm not sure we expect more than one connection.
Having different set of actors per connection sounds fine, but I can easily imagine it would mess up things regarding e10s support now that you highlight this behavior.
This code seems to confirm the multiple root and global actors:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#952

But do we really create a new connection for each tab when opening toolboxes or are you creating new connections from the addons?
Flags: needinfo?(poirot.alex)
> But do we really create a new connection for each tab when opening toolboxes
Yes. You can test it by putting a log into DebuggerServerConnection ctor
and activating the Toolbox on two tabs.

This code show instantiating DebuggerClient for every Toolbox when
calling makeRemote on the associated target:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/target.js#388

openToolbox method calls makeRemote:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#359

Honza
Summary: Custom global actor is instantiated again when attached → A global actor is instantiated for every toolbox
Arf, that looks like quite a waste of ressources to do such thing.
And we create a new client for each tab.

There may be some historical choice that I'm not aware of, but I would ensure creating only one client and share more, if not everything!

Panos, any thoughts?
Flags: needinfo?(past)
Every time a toolbox opens in a tab, a new client is created and consequently a new connection to the single server. Clients contain toolbox state. Sharing them would be a state management nightmare. Actors (even global ones) are scoped per connection in order to avoid interference between separate debugging sessions and needing to maintain state in the actor for each connection. Sharing global actors is a possibility that trades connection management complexity with actor complexity. I believe we made the right choice here, considering we rarely ever introduce new connection types, whereas we keep adding new global actors all the time.
Flags: needinfo?(past)
But then, I'm wondering what is the memory impact of such choice now that we have e10s.
Hard to tell... but that could be worth looking at number.
Having said that, I would imagine the typical usecase is to open a toolbox against one tab, I'm not sure opening many toolboxes is a common scenario?

But the fact that we don't share global actors means that we have to be extra careful about state that needs to be shared across tabs/clients, like new dynamicaly registered actors.

Also a lot of stuff changed with e10s, I don't think there is much important state in the parent process.
Most of the tab specific state now lives in the child process/tab actors.
If I follow the codepath correctly, we intanciate a new client in the parent process just to call listTabs and create the RemoteBrowserTabActor() itself instanciating a new server in child with the tab actors. And each client is going to do the same, but always for a different RemoteBrowserTabActor instance. Are we really going to share some state at some point?
What I fear the most in sharing is message disorder, connection failure that would break all toolboxes at once.
> But the fact that we don't share global actors means that we have to be extra careful
> about state that needs to be shared across tabs/clients
So, how can we actually share state across tabs on the backend?

Honza
One more question, what are the main differences between global and tab actor?
When an extension developer should use global and when tab actor?

I am thinking about:
* global actors are instantiated in the parent process while tab actors in the child process (in case of e10s)
* tab actor has direct access to the parent tab browser while global actor does not?
* what else?

Honza
(In reply to Alexandre Poirot [:ochameau] from comment #7)
> But then, I'm wondering what is the memory impact of such choice now that we
> have e10s.
> Hard to tell... but that could be worth looking at number.
> Having said that, I would imagine the typical usecase is to open a toolbox
> against one tab, I'm not sure opening many toolboxes is a common scenario?

Agreed.

> But the fact that we don't share global actors means that we have to be
> extra careful about state that needs to be shared across tabs/clients, like
> new dynamicaly registered actors.
> 
> Also a lot of stuff changed with e10s, I don't think there is much important
> state in the parent process.
> Most of the tab specific state now lives in the child process/tab actors.
> If I follow the codepath correctly, we intanciate a new client in the parent
> process just to call listTabs and create the RemoteBrowserTabActor() itself
> instanciating a new server in child with the tab actors. And each client is
> going to do the same, but always for a different RemoteBrowserTabActor
> instance. Are we really going to share some state at some point?
> What I fear the most in sharing is message disorder, connection failure that
> would break all toolboxes at once.

I don't understand the circumstances that could lead to messages being delivered out of order, care to elaborate?  About client state, remember that we are not in an e10s world yet, and we won't be for the near future. Let's reevaluate our tradeoffs when we get there.

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> So, how can we actually share state across tabs on the backend?

Presumably global-scoped actors would utilize some singleton object/service that would manage the state of the actual resource. Hopefully that answers your question.

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> One more question, what are the main differences between global and tab
> actor?
> When an extension developer should use global and when tab actor?
> I am thinking about:
> * global actors are instantiated in the parent process while tab actors in
> the child process (in case of e10s)
> * tab actor has direct access to the parent tab browser while global actor
> does not?
> * what else?

In the pre-e10s world, a tab actor gets a reference to the tab that it can use to manage its resources more efficiently. A global actor would only get a reference to the browser window. Of course a global actor could use the window manager service to manually retrieve the selected tab and operate solely on it, but that would be silly.

In essence then, the choice between global and tab actor would amount to the actor's answer to the question "what is it that I'm managing"? If the managed resource is contained in a tab, then it would make sense to have a tab actor. If not, a global actor would be more appropriate.

e10s muddles this distinction somewhat, especially with a single content process, since there are actors in both sides of the process divide (e.g. RemoteBrowserTabActor and ContentActor). I think this is what is confusing you, but it is an entirely orthogonal issue to the tab or global scope.
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.