1408326 - Some object actors are never released

Status: NEW

(DevTools :: Console, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce: 
1. Open the console
2. Evaluate `console.log({a: [1]})` (the console api call is important, or we hit Bug 1408321)
3. Click the "clear console" button

Expected results:
All the object actors are cleared, since we don't need them anymore

Actual results:
There is an extra actor staying on the pool.

----

So here, in 2., 2 actors are created:
- the obvious one, to get the grip and send it to the console
- another one, for the nested array (created in http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/devtools/server/actors/object.js#541 )

The problem is that we only release the first actor in the console, so the second one stay around, which might cause memory issues.

Comment 1

[Isabel Rios[:isabel_rios]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/3296

Comment 2

[Isabel Rios[:isabel_rios]]

Sorry PR related to anothe bug, ignore previous comment

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1408326 - Add a parent/children relationship in the ObjectActor;

We were only getting the root actor of an object in this function.
But actors can be created in many other places than the root: in
properties, getter, setter, entries, symbols, … So we could have lots
of actors that were never released until the toolbox gets destroyed.
This patch fix this and add two tests to make sure we do retrieve those
actors and release them.

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

https://reviewboard.mozilla.org/r/189110/#review194456

Clearing for now as we wait for responses on https://groups.google.com/d/msg/mozilla.dev.developer-tools/5D-jTsWs3yc/PaOLC7TwAwAJ

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Brian, do you think it would worth it landing this no matter what we do on the server ?
The server-side work will take some time I think, and will impact other tools (reps, debugger, …). This patch is quite small and could be uplifted, and we could get rid of it when the proper server solution lands.
I don't feel strongly about this, just think it would be nice to at least "patch" the console for the time a good solution is found

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Note that I have a patch to deal with this server side (pushed to try and there's a bunch of failures https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffcdbf350414042af95f80bd630a06abfd49a90b ) which might be better suited than the patch here in review, and might not take that long to land.
so let's wait until the middle of next week to see if I can come up with the server side patch

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

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

Comment 8

[Brian Grinstead [:bgrins]]

I'm not sure I understand this well enough to review it. Maybe Nick can give it at least an initial look (the Object Actor part and new tests)?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

https://reviewboard.mozilla.org/r/189110/#review200544

Let me clarify my understanding of what is happening here.

1. The webconsole already releasing the object actor associated with each log message as the console is cleared or when new log messages push old log messages out.
2. But that doesn't release the various iterator/enumerator actors for the released object actor
3. Therefore, leaking actors

If that understanding is correct, then I have some questions and thoughts:

In (1) is it always guaranteed that a log message is associated with at most one object actor? What about something like `console.table` that might contain many objects within it? If the webconsole is already keeping track of all of these actors that appear within each log message and is releasing them properly, then why can't it also track the various iterator/enumerator/etc actors? It seems that there should be a pool for each log message, that the client can explicitly release as the log messages disappear from screen.

Alternatively, if object actors used by the console were numbered monotonically, then we could have a "release all actors numbered <= X" operation, and it would be easy to ensure that the client and server stay in sync and don't leak, even if we had a hiccup between then and now. When clearing the whole console, then X=latest actor's number. When messages get pushed off the end of the log, then X=the last log messages smallest actor number.

Overall, my take on this patch is that it feels like a bandaid: it happens to fix this one leak, but doesn't address the underlying issue, which is the webconsole client and server not being properly synced up on what actors are still relevant at any given moment.

::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:102
(Diff revision 2)
>        }
>      }],
>      createObjectClient: object =>
>        new ObjectClient(serviceContainer.hudProxy.client, object),
>      releaseActor: actor => {
> -      if (!actor || !serviceContainer.hudProxy.releaseActor) {
> +      if (true || !actor || !serviceContainer.hudProxy.releaseActor) {

I don't know anything about this particular component, and can't comment on the logic changes' correctness.

However, `if (true || ...)` is a tautology, and so there's no need for an `if`. The code following this `return` should be deleted, rather than left as dead, unreachable code.

::: devtools/server/actors/object.js:68
(Diff revision 2)
>      incrementGripDepth,
>      decrementGripDepth,
>      getGlobalDebugObject
>    };
> -  this.iterators = new Set();
> +
> +  this.linkedActorsPool = new ActorPool(connection);

This pool *really* needs documentation:

* When is it intended to be used?
* When should it be used instead of the thread lifetime pool?
* When should it be used instead of the pause lifetime pool?

::: devtools/server/actors/script.js:1723
(Diff revision 2)
> +
> +    if (this.threadLifetimePool.objectActors.has(value)) {
>        return this.threadLifetimePool.objectActors.get(value).grip();
>      }
>  
>      let actor = new PauseScopedObjectActor(value, {

This is an extant issue that I just stumbled across when refreshing my memory about these things, but it is massively confusing that we create `PauseScopedObjectActor`s for any pool other than the pause lifetime pool...

::: devtools/server/actors/script.js:1750
(Diff revision 2)
>     * Create a grip for the given debuggee object with a pause lifetime.
>     *
>     * @param value Debugger.Object
>     *        The debuggee object value.
>     */
> -  pauseObjectGrip: function (value) {
> +  pauseObjectGrip: function (value, pool) {

This new parameter should be documented.

It isn't clear to me why one would pass a custom pool to `pauseObjectGrip` rather than calling `objectGrip` directly. It seems to me that the whole point of `pauseObjectGrip` is to remove the need for a `pool` parameter when one knows that they are using debuggee objects with a lifetime that matches the debugger's pause. If that isn't true, than it seems that using `objectGrip` directly is more appropriate.

::: devtools/server/actors/webconsole.js:433
(Diff revision 2)
> -   * @return object
> +   * @param ActorPool pool
> +   *        Optional ActorPool (falls back to this._actorPool)
> +   * @return Object
> +   *         ObjectActor.grip() result.
>     */
> -  createValueGrip: function (value) {
> +  createValueGrip: function (value, pool) {

Slightly nicer with default parameters:

    function (value, pool = this._actorPool) { ... }

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the review Nick. It was more like a f? , this patch isn't ready for review yet.


> Let me clarify my understanding of what is happening here.
> 
> 1. The webconsole already releasing the object actor associated with each log message as the console is cleared or when new log messages push old log messages out.
> 2. But that doesn't release the various iterator/enumerator actors for the released object actor
> 3. Therefore, leaking actors

Iterators are released too (through the `this.iterators` Set in ObjectActor). What is leaking is actors created for nested objects (e.g. evaluating `({foo: {value: 42})` creates an actor for the whole object, which will be released, and another one for the `foo` property, which currently won't be released in the new frontend).
I tried to have a consistent way of dealing with "linked" actors, hence the switch for iterators too.

> In (1) is it always guaranteed that a log message is associated with at most one object actor? What about something like `console.table` that might contain many objects within it? If the webconsole is already keeping track of all of these actors that appear within each log message and is releasing them properly, then why can't it also track the various iterator/enumerator/etc actors? It seems that there should be a pool for each log message, that the client can explicitly release as the log messages disappear from screen.

You're right, as said before, a message can contain multiple actors, either because the logged object is complex, or because we log multiple object (console.table, console.log(a,b,c), …).
The console could keep track of all the actors "displayed", but this is messy and error prone : actors IDs can be in different places in RDP grips, so you need to gather all of them. An initial patch for this bug was doing this (https://reviewboard.mozilla.org/r/189110/diff/1/) and it seemed we could find a better solution.

> Alternatively, if object actors used by the console were numbered monotonically, then we could have a "release all actors numbered <= X" operation, and it would be easy to ensure that the client and server stay in sync and don't leak, even if we had a hiccup between then and now. When clearing the whole console, then X=latest actor's number. When messages get pushed off the end of the log, then X=the last log messages smallest actor number.

This could be interesting indeed. It looks like we could trust the order of the actors on the server.

> Overall, my take on this patch is that it feels like a bandaid: it happens to fix this one leak, but doesn't address the underlying issue, which is the webconsole client and server not being properly synced up on what actors are still relevant at any given moment.

Agreed

> I don't know anything about this particular component, and can't comment on the logic changes' correctness.
> However, `if (true || ...)` is a tautology, and so there's no need for an `if`. The code following this `return` should be deleted, rather than left as dead, unreachable code.

Here only for quick local testing.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

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

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

This patch still needs a lot of tests to make sure everything is working fine, but I'd want to have your feedback on how this is going.
Basically, the ObjectActor is responsible of managing the actor it creates, which allow us to simplify the hooks a bit (we don't pass createValueGrip anymore).

On actor release, the object actor will destroy its pool which will release all the linked actors.

This introduced some changes in how we look for actors in the webconsole actor, since actors may now be in ObjectActor linkedActorsPool, but I don't think it adds too much complexity.

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

https://reviewboard.mozilla.org/r/189110/#review204836

I think it makes a lot of sense and the createValueGrip simplification is much welcomed!

Thanks for looking into such hairy topic.

::: devtools/server/actors/object.js:65
(Diff revision 3)
> +
>    this.hooks = {
> -    createValueGrip: createValueGripHook,
> +    createValueGrip: value => {
> +      if (this.linkedActorsMap.has(value)) {
> +        return this.linkedActorsMap.get(value).grip();
> +      }

About this Map, this is a bit unfortunate to have this Map *and* the ActorPool. But I imagine it is the only way to be really efficient right here.

But that makes me wonder... we don't have any such caching in existing code?
i.e. we always recreate a new ObjectActor, every time we access an object. That, even if we created it before?

Here, we are doing some caching, via this "linkedActorsMap". We should comment that this cache is per top-level ObjectActor. If two top-level ObjetActors access the same object, there will be two instances.

::: devtools/server/actors/object.js:78
(Diff revision 3)
> +        this.linkedActorsMap.set(v, actor);
> +        return actor.grip();
> +      };
> +
> +      return createValueGrip(value, this.linkedActorsPool, makeObjectGrip);
> +    },

The `createValueGrip` chain is very hard to follow. There is too many createValueGrip calling other createValueGrip.
Actually, your patch already simplifies it a bit, this is no longer imported from script.js and webconsole.js and promises.js. (you would need to do the same for environment.js).
With the new form of createValueGrip, I'm wondering if you could now merge this intermediate function into createValueGrip. (It may help to move it back to  ObjectActor.prototype)

If you do that, you could get rid of the 3rd makeObjectGrip parameter and inline it.

::: devtools/server/actors/object.js:118
(Diff revision 3)
> +      actor = poolActor.findLinkedActor(actorID, seen);
> +      return actor;
> +    });
> +
> +    return actor;
> +  },

This is interesting. This is where protocol.js may be better.
In protocol.js all actors are also implementing ActorPool interface and I imagine such "search actor in any children (pool)" would be more natural.
https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/shared/protocol.js#830-853
All actors have a "get" method that will look for direct children. But you could adapt "poolChildren" in order to implement this findLinkedActor.

So. What I say here is that you reimplement a procotol.js feature (`linkedActorPool` versus `__poolMap`). But linkedActorPool is very specific to ObjectActor whereas procotol.js make it generic to any actor.

If you are ready for it, you may try to study what it would cost to convert ObjectActor to protocol.js...
Note that you can keep the client as-is and only convert the actor codebase. Switching to protocol.js doesn't force changing RDP messages. See bug 1235374 for example.
If you happen to do that, please please please, do a changeset to only switch to protocol.js and avoid doing anything else.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8918267 [details]
Bug 1408326 - Add a parent/children relationship in the ObjectActor;

(and sorry for the feedback delay)

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p3 because no activity for at least 24 weeks.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information