Closed Bug 1473513 Opened Last year Closed Last year

Browsing context target actor: refactor Protocol.js actor pools

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: yulia, Assigned: yulia)

References

(Blocks 1 open bug)

Details

Attachments

(30 files, 3 obsolete files)

59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
ochameau
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
With moving browsing context target actor to protocol.js we have a bit of further refactoring work to do on the pools: We have three pools on the browsing context target: contextPool, targetScopedActorPool, and workerActorPool. targetScopedActorPool and workerActorPool have difference lifespans from the browsing context target actor, so we need to find a solution that works within protocol.js and is less hacky than what we currently have.

Options include: using the Pool provided by protocol js, or creating managing actors that delegate to the browsing context target actor
Severity: normal → enhancement
I have made a naive re-implementation of the ObservedActorFactories class. I am not sure if this is the best solution, and maybe this makes more sense in protocol.js eventually. Feedback is greatly appreciated
try run: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=fab0677050c0a7b2db855ec3d7a5865ed0f73901

one more thing about implementing lazy actors in protocol.js -- if we do that, then we can extend the Pool prototype, and get the parent pool from there. it might be cleaner than passing the pool in...
(In reply to Yulia Startsev [:yulia] from comment #3)
> try run:
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=fab0677050c0a7b2db855ec3d7a5865ed0f
> 73901

FYI this try syntax is wrong and only ran xpcshell and damp.

A good equivalent is the following one:
  try: -b do -p linux64 -u xpcshell,mochitest-dt,mochitest-chrome -t damp-e10s --rebuild-talos 6 --artifact
(The main error was having a 's' on mochitest-dt)

DAMP reports an error:
06:38:00     INFO -  PID 8129 | Loading test 'inspector/cold-open.js'
06:38:00     INFO -  PID 8129 | Executing test 'inspector/cold-open.js'
06:38:01     INFO -  PID 8129 | Garbage collect
06:38:01     INFO -  PID 8129 | console.error: "Error while calling actor 'frameTarget's method 'detach'" "this.targetScopedActorPool is undefined"
06:38:01     INFO -  PID 8129 | console.error: "_detach@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/targets/browsing-context.js:1019:7\ndetach@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/targets/browsing-context.js:1061:10\nhandler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1173:21\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1783:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:735:7\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:709:7\nready@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:726:7\n_onConnection@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1201:5\nconnectToParent@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:683:12\nonConnect</<@resource://devtools/server/startup/frame.js:49:22\nonConnect<@resource://devtools/server/startup/frame.js:48:7\nexports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:71:5\n@resource://devtools/server/startup/frame.js:19:4\n"
06:38:01    ERROR -  PID 8129 | Handler function threw an exception: TypeError: this.threadActor is null

But I'll have a look at your patch and provide feedback.
Comment on attachment 8990292 [details]
Bug 1473513 - use Protocol.js pools for browsing context target actor;

https://reviewboard.mozilla.org/r/255318/#review262466

It looks like you are drafting a good plan to move forward on this!
I think we can split this is pieces to ease discussing/landing this by smaller, more focused steps.

::: devtools/server/actors/targets/browsing-context.js:103
(Diff revision 1)
>                 .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
>  }
>  
> +/*
> + * Methods shared between RootActor and BrowsingContextTargetActor.
> + */

Do you think you can modify both Root and BrowingContext at once and so modify common.js helpers directly instead of copying most of it here?

I ask this for two reasons:
1) It would help me see what are the differences between code in common.js and the copy here.
2) It would help you make simplifications to both create and append extra actors helpers.

But it is also totally fine if you prefer doing it incrementaly!

::: devtools/server/actors/targets/browsing-context.js:172
(Diff revision 1)
> +  this._name = name;
> +  this._pool = pool;
> +
> +  // needed for taking a place in a pool
> +  this.typeName = actorFactory.prefix;
> +  this.actorID = actorFactory.actorID;

I understand for typeName, but actorID should be computed when calling pool.manage()?
(Note that bug 1467565 is going to help unify all variable names to typeName instead of going from/to prefix/typeName everywhere ;))

::: devtools/server/actors/targets/browsing-context.js:183
(Diff revision 1)
> +  this._pool = null;
> +};
> +
> +LazyActor.prototype.createActor = function() {
> +  // Fetch the actor constructor
> +  const C = this._factory._getConstructor();

I don't remember why I made this `RegisteredActorFactory` intermediate class. (may be for old addons compatibility...)
But this class looks useless, the main content of it is `_getConstructor` and it would be easier to move it to `LazyActor`. (but it may hard to do while RootActor is still using the old codebase from common.js...)
DebuggerServer.targetScopedActorFactories and DebuggerServer.globalActorFactories may be dictionaries of `{ options: actor, prefix: name }` instead of `RegisteredActorFactory`.
Then in `createExtraActors` you would pass that `{ options, prefix }` to LazyActor instead of a `RegisteredActorFactory`.

May be that's a simplication we can make today on existing codebase that would help write your current patch?

::: devtools/server/actors/targets/browsing-context.js:198
(Diff revision 1)
> +  // actor. Reusing the existing actor ID will make sure Pool.manage
> +  // replaces the old actor with the new actor.
> +  instance.actorID = this.actorID;
> +
> +  this._pool.manage(instance);
> +  this.destroy();

What is the value of nullifying actorID/pool here?

::: devtools/server/actors/targets/browsing-context.js
(Diff revision 1)
> -        pool.addActor(actor);
> +        pool.manage(actor);
>        }
>  
> -      this.conn.removeActorPool(this._workerTargetActorPool);
> +      this._workerTargetActorPool.destroy();
>        this._workerTargetActorPool = pool;
> -      this.conn.addActorPool(this._workerTargetActorPool);

It looks like it is simplier to convert `_workerTargetActorPool` to protocol.js's Pool than the targetScopedActorPool.
So it would be worth doing it in a dedicated changeset.
You may push just that to try to see if that works as-is. For some reason, I thought it was harder to reuse protocol.js Pool object like that. But lookint at Pool implementation, it seems to make sense.

::: devtools/server/main.js:1610
(Diff revision 1)
>                              message: "No such actor for ID: " + actorID });
>        return null;
>      }
>  
>      // Dynamically-loaded actors have to be created lazily.
> -    if (actor instanceof ObservedActorFactory) {
> +    if (actor instanceof ObservedActorFactory || actor.isLazy) {

Is there any reason to do such duck typing here instead of using `instanceof` like the old codebase?
Attachment #8990292 - Flags: review?(poirot.alex)
(In reply to Yulia Startsev [:yulia] from comment #3)
> one more thing about implementing lazy actors in protocol.js -- if we do
> that, then we can extend the Pool prototype, and get the parent pool from
> there. it might be cleaner than passing the pool in...

It is not really clear what you have in mind here.
I had in mind to replace these factories classes by a subclass of Pool,
where `get` method is overloaded to handle laziness.
Today we hack into DebuggerServerConnection here:
  https://searchfox.org/mozilla-central/source/devtools/server/main.js#1536
But it may be better handled into a custom pool as lazy actors only exist in RootActor and BrowsingContextActor.
Attachment #8990292 - Attachment is obsolete: true
Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

https://reviewboard.mozilla.org/r/256022/#review262876

Looks good, thanks for the dedicated patch :)

You may want to apply the same change to these other actors in the same patch:
https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#433-440
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/content-process.js#127-134
Attachment #8991026 - Flags: review?(poirot.alex) → review+
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review263140


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/protocol.js:1026
(Diff revision 1)
> +    }
> +  }
> +}
> +
> +exports.createExtraActors = createExtraActors;
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]
I have a work in progress from today's experiments. There is still an issue with cleanup -- when the browser is shut down, if the memory actor has been instantiated, it complains that no such actor exists. I think this has to do with `manage`/`unmanage` but I will return to this tomorrow. 

One spot where I am struggling is with `getActor`/_getorCreateActor`. 

Since I have moved the instantiation of lazy actors into protocol.js (which I do think makes more sense), I am not too sure about how to handle errors. There is some transport that needs to be done, but I would like to do it in a way that is more elegant, perhaps removing the distinction between these two functions. 

One issue that I see with these two is that _getOrCreateActor has a lot of legacy cases, however it is accessed from outside of this class even though it is sort of "private". I would rather have everything use the `getActor` method.

Tomorrow I will pick up on this, and see about cleaning up `registerModule` and the other lazy actor infrastructure and see if I can make it a little tighter
New try run for the registerModule changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f9fd95adbb27f080b19053370cbec1ebb68f2f7

As a next step, i would like to refactor the actor-registry file, you can see some first steps at the top of the file with the browserActorList. We have a lot of duplication of names and weird relationships between things, and I would like this to be a bit cleaner.

There are also a couple of spots where the registry is interfacing with the main.js file. Namely in setupInChild and a few remaining register methods. I would like to move those to the actorRegistry, and have actorRegistry rely on debuggerServer rather than the other way around. This way we could get rid of the weird actorRegistry._connections and merge actorRegistry with actorRegistryUtils.
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review263978

Thanks Yulia. It looks like a nice simplification, but let's not move complex things from one complex class to another complex class. I think we can move that to its own class/module.

::: devtools/shared/protocol.js:894
(Diff revision 4)
>    actor: function(actorID) {
> -    return this.__poolMap ? this._poolMap.get(actorID) : null;
> +    if (this.__poolMap) {
> +      const entry = this._poolMap.get(actorID);
> +      if (entry instanceof LazyActor) {
> +        return entry.createActor();
> +      }

This concept of laziness and LazyActor is something specific to RootActor and BrowsingContextActor.
To me it shouldn't be in protocol.js. Protocol.js should only be about the base actor/front implementations.
So I would rather see RootActor BrowsingContextActor use a dedicated subclass of Pool that supports lazy actors. That, outside of protocol.js, so that you clearly see what specialized code from the Pool connect to LazyActor code.

::: devtools/shared/protocol.js:910
(Diff revision 4)
> +      if (entry instanceof LazyActor) {
> +        return entry.createActor();
> +      }
> +      return entry;
> +    }
> +    return null;

(not really related to your patch, but get should call actor instead of duplicating its implementation)

::: devtools/shared/protocol.js:1034
(Diff revision 4)
> +    // so make sure not to overwrite it by a non-instantiated version.
> +    if (!pool.has(actor.actorID)) {
> +      pool.manage(actor);
> +    }
> +  }
> +}

Then, once we have a dedicated Pool class, I'm wondering if this method should become part of such Pool. But we can look into this as a followup as it may force to also move appendExtraActors and modify _extraActors.

I don't know if you saw that, but we always have the same pattern:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#482-494
https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#240-248
1) Create a singleton pool
2) call createExtraActors
3) call appendExtraActors
4) return the augmented response

::: devtools/shared/protocol.js:1101
(Diff revision 4)
> +
> +  this._pool.manage(instance);
> +  this.destroy();
> +
> +  return instance;
> +};

It would be really great if you could copy common.js so that we keep the changelog of this code, and I can more easily what changed ;)
Attachment #8991344 - Flags: review?(poirot.alex)
Comment on attachment 8991968 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer [WIP];

https://reviewboard.mozilla.org/r/256862/#review263982

Looks good to me with a capital A ;)
(you may have to rebase against bug 1474980)

::: devtools/server/actor-registry.js:456
(Diff revision 1)
> -    });
> -
> -    this._actorResponses.set(from, responsePromise);
> -  },
>  
> -  /**
> +exports.actorRegistry = LazyActorRegistry;

Singleton objects like this one are commonly exported with camel case and don't use different names internally:
actor-registry.js would export ActorRegistry and contains ActorRegistry object.

So I'm suggesting doing that on line 48:
 exports.ActorRegistry = {
   ...
 };

Note that some modules also export their unique singleton like that (DevToolsUtils is a good example), you would have to do that instead:
  module.exports = {
    ...
  };

Thanks for having done a copy, that really helps ready your patch :)

::: devtools/server/main.js:109
(Diff revision 1)
>      if (this.initialized) {
>        return;
>      }
>  
>      this._connections = {};
> +    actorRegistry._connections = this._connections;

This is too cryptic. Please add an `init` method to the registry and get it called from here.

::: devtools/server/main.js:186
(Diff revision 1)
>        const { createRootActor } = require("devtools/server/actors/webbrowser");
>        this.setRootActor(createRootActor);
>      }
>  
>      if (target) {
> -      this._addTargetScopedActors();
> +      actorRegistry._addTargetScopedActors();

You should unprefix these two _addBrowserActors and _addTargetScopedActors now that it is being called form an external class.
Attachment #8991968 - Flags: review?(poirot.alex) → review+
I have made an update to the lazy actor class. I realized that the collections that extraActors held and the pool were essentially the same, with the exception that extraActors never get replaced with real actors. A couple of tests will fail, but that is expected. I am moving on to updating the actor registry so that it makes sense with this pool. If there are naming issues, or something is unclear, let me know -- I am happy to update
Attachment #8991968 - Attachment is obsolete: true
Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

https://reviewboard.mozilla.org/r/259104/#review266116


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/main.js:181
(Diff revision 1)
> -   *          - "target"
> -   *            registers a target-scoped actor instance, if true.
> -   *            A new actor will be created for each target, such as a tab.
>     */
> -  registerModule(id, options) {
> -    if (id in gRegisteredModules) {
> +  addActors(url) {
> +    loadSubScript.call(this, url);

Error: 'loadSubScript' is not defined. [eslint: no-undef]
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review266118


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/targets/browsing-context.js:26
(Diff revision 7)
>   */
>  
>  var { Ci, Cu, Cr, Cc } = require("chrome");
>  var Services = require("Services");
>  const ChromeUtils = require("ChromeUtils");
> -var {
> +var { ActorPool, appendExtraActors } = require("devtools/server/actors/common");

Error: 'ActorPool' is assigned a value but never used. [eslint: no-unused-vars]
Attachment #8994554 - Attachment is obsolete: true
Attachment #8994554 - Flags: review?(poirot.alex)
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review266330


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/targets/browsing-context.js:26
(Diff revision 8)
>   */
>  
>  var { Ci, Cu, Cr, Cc } = require("chrome");
>  var Services = require("Services");
>  const ChromeUtils = require("ChromeUtils");
> -var {
> +var { ActorPool, appendExtraActors } = require("devtools/server/actors/common");

Error: 'ActorPool' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

https://reviewboard.mozilla.org/r/259104/#review266338


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/tests/mochitest/test_connectToFrame.html:70
(Diff revision 3)
>        }
>      };
>      TestActor.prototype.requestTypes = {
>        "hello": TestActor.prototype.hello
>      };
> -    DebuggerServer.addTargetScopedActor({
> +    ActorRegistry.addTargetScopedActor({

Error: 'ActorRegistry' is not defined. [eslint: no-undef]
Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

https://reviewboard.mozilla.org/r/259104/#review266340


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/tests/mochitest/test_connectToFrame.html:70
(Diff revision 2)
>        }
>      };
>      TestActor.prototype.requestTypes = {
>        "hello": TestActor.prototype.hello
>      };
> -    DebuggerServer.addTargetScopedActor({
> +    ActorRegistry.addTargetScopedActor({

Error: 'ActorRegistry' is not defined. [eslint: no-undef]
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review266610

It looks good to me. I hope the comprehension of extra actors will now be easier to understand for new comers!

::: devtools/shared/protocol/lazy-pool.js:20
(Diff revision 9)
> - * These actors only exposes:
> - * - `name` string attribute used to match actors by constructor name
> - *   in DebuggerServer.remove{Global,Tab}Actor.
> - * - `createObservedActorFactory` function to create "observed" actors factory
> - *
> - * @param options object
> + * objects. Pools are used on both sides of the connection to help coordinate lifetimes.
> + *
> + * @param optional conn
> + *   Either a DebuggerServerConnection or a DebuggerClient.  Must have
> + *   addActorPool, removeActorPool, and poolFor.
> + *   conn can be null if the subclass provides a conn property.

If I'm following correctly, `conn` is always set in the case of LazyPool. That is much better than Pool's behavior. so I think we should assume here that it is always passed as a constructor argument and remove the `if (conn)`.

This comment looks more like a documentation for Pool rather than LazyPool. As LazyPool is only server side and will only receive a DebuggerServerConnection object as first argument.

::: devtools/shared/protocol/lazy-pool.js:43
(Diff revision 9)
> -  this._parentActor = parentActor;
> -
> -  this.actorPrefix = prefix;
>  
> -  this.actorID = null;
> -  this.registeredPool = null;
> +  // Same as actor, should update debugger connection to use 'actor'
> +  // and then remove this.

Is there a bug to followup on that?
I imagine it should be pretty straightforward to address.

::: devtools/server/actors/root.js:248
(Diff revision 9)
> -      this.conn.addActorPool(this._globalActorPool);
>      }
> -    this._createExtraActors(this._parameters.globalActorFactories, this._globalActorPool);
> +    createExtraActors(this._parameters.globalActorFactories, this._globalActorPool, this);
>  
>      // List the global actors
>      this._appendExtraActors(reply);



::: devtools/server/actors/targets/browsing-context.js:495
(Diff revision 9)
> -      this._targetScopedActorPool);
> +      DebuggerServer.targetScopedActorFactories,
> +      this._targetScopedActorPool,
> +      this
> +    );
>  
> -    this._appendExtraActors(response);
> +    return { ...response, ...addedActors };

It may be easier to read with your code, but the following would prevent having to copy `response`:
```
  Object.assign(response, addedActors);
  return response;
```
(Note that you may also pass `response` to `createExtraActors` in order to let it append the actor's IDs. In such case, may be createExtraActors would be better named appendExtraActors?)

::: devtools/server/main.js
(Diff revision 9)
> -        actor = actor.createActor();
> -      } catch (error) {
> -        const prefix = "Error occurred while creating actor '" + actor.name;
> -        this.transport.send(this._unknownError(actorID, prefix, error));
> -      }
> -    } else if (typeof (actor) !== "object") {

That's great to see this code go away from here!

::: devtools/server/main.js:1481
(Diff revision 9)
> -        const prefix = "Error occurred while creating actor '" + actor.name;
> -        this.transport.send(this._unknownError(actorID, prefix, error));
> -      }
> -    } else if (typeof (actor) !== "object") {
> -      // ActorPools should now contain only actor instances (i.e. objects)
> +        // ActorPools should now contain only actor instances (i.e. objects)
> -      // or ObservedActorFactory instances.
> +        // or ObservedActorFactory instances.

You should update this comment. There should be only actor instances right here.

::: devtools/server/tests/unit/testactors.js:157
(Diff revision 9)
>      delete this._extraActors[name];
>    },
>  
>    /* Support for DebuggerServer.addTargetScopedActor. */
> -  _createExtraActors: createExtraActors,
>    _appendExtraActors: appendExtraActors



::: devtools/shared/transport/tests/unit/testactors.js:114
(Diff revision 9)
>      return { type: "detached" };
>    },
>  
>    /* Support for DebuggerServer.addTargetScopedActor. */
> -  _createExtraActors: createExtraActors,
>    _appendExtraActors: appendExtraActors
Attachment #8991344 - Flags: review?(poirot.alex) → review+
Comment on attachment 8994817 [details]
Bug 1473513 - make log function safer so that tests do not time out;

https://reviewboard.mozilla.org/r/259356/#review266652

::: devtools/client/shared/redux/middleware/log.js:27
(Diff revision 2)
> +       */
>      } catch (e) {
> +      // this occurs if an object has a cyclical value and is printed out.
> +      console.warn(e);
>        console.log("[DISPATCH]", action);
>      }

I'm wondering if we should just do:
  console.log(`[DISPATCH] action type: ${action.type}`);

It looks like we only pass actions with type property.
It currently prints:
```
  console.log: "[DISPATCH]" "{\n  \"type\": \"take-census-start\"\n}"
```
which is very verbose for little reason, compared to what we could print:
```
  console.log: "[DISPATCH] action type: take-census-start"
```
Attachment #8994817 - Flags: review?(poirot.alex) → review+
Comment on attachment 8994818 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

https://reviewboard.mozilla.org/r/259358/#review266654

::: devtools/server/actors/root.js:246
(Diff revision 2)
> +      this._parameters.globalActorFactories,
> +      this._globalActorPool,
> +      this
> +    );
>  
> -    // List the global actors
> +    return { from: this.actorID, ...actors };

Returning `from` attribute manually from actors is a relic from ancient times.
Here you should be able to do:
```
  return actors;
```

(`from` is now automatically set by protocol.js here:
  https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#1188
  or for old actors here:
  https://searchfox.org/mozilla-central/source/devtools/server/main.js#1528 )
Attachment #8994818 - Flags: review?(poirot.alex) → review+
Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

https://reviewboard.mozilla.org/r/259104/#review266658

Thanks a lot, this simple code move is really simplifying the comprehension of these global/target-scoped actors and how they are managed!

::: devtools/client/debugger/test/mochitest/browser_dbg_target-scoped-actor-01.js:1
(Diff revision 5)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Note that you may have to rebase against bug 1478244 to land your patch.
Attachment #8994555 - Flags: review?(poirot.alex) → review+
Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

https://reviewboard.mozilla.org/r/256246/#review266610

> Is there a bug to followup on that?
> I imagine it should be pretty straightforward to address.

I made a new bug to address this, as this was in the original code already: https://bugzilla.mozilla.org/show_bug.cgi?id=1478973
I will be away next week, and so far i haven't been able to track down the regression. It looks like we have two new writes that we didnt have before. here is a profile comparison: https://perfht.ml/2vaoYm2
Comment on attachment 9002742 [details]
Bug 1473513 - refactor main.js to use protocol.js pools;

https://reviewboard.mozilla.org/r/261710/#review269674

\o/ Do you know where we are still using old ActorPool after all this work?

::: devtools/server/tests/unit/test_protocol_children.js:251
(Diff revision 1)
>    initialize: function(conn) {
>      rootActor = this;
>      this.actorID = "root";
>      this._children = {};
>      protocol.Actor.prototype.initialize.call(this, conn);
>      // Root actor owns itself.

You should also remove this comment
Attachment #9002742 - Flags: review?(poirot.alex) → review+
Comment on attachment 9002743 [details]
Bug 1473513 - reduce number of poolFor calls;

https://reviewboard.mozilla.org/r/261712/#review269672

Thanks a lot for all this patch queue, it is surely going to help working on all of that!

::: devtools/shared/protocol.js:869
(Diff revision 1)
> -    // TODO: not all actors have been moved to protocol.js, so they do not all have
> +      // TODO: not all actors have been moved to protocol.js, so they do not all have
> -    // a parent field. Remove the check for the parent once the conversion is finished
> +      // a parent field. Remove the check for the parent once the conversion is finished
> -    const parent = this.poolFor(actor.actorID);
> +      const parent = this.poolFor(actor.actorID);
> -    if (parent) {
> +      if (parent) {
> -      parent.unmanage(actor);
> +        parent.unmanage(actor);
> -    }
> +      }

Do you think we should keep this if, in the future, we remove the lazy actor things?

(It may be worth keeping that in mind, put a comment somewhere in a possible followup or something)

May be we should comment about why we need to unmanage the actor here. Why an actor already be managed to start with?
Attachment #9002743 - Flags: review?(poirot.alex) → review+
Comment on attachment 9002743 [details]
Bug 1473513 - reduce number of poolFor calls;

https://reviewboard.mozilla.org/r/261712/#review269676

::: devtools/shared/protocol.js:869
(Diff revision 1)
> -    // TODO: not all actors have been moved to protocol.js, so they do not all have
> +      // TODO: not all actors have been moved to protocol.js, so they do not all have
> -    // a parent field. Remove the check for the parent once the conversion is finished
> +      // a parent field. Remove the check for the parent once the conversion is finished
> -    const parent = this.poolFor(actor.actorID);
> +      const parent = this.poolFor(actor.actorID);
> -    if (parent) {
> +      if (parent) {
> -      parent.unmanage(actor);
> +        parent.unmanage(actor);
> -    }
> +      }

I am not sure if it will be resolved by removing lazy actors. I came across this behavior because devtools/server/tests/unit/test_addon_reload.js was failing.

::: devtools/shared/protocol.js:869
(Diff revision 1)
> -    // TODO: not all actors have been moved to protocol.js, so they do not all have
> +      // TODO: not all actors have been moved to protocol.js, so they do not all have
> -    // a parent field. Remove the check for the parent once the conversion is finished
> +      // a parent field. Remove the check for the parent once the conversion is finished
> -    const parent = this.poolFor(actor.actorID);
> +      const parent = this.poolFor(actor.actorID);
> -    if (parent) {
> +      if (parent) {
> -      parent.unmanage(actor);
> +        parent.unmanage(actor);
> -    }
> +      }

I am not sure if it will be resolved by removing lazy actors. I came across this behavior because devtools/server/tests/unit/test_addon_reload.js was failing.
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e1e283b347e
use Protocol.js pools for workerTargetActorPool in Target Actors; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/7acc52a7f81f
create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ce86ea60a31c
make log function safer so that tests do not time out; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/12eb1139a802
remove other instances of ActorPool from browsingContext and root; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6e157bea362a
separate registerModule behavior from DebuggerServer; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/5bf38cfa04f9
refactor main.js to use protocol.js pools; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/deb8812556ef
reduce number of poolFor calls; r=ochameau
create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Depends on D6468
MozReview-Commit-ID: BIk5pWzxJcx

Depends on D6470
MozReview-Commit-ID: GxkLzvxJgdY

Depends on D6471
MozReview-Commit-ID: 3GsXRxcIKfx

Depends on D6473
MozReview-Commit-ID: FNMK4f553yI

Depends on D6474
MozReview-Commit-ID: 9VqKPauAP9j

Depends on D6475
Depends on D6476
Comment on attachment 9010865 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010865 - Flags: review+
Comment on attachment 9010867 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010867 - Flags: review+
Comment on attachment 9010868 [details]
Bug 1473513 - make log function safer so that tests do not time out; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010868 - Flags: review+
Comment on attachment 9010869 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010869 - Flags: review+
Comment on attachment 9010870 [details]
Bug 1473513 - Ensure that actorPools in root are destroyed on destroy; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010870 - Flags: review+
Comment on attachment 9010871 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010871 - Flags: review+
Comment on attachment 9010872 [details]
Bug 1473513 - refactor main.js to use protocol.js pools; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010872 - Flags: review+
Comment on attachment 9010873 [details]
Bug 1473513 - reduce number of poolFor calls; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010873 - Flags: review+
Comment on attachment 9010874 [details]
Bug 1473513 - resolve memory leak

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010874 - Flags: review+
Comment on attachment 9010888 [details]
Bug 1473513 - move ActorRegistry module to server/actors/utils; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #9010888 - Flags: review+
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e431edddacd
use Protocol.js pools for workerTargetActorPool in Target Actors; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/27419719402e
create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6fc241b06e0b
make log function safer so that tests do not time out; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e56c70bd2f9a
remove other instances of ActorPool from browsingContext and root; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c690d2383ca4
Ensure that actorPools in root are destroyed on destroy; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d72f6ff37ca6
separate registerModule behavior from DebuggerServer; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/f9f12b44a3ab
refactor main.js to use protocol.js pools; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/db43912632fb
reduce number of poolFor calls; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/80c51a6e896e
resolve memory leak r=ochameau
https://hg.mozilla.org/integration/autoland/rev/174fe98999ea
move ActorRegistry module to server/actors/utils; r=ochameau
Depends on D6479
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/932554641182
fix faulty path in unit tests. r=fix. CLOSED TREE
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/950ce58bfb24
Fix merge conflict between bug 1473513 and 1485676 against this test. CLOSED TREE
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2f0d061af12
Backed out changeset 950ce58bfb24 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/4df71e54cd06
Backed out changeset 174fe98999ea for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/2b36f3c32272
Backed out changeset 80c51a6e896e for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/794c7e4ab8f8
Backed out changeset db43912632fb for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/cf844f66a6a9
Backed out changeset f9f12b44a3ab for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/1d1aac7f0245
Backed out changeset d72f6ff37ca6 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/44a4558db6fe
Backed out changeset c690d2383ca4 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/4981a0d3ed1c
Backed out changeset e56c70bd2f9a for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/bef0af13e60e
Backed out changeset 6fc241b06e0b for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/f4613b86e353
Backed out changeset 27419719402e for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/a2df10c9e819
Backed out changeset 0e431edddacd for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE
It looks like this got backed out again?: https://hg.mozilla.org/mozilla-central/rev/bef0af13e60e

Tentatively re-opening - hopefully I'm not mistaken here.
Status: RESOLVED → REOPENED
Flags: needinfo?(ystartsev)
Resolution: FIXED → ---
I apologize - this looks like only a partial backout. I'll close this again.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
it was fully backed out. sorry for not getting to this sooner. I have a fix
Status: RESOLVED → REOPENED
Flags: needinfo?(ystartsev)
Resolution: FIXED → ---
No no, you were right, this patch queue was backed out.

It was quite long/sad story.
As there was a merge conflict, the patch broke autoland and as I couldn't push myself to autoland, I had to go through phabricator to hand over the backout to the sheriffs having push credentials to autoland.
But as I'm using phlay, it requires a bug number in the comment, so I put one (unless it is a phabricator requirement?).
Finally, the sheriffs took my backout patches as-is, including the bug number.
That ultimately confused people and/or tooling, considering that's valid patches instead of backouts...
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0187f382339c
use Protocol.js pools for workerTargetActorPool in Target Actors; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/3612d32de44c
create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dab963c9f14
make log function safer so that tests do not time out; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e02f7e5e2d
remove other instances of ActorPool from browsingContext and root; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/f81e08c372b5
Ensure that actorPools in root are destroyed on destroy; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c36b65f23c2
separate registerModule behavior from DebuggerServer; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/04e64ca2ad0f
refactor main.js to use protocol.js pools; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/479aed564448
reduce number of poolFor calls; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1f0957ef4d
resolve memory leak
https://hg.mozilla.org/integration/mozilla-inbound/rev/6436d45c7bd3
move ActorRegistry module to server/actors/utils; r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/48445cd6f7c5
fix faulty path for actor-registry;
You need to log in before you can comment on or make changes to this bug.