1473513 - Browsing context target actor: refactor Protocol.js actor pools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Yulia Startsev [:yulia]]

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

Comment 1

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - use Protocol.js pools for browsing context target actor;

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

Comment 2

[Yulia Startsev [:yulia]]

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

Comment 3

[Yulia Startsev [:yulia]]

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...

Comment 4

[Alexandre Poirot [:ochameau]]

(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 5

[Alexandre Poirot [:ochameau]]

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?

Comment 6

[Alexandre Poirot [:ochameau]]

(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.

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

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

Comment 8

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

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

Comment 9

[Alexandre Poirot [:ochameau]]

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

Comment 10

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

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

Comment 11

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

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

Comment 12

[Code Review Bot [:reviewbot]]

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]

Comment 13

[Yulia Startsev [:yulia]]

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

Comment 14

[Yulia Startsev [:yulia]]

try run to see how this fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5da6a2413483a6182e24b13944ad24a4dbb5a273

Comment 15

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

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

Comment 16

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

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

Comment 17

[Yulia Startsev [:yulia]]

new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea28d21421397cfb4c89cc258d38f3fc4a373a77

Comment 18

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

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

Comment 19

[Yulia Startsev [:yulia]]

new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c832ff0b61a5a185e34cb998ef1d4e735ef58a1a

Comment 20

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

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

Comment 21

[Yulia Startsev [:yulia]]

new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09f78011e19d808ff066e8d67a1a126c221b2906

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - separate registerModule behavior from DebuggerServer [WIP];

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

Comment 23

[Yulia Startsev [:yulia]]

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 24

[Alexandre Poirot [:ochameau]]

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 ;)

Comment 25

[Alexandre Poirot [:ochameau]]

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.

Comment 26

[Yulia Startsev [:yulia]]

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

Comment 27

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/4-5/

Comment 28

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1168d70344bc7f267776c201dd2eab50787075e

Comment 29

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/5-6/

Comment 30

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5045d2c34da646535e758b07de9574548b659e58

Comment 31

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25df133c048169b39ec48b2861add6d40fe32a6c

Comment 32

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

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

Comment 33

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - separate registerModule behavior from DebuggerServer;

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

Comment 34

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256022/diff/4-5/

Comment 35

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/6-7/

Comment 36

[Code Review Bot [:reviewbot]]

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 37

[Code Review Bot [:reviewbot]]

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]

Comment 38

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - make log function safer so that tests do not time out;

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

Comment 39

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

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

Comment 40

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/7-8/

Comment 41

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

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

Comment 42

[Code Review Bot [:reviewbot]]

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 43

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/8-9/

Comment 44

[Yulia Startsev [:yulia]]

Comment on attachment 8994817 [details]
Bug 1473513 - make log function safer so that tests do not time out;

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

Comment 45

[Yulia Startsev [:yulia]]

Comment on attachment 8994818 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

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

Comment 46

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

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

Comment 47

[Yulia Startsev [:yulia]]

try run with talos tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=609352227a1640a98dd01da96f854dde7a17b909

Comment 48

[Code Review Bot [:reviewbot]]

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 49

[Code Review Bot [:reviewbot]]

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 50

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

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

Comment 51

[Yulia Startsev [:yulia]]

hopefully last try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2ceb489e5f5fec04b93835ac2e707dee4a7f091

Comment 52

[Yulia Startsev [:yulia]]

talos tests are fudged: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92de1e60b4a4c1aebe59c0a126bb340298ddedc1&selectedJob=190046617

I am working on them

Comment 53

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259104/diff/4-5/

Comment 54

[Yulia Startsev [:yulia]]

new try run!: https://treeherder.mozilla.org/#/jobs?repo=try&revision=668654a51f65685699813d28c69baaa9915d1895

Comment 55

[Alexandre Poirot [:ochameau]]

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

Comment 56

[Alexandre Poirot [:ochameau]]

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"
```

Comment 57

[Alexandre Poirot [:ochameau]]

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 )

Comment 58

[Alexandre Poirot [:ochameau]]

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.

Comment 59

[Yulia Startsev [:yulia]]

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

Comment 60

[Yulia Startsev [:yulia]]

Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8c00770dc1a26717ad5fc9d600fb45dcdaf19f

Comment 61

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256022/diff/5-6/

Comment 62

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/9-10/

Comment 63

[Yulia Startsev [:yulia]]

Comment on attachment 8994817 [details]
Bug 1473513 - make log function safer so that tests do not time out;

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

Comment 64

[Yulia Startsev [:yulia]]

Comment on attachment 8994818 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

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

Comment 65

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259104/diff/5-6/

Comment 66

[Yulia Startsev [:yulia]]

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 67

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - refactor main.js to use protocol.js pools;

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

Comment 68

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - reduce number of poolFor calls;

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

Comment 69

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256022/diff/6-7/

Comment 70

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/10-11/

Comment 71

[Yulia Startsev [:yulia]]

Comment on attachment 8994817 [details]
Bug 1473513 - make log function safer so that tests do not time out;

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

Comment 72

[Yulia Startsev [:yulia]]

Comment on attachment 8994818 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

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

Comment 73

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259104/diff/6-7/

Comment 74

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57274456b54ee1b6eef38bd80212e61912406bfe

Comment 75

[Alexandre Poirot [:ochameau]]

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

Comment 76

[Alexandre Poirot [:ochameau]]

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?

Comment 77

[Yulia Startsev [:yulia]]

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.

Comment 78

[Yulia Startsev [:yulia]]

Comment on attachment 8991026 [details]
Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256022/diff/7-8/

Comment 79

[Yulia Startsev [:yulia]]

Comment on attachment 8991344 [details]
Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256246/diff/11-12/

Comment 80

[Yulia Startsev [:yulia]]

Comment on attachment 8994817 [details]
Bug 1473513 - make log function safer so that tests do not time out;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259356/diff/4-5/

Comment 81

[Yulia Startsev [:yulia]]

Comment on attachment 8994818 [details]
Bug 1473513 - remove other instances of ActorPool from browsingContext and root;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259358/diff/4-5/

Comment 82

[Yulia Startsev [:yulia]]

Comment on attachment 8994555 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259104/diff/7-8/

Comment 83

[Yulia Startsev [:yulia]]

Comment on attachment 9002742 [details]
Bug 1473513 - refactor main.js to use protocol.js pools;

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

Comment 84

[Yulia Startsev [:yulia]]

Comment on attachment 9002743 [details]
Bug 1473513 - reduce number of poolFor calls;

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

Comment 85

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a63020f398a56275a4bcb76fd524da1c3cfb2514

Comment 86

[Pulsebot]

Pushed by [e-mail]:
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

Comment 87

[Arthur Iakab [arthur_iakab]]

Backed out 7 changesets (bug 1473513) for failing devtools e.g. leakcheck | default process: 1618727 bytes leaked 
Backout revision https://hg.mozilla.org/integration/autoland/rev/98b5ff9533ee519674b8c441083470d91e1080f8
Failed push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=deb8812556ef956417c81f6db7df15ff04dbbe9f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195298698&repo=autoland

Comment 88

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - use Protocol.js pools for workerTargetActorPool in Target Actors; r=ochameau

MozReview-Commit-ID: 5uIWwOR7CHp

Comment 89

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor; r=ochameau

create LazyActorClass based off ObservedActorFactory and RegisterdFactory classes for use in RootActor and BrowsingContextActor;

Depends on D6468

Comment 90

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - make log function safer so that tests do not time out; r=ochameau

MozReview-Commit-ID: BIk5pWzxJcx

Depends on D6470

Comment 91

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - remove other instances of ActorPool from browsingContext and root; r=ochameau

MozReview-Commit-ID: GxkLzvxJgdY

Depends on D6471

Comment 92

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - Ensure that actorPools in root are destroyed on destroy; r=ochameau

Depends on D6472

Comment 93

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - separate registerModule behavior from DebuggerServer; r=ochameau

MozReview-Commit-ID: 3GsXRxcIKfx

Depends on D6473

Comment 94

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - refactor main.js to use protocol.js pools; r=ochameau

MozReview-Commit-ID: FNMK4f553yI

Depends on D6474

Comment 95

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - reduce number of poolFor calls; r=ochameau

MozReview-Commit-ID: 9VqKPauAP9j

Depends on D6475

Comment 96

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - resolve memory leak

Depends on D6476

Comment 97

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - move ActorRegistry module to server/actors/utils; r=ochameau

Depends on D6477

Comment 98

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfcd707e790aa28e24e4e0a61742792db2582e50

Comment 99

[Alexandre Poirot [:ochameau]]

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.

Comment 100

[Alexandre Poirot [:ochameau]]

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.

Comment 101

[Alexandre Poirot [:ochameau]]

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.

Comment 102

[Alexandre Poirot [:ochameau]]

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.

Comment 103

[Alexandre Poirot [:ochameau]]

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.

Comment 104

[Alexandre Poirot [:ochameau]]

Comment on attachment 9010871 [details]
Bug 1473513 - separate registerModule behavior from DebuggerServer; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.

Comment 105

[Alexandre Poirot [:ochameau]]

Comment on attachment 9010872 [details]
Bug 1473513 - refactor main.js to use protocol.js pools; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.

Comment 106

[Alexandre Poirot [:ochameau]]

Comment on attachment 9010873 [details]
Bug 1473513 - reduce number of poolFor calls; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.

Comment 107

[Alexandre Poirot [:ochameau]]

Comment on attachment 9010874 [details]
Bug 1473513 - resolve memory leak

Alexandre Poirot [:ochameau] has approved the revision.

Comment 108

[Alexandre Poirot [:ochameau]]

Comment on attachment 9010888 [details]
Bug 1473513 - move ActorRegistry module to server/actors/utils; r=ochameau

Alexandre Poirot [:ochameau] has approved the revision.

Comment 109

[Pulsebot]

Pushed by [e-mail]:
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

Comment 110

[Yulia Startsev [:yulia]]

Attachment: Bug 1473513 - fix faulty path in unit tests;

Depends on D6479

Comment 111

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/932554641182
fix faulty path in unit tests. r=fix. CLOSED TREE

Comment 112

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Fix merge conflict between bug 1473513 and 1485676 against this test. CLOSED TREE

MozReview-Commit-ID: KyizI0V5pt5

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/950ce58bfb24
Fix merge conflict between bug 1473513 and 1485676 against this test. CLOSED TREE

Comment 114

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 950ce58bfb24 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 1HAisV9S6ls

Comment 115

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 174fe98999ea for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: GfSX2wYfLrN

Depends on D6803

Comment 116

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 80c51a6e896e for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 3TCiKetO6fn

Depends on D6804

Comment 117

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset db43912632fb for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: DbUwQ0NQZcE

Depends on D6805

Comment 118

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset f9f12b44a3ab for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 8POm9seFrAk

Depends on D6806

Comment 119

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset d72f6ff37ca6 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: LKPmFrhaQ5q

Depends on D6807

Comment 120

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset c690d2383ca4 for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: KSgxXnGdsJk

Depends on D6808

Comment 121

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset e56c70bd2f9a for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 9FIL35Nq726

Depends on D6809

Comment 122

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 6fc241b06e0b for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 6dCPXvBYhsJ

Depends on D6810

Comment 123

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 27419719402e for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 43tUMmC1Je0

Depends on D6811

Comment 124

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473513 - Backed out changeset 0e431edddacd for failures on mobile/android/tests/browser/chrome/test_debugger_server.html CLOSED TREE

MozReview-Commit-ID: 1BlQk4H0c1w

Depends on D6812

Comment 125

[Pulsebot]

Pushed by [e-mail]:
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

Comment 126

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0e431edddacd
https://hg.mozilla.org/mozilla-central/rev/27419719402e
https://hg.mozilla.org/mozilla-central/rev/6fc241b06e0b
https://hg.mozilla.org/mozilla-central/rev/e56c70bd2f9a
https://hg.mozilla.org/mozilla-central/rev/c690d2383ca4
https://hg.mozilla.org/mozilla-central/rev/d72f6ff37ca6
https://hg.mozilla.org/mozilla-central/rev/f9f12b44a3ab
https://hg.mozilla.org/mozilla-central/rev/db43912632fb
https://hg.mozilla.org/mozilla-central/rev/80c51a6e896e
https://hg.mozilla.org/mozilla-central/rev/174fe98999ea
https://hg.mozilla.org/mozilla-central/rev/932554641182
https://hg.mozilla.org/mozilla-central/rev/950ce58bfb24
https://hg.mozilla.org/mozilla-central/rev/e2f0d061af12
https://hg.mozilla.org/mozilla-central/rev/4df71e54cd06
https://hg.mozilla.org/mozilla-central/rev/2b36f3c32272
https://hg.mozilla.org/mozilla-central/rev/794c7e4ab8f8
https://hg.mozilla.org/mozilla-central/rev/cf844f66a6a9
https://hg.mozilla.org/mozilla-central/rev/1d1aac7f0245
https://hg.mozilla.org/mozilla-central/rev/44a4558db6fe
https://hg.mozilla.org/mozilla-central/rev/4981a0d3ed1c
https://hg.mozilla.org/mozilla-central/rev/bef0af13e60e
https://hg.mozilla.org/mozilla-central/rev/f4613b86e353
https://hg.mozilla.org/mozilla-central/rev/a2df10c9e819

Comment 127

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 128

[Mike Conley (:mconley) (:⚙️)]

I apologize - this looks like only a partial backout. I'll close this again.

Comment 129

[Yulia Startsev [:yulia]]

it was fully backed out. sorry for not getting to this sooner. I have a fix

Comment 130

[Alexandre Poirot [:ochameau]]

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...

Comment 131

[Pulsebot]

Pushed by [e-mail]:
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;

Comment 132

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0187f382339c
https://hg.mozilla.org/mozilla-central/rev/3612d32de44c
https://hg.mozilla.org/mozilla-central/rev/6dab963c9f14
https://hg.mozilla.org/mozilla-central/rev/f8e02f7e5e2d
https://hg.mozilla.org/mozilla-central/rev/f81e08c372b5
https://hg.mozilla.org/mozilla-central/rev/4c36b65f23c2
https://hg.mozilla.org/mozilla-central/rev/04e64ca2ad0f
https://hg.mozilla.org/mozilla-central/rev/479aed564448
https://hg.mozilla.org/mozilla-central/rev/ee1f0957ef4d
https://hg.mozilla.org/mozilla-central/rev/6436d45c7bd3
https://hg.mozilla.org/mozilla-central/rev/48445cd6f7c5