795263 - Actor names should not change when promoted to thread-lifetime

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jim Blandy :jimb]

At present, sending a pause-lifetime actor a "threadGrip" message elicits a reply with a new name for the thread-lifetime actor. This means that there are now two actor names for the same underlying thing.

Firebug and our developer tools would like to be able to correlate objects appearing in different panels. For example, if the debugger comes across a DOM element that is also displayed in the markup panel or the highlighter, it would like to link those things together. Having distinct pause-lifetime and thread-lifetime actor names for the same object makes it more difficult to implement this behavior: in theory, one would need to promote every pause-lifetime grip one encounters to a thread-lifetime grip just to see if it's the same as some other thread-lifetime grip one has already acquired.

Comment 1

[Jim Blandy :jimb]

Ach, forgot

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

This patch works, as can be verified by the passing unit tests. We don't have any use for this functionality currently in the Firefox debugger, but Firebug needs it. I also fixed a bug with pool.removeActor, which in some cases was passed an actor and in others an actor ID. I opted for an actor parameter, for aesthetic reasons.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=24bb76c8e949

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Oh, and a minor protocol change: when a |releaseMany| packet is received with pause-lifetime actors, I changed the response to include an error message, instead of simply ignoring that actor. Does that sound OK Jim?

Comment 4

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #3)
> Oh, and a minor protocol change: when a |releaseMany| packet is received
> with pause-lifetime actors, I changed the response to include an error
> message, instead of simply ignoring that actor. Does that sound OK Jim?

Yes, that sounds fine. Does it still release all the thread-lifetime actors, though? We need to leave the server in a well-known state.

Comment 5

[Jim Blandy :jimb]

Could we drop the "threadGrip" property from the successful reply, making it simply:

  { "from":<i>gripActor</i> }

?  Returning a grip suggests that there's some information to convey there, but there isn't.

Comment 6

[Jim Blandy :jimb]

Does this look okay?

https://github.com/jimblandy/DebuggerDocs/compare/thread-lifetime-actor-names

(sorry about the long lines)

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #4)
> (In reply to Panos Astithas [:past] from comment #3)
> > Oh, and a minor protocol change: when a |releaseMany| packet is received
> > with pause-lifetime actors, I changed the response to include an error
> > message, instead of simply ignoring that actor. Does that sound OK Jim?
> 
> Yes, that sounds fine. Does it still release all the thread-lifetime actors,
> though? We need to leave the server in a well-known state.

Yes, of course.

(In reply to Jim Blandy :jimb from comment #5)
> Could we drop the "threadGrip" property from the successful reply, making it
> simply:
> 
>   { "from":<i>gripActor</i> }
> 
> ?  Returning a grip suggests that there's some information to convey there,
> but there isn't.

Yes, that makes a lot of sense.

(In reply to Jim Blandy :jimb from comment #6)
> Does this look okay?
> 
> https://github.com/jimblandy/DebuggerDocs/compare/thread-lifetime-actor-names
> 
> (sorry about the long lines)

Looks good, thanks.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

Updated per Jim's comments. I repurposed test_threadlifetime.js that didn't make sense any more, into a test for the error response when releasing pause-lifetime actors along with thread-lifetime ones. I also added a threadClient.releaseMany() method that was missing from the client library.

Comment 9

[Jim Blandy :jimb]

Awesome --- let me know when this lands, and I'll update the docs on the wiki.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 670301 [details] [diff] [review]
Patch v2

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +383,5 @@
> +      if (actor) {
> +        actor.onRelease();
> +      } else {
> +        res = { error: "notReleasable",
> +                message: "Only thread-lifetime actors can be released." };

slightly funny logic here. Looks like you're doing an existence check and then returning an error because the actor isn't thread-lifetime. It's only because it isn't in that particular pool that this works.

I might check if !actor res = { error: ... and then continue; my for loop and do the actor.onRelease() outside of the condition.

Not sure if that would make this a little more readable or not.

You're also setting res potentially more than once with the same literal contents. You could check if res is null and then set it. Minor quibble.

@@ +889,5 @@
> +    // We want to reuse the existing actor ID, so we just remove it from the
> +    // current pool's weak map and then let pool.addActor do the rest.
> +    aActor.registeredPool.objectActors.delete(aActor.obj);
> +    this.threadLifetimePool.addActor(aActor);
> +    this.threadLifetimePool.objectActors.set(aActor.obj, aActor);

weakmap, eh?

We're ok with objects vanishing from this collection. In fact, I think it makes sense for them to do so.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 670301 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 670301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +383,5 @@
> > +      if (actor) {
> > +        actor.onRelease();
> > +      } else {
> > +        res = { error: "notReleasable",
> > +                message: "Only thread-lifetime actors can be released." };
> 
> slightly funny logic here. Looks like you're doing an existence check and
> then returning an error because the actor isn't thread-lifetime. It's only
> because it isn't in that particular pool that this works.
> 
> I might check if !actor res = { error: ... and then continue; my for loop
> and do the actor.onRelease() outside of the condition.
> 
> Not sure if that would make this a little more readable or not.

OK, done.

> You're also setting res potentially more than once with the same literal
> contents. You could check if res is null and then set it. Minor quibble.

I opted for readability instead of performance, because we don't really care if the first or last error is returned, since they are the same. But I'm all for tiny speed boosts, so I made the change.

> @@ +889,5 @@
> > +    // We want to reuse the existing actor ID, so we just remove it from the
> > +    // current pool's weak map and then let pool.addActor do the rest.
> > +    aActor.registeredPool.objectActors.delete(aActor.obj);
> > +    this.threadLifetimePool.addActor(aActor);
> > +    this.threadLifetimePool.objectActors.set(aActor.obj, aActor);
> 
> weakmap, eh?
> 
> We're ok with objects vanishing from this collection. In fact, I think it
> makes sense for them to do so.

Yes, we're mostly concerned about cycles in the actor graph preventing GC and a weakmap is what the doctor prescribed.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/a8f4069538a6

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a8f4069538a6

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #9)
> Awesome --- let me know when this lands, and I'll update the docs on the
> wiki.

Doc update ping.