1119794 - TypeError: tab.removeActorByName is not a function

Status: RESOLVED INVALID

(DevTools :: Framework, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

RemoteBrowserTabActor (not derived from TabActor) must implement "removeActorByName" method.

Remote tab-actor doesn't have _tabActorPool and it should send a message to the child process to do the job, correct?

Alex, where exactly is the (tab) actor stored in the child process?

Honza

Comment 1

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #0)
> RemoteBrowserTabActor (not derived from TabActor) must implement
> "removeActorByName" method.
> 
> Remote tab-actor doesn't have _tabActorPool and it should send a message to
> the child process to do the job, correct?

Yes, if you want to remove the actor also in child processes.

> 
> Alex, where exactly is the (tab) actor stored in the child process?

In a TabActor instanciated via DebuggerServer.connectToChild -> child.js and message manager messages.
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js#52

Note that RemoteTabActor is only for OOP tabs in Firefox Desktop,
a similar pattern but with a different codepath exists for apps on Firefox OS.
They are instanciated, from the parent process over here, from webapps actors:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#961

But instead of digging deep in each codepath, I think it would be easier to broadcast a message to all childs (from main.js removeActorByName), and listen from child.js for such message.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1119794-1.patch

Alex, I tried to create a patch for this, but didn't succeed much. Can you please take a look and let me know what could be wrong with this? Any better ideas how to implement it?

Honza

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8547678 [details] [diff] [review]
bug1119794-1.patch

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

I don't know, it looks good to me.
We already have some usecase where we use globalmessagemanager to communicate with child.js and its addMessageListener (debug-setup-in-child message).

::: toolkit/devtools/server/child.js
@@ +53,5 @@
>      let actorPool = new ActorPool(conn);
>      actorPool.addActor(actor);
>      conn.addActorPool(actorPool);
>  
> +    let onRemoveTabActor = DevToolsUtils.makeInfallible(function(msg) {

So I imagine you issue is that this listener is never called?

::: toolkit/devtools/server/main.js
@@ +1046,5 @@
>          for (let connID of Object.getOwnPropertyNames(this._connections)) {
> +          let rootActor = this._connections[connID].rootActor;
> +          if (rootActor) {
> +            rootActor.removeActorByName(name);
> +          }

Out of curiosity, have you seen any case where there is no rootActor on a connection? If yes, do you remember in what scenario?

@@ +1058,5 @@
> +
> +      // Remove actor instances from all child processes.
> +      gMessageManager.broadcastAsyncMessage("debug:remove-tab-actor", {
> +        actor: aActor
> +      });

Are you sure this code doesn't throw and is able to serialize aActor correctly?

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1119794-2.patch

Attaching new patch, it works for me now.

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Out of curiosity, have you seen any case where there is no rootActor on a
> connection? If yes, do you remember in what scenario?
The root actor isn't available in child processes, so any time a tab actor is registered and consequently unregistered even from all child processes.

It would be great if the ContentActor instance is globally accessible inside the child process. Just like the rootActor in the parent process. In such case we could do something like as follows (in DebuggerServer.removeTabActor):

// The rootActor isn't available in child process. Child processes have
// content actor.
if (rootActor) {
  rootActor.removeActorByName(name);
}
else if (contentActor) {
  contentActor.removeActorByName(name);
}

Or we could perhaps set the DebuggerServerConnection.rootActor to point to the instance of ContentActor in child processes? Not sure how big change this would be.

Honza

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8550285 [details] [diff] [review]
bug1119794-2.patch

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

Wait a minute. Is all this stuff really necessary?
DebuggerServer.removeTabActor is not meant to care about e10s and is only going to remove actors in its own process.
(Given that registerModule/addTabActor is not e10s compatible... there is no point in making the removal e10s compatible!)

For actor registry, actor-registry-utils.js:exports.unregisterActor is going to handle e10s.

If all this is correct, implementing an empty removeActorByName method on BrowserTabActor will be enough!

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8550285 [details] [diff] [review]
> bug1119794-2.patch
> 
> Review of attachment 8550285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wait a minute. Is all this stuff really necessary?
> DebuggerServer.removeTabActor is not meant to care about e10s and is only
> going to remove actors in its own process.
> (Given that registerModule/addTabActor is not e10s compatible... there is no
> point in making the removal e10s compatible!)
> 
> For actor registry, actor-registry-utils.js:exports.unregisterActor is going
> to handle e10s.
> 
> If all this is correct, implementing an empty removeActorByName method on
> BrowserTabActor will be enough!

Having only the empty RemoteBrowserTabActor.removeActorByName() solves the exception, but it's not enough - I can still see instances of the registered actors in back-end pools (and I don't see anything in the pools if I apply the entire patch).

Honza

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Having only the empty RemoteBrowserTabActor.removeActorByName() solves the
> exception, but it's not enough - I can still see instances of the registered
> actors in back-end pools (and I don't see anything in the pools if I apply
> the entire patch).

When? What kind of actors? When you try to unregister actors via ActorActor.unregister?

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to Jan Honza Odvarko [:Honza] from comment #6)
> > Having only the empty RemoteBrowserTabActor.removeActorByName() solves the
> > exception, but it's not enough - I can still see instances of the registered
> > actors in back-end pools (and I don't see anything in the pools if I apply
> > the entire patch).
> 
> When? What kind of actors? When you try to unregister actors via
> ActorActor.unregister?

Here is my scenario:

1. Run Firebug
2. Click a test button that registers one global and one tab actor (using ActorRegistryActor). These actors are also attached (i.e. also instances are created).
3. Inspecting list of global and tab factories (in DebuggerServer) shows that there is one new global and one new tab factory in these lists. Note that they are available in both, the parent and child process.
4. Inspecting actor pools (in DebuggerServerConnection) shows that there is an instance of the global actor (in one of the extra pools within the parent process) and also one instance of the tab actor (in one of the extra pools within the child process).
5. Click a test button that unregisters both, the global and tab actor.
6. Inspecting actor factories again shows that all registered factories are gone (from the parent as well as the child process). So far so good.
7. Inspecting the actor pools shows that instance of the global actor as well as instance of the tab actor are still there -> BUG

My patch removes also actor instances from the pools (from both, child and parent process). It might be possible to simplify the patch (that would be great), but actor instances should be also removed from the pools, I believe.

Honza

Comment 9

[Alexandre Poirot [:ochameau]]

Ok, ok, I see. The thing is that in this patch you are adding yet another codepath to unregister actors, whereas there is already one in actor-registry-utils.js:exports.unregisterActor.
That fails in child as Debugger.remove{Tab,Global}Actor isn't able to call TabActor.removeActorByName as ContentActor, in child.js isn't registered as rootActor. We could set it as rootActor, but I'm not sure it is a good idea as it isn't really a root actor.
The more I look at removeActorByName the more I think it is too complex.
DebuggerServerConnection should have a removeActorByName and go through all pools and all of their actors and check for actor name. But I think I already suggested you that and it wasn't possible as there isn't the name information in pools/actors. But at the end, It would simplify everything if we make this information available.
For example, we could expose actor name from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/common.js#112
  instance.actorName = this.actorName;
We would have to pass actor name from here to ObservedActorFactory:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/common.js#79
  new ObservedActorFactory(..., this.name);
In order to just call DebuggerServerConnect.removeActorByName from remove{Tab,Global}Actor:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#1042
  this._connections[connID].removeActorByName(name);

The benefit of this is that we wouldn't have to care about actors hierarchy, nor ensure to implement/call all the possible removeActorByName on all kind of actors!!

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1119794-3.patch

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Ok, ok, I see. The thing is that in this patch you are adding yet another
> codepath to unregister actors, whereas there is already one in
> actor-registry-utils.js:exports.unregisterActor.
Good point

I am attaching new version.

Some comments:
* Using 'typeName' instead of ctor name. The type name is taken from registered actor (actor.prototype.typeName) after evaluation. This way we are closer to protocol.js conventions (and it's safer than ctor name).
* this._tabActorPool2 pool is created just once. Otherwise a new pool was created every time when a form is needed e.g. for 'listTabs' packet (there was many empty pools in the child connection as the result).
* contentActor instance is exposed to the parent connection object (just like rootActor in the parent process), so it can be accessed to call removeActorByName (see DebuggerServer.remove{Tab|Global}Actor methods)
* Actors based on protocol.js put themselves as pools into the parent connection (!). This doesn't seem right. This is why an extra 'else' is needed in DebuggerServerConnection.removeActorByName. Should I file another bug for this?


Honza

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8557168 [details] [diff] [review]
bug1119794-3.patch

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

I'm sorry Honza but this is still not good, but really, big thanks for going so deep in the codebase,
I'm learning it with all the bugs you fill!

::: toolkit/devtools/server/actors/childtab.js
@@ +74,4 @@
>      this.conn.addActorPool(this._tabActorPool2);
>    }
>  
> +  this._createExtraActors(DebuggerServer.tabActorFactories, this._tabActorPool2);

I think you will end up registering duplicated factories here,
because of this check in createExtraActors (from common.js):
    if (!aPool.has(actor.actorID)) {
      aPool.addActor(actor);
    }
I don't really know the conventions behind form(), it predates my arrival within the codebase, it looks like it is meant to be called only once, but we are calling it multiple times. If we call more than once, even within TabActor codepath (if we ignore ContentActor), _tabActorPool will be overriden and the previous one leaked in the connection.
But here in ContentActor, given the comment of form override, it looks like we are calling this multiple times. 
And I verified, we do call it everytime we reconnect and reopen a toolbox against the same application. But DebuggerServerConnection._dumpPools() doesn't seem to highlight any garbage in pools. I don't really know why.

::: toolkit/devtools/server/main.js
@@ +1282,5 @@
> +        }
> +        this.removeActorPool(pool, true);
> +      }
> +    }
> +  },

Instead of hacking into ActorPool privates (_actors),
you should expose a removeActorByName method on ActorPool(common.js) and on Pool(protocol.js).
So that you just iterate here.
  for (let pool of this._extraPools) {
    pool.removeActorByName(name);
  }

I don't think that's an issue of adding protocol.js actors as pool as soon as they expose the pool API!

Also you would not need the conn.contentActor workaround and simplify the whole story, if you look for a "removeActorByName" method on all actors when calling ActorPool.removeActorByName:

ActorPool.removeActorByName = function () {
   pool.forEach(actor => {
     if (name == actor.typeName) {
       pool.removeActor(actor);
       if (actor.disconnect)
         actor.disconnect();
     } else if (typeof(actor.removeActorByName)=="function") {
       actor.removeActorByName(name);
     }
  }
}

Actually, if TabActor was doing like protocoljs, I think we wouldn't need that special removeActorByName on actors... it would be a pool and expose removeActorByName like all pools (and this method makes sense on pool, whereas on actors it feel more like a hack).

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I believe that this bug isn't valid anymore, closing.
Feel free to reopen if you thing it should still be completed.