1009312 - Don't call client.addActorPool(this) twice in actor Front::initialize()

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jan Keromnes [:janx]]

Many actor fronts, in their `initialize` method, call `client.addActorPool(this)` followed by `this.manage(this)`.

However, `Pool.prototype.manage` accesses `this._poolMap`, triggering a getter function that calls `this.conn.addActorPool(this)` again: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/protocol.js#717,729

Comment 1

[Jan Keromnes [:janx]]

Attachment: Remove duplicate calls to addActorPool(). r=paul

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8421423 [details] [diff] [review]
Remove duplicate calls to addActorPool(). r=paul

Paul, please have a look.

Try: https://tbpl.mozilla.org/?tree=Try&rev=cce4180af9d1

Comment 3

[Paul Rouget [:paul]]

Comment on attachment 8421423 [details] [diff] [review]
Remove duplicate calls to addActorPool(). r=paul

dcamp or panos should review this.

Comment 4

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

Comment on attachment 8421423 [details] [diff] [review]
Remove duplicate calls to addActorPool(). r=paul

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

Hunh. I could swear that I've tried to omit that call in the memory actor and it hadn't worked, but it sure does now. Looks good to me, but let's get Dave to look at this too, as he is the one who had suggested the workaround in the first place.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 8421423 [details] [diff] [review]
Remove duplicate calls to addActorPool(). r=paul

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

::: toolkit/devtools/server/actors/gcli.js
@@ -201,5 @@
>      this.actorID = tabForm.gcliActor;
>  
>      // XXX: This is the first actor type in its hierarchy to use the protocol
>      // library, so we're going to self-own on the client side for now.
> -    client.addActorPool(this);

Please can we get rid of the comment above too. I think the same goes for inspector.js which looks suspiciously similar.

Comment 6

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

Note that the comment also applies to the manage() call below, which should go away when tab actor and the rest are converted to protocol.js (bug 698841).

Comment 7

[Jan Keromnes [:janx]]

I left these two identical comments because they also relate to the `this.manage(this)` line, but I'm not sure they're really that useful. Should I remove them?

Comment 8

[Joe Walker [:jwalker]]

(In reply to Jan Keromnes [:janx] from comment #7)
> I left these two identical comments because they also relate to the
> `this.manage(this)` line, but I'm not sure they're really that useful.
> Should I remove them?

I don't have strong feelings, but I don't suppose they're helping much.

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 8421423 [details] [diff] [review]
Remove duplicate calls to addActorPool(). r=paul

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

r+ if tests pass.

Comment 10

[Jan Keromnes [:janx]]

Attachment: Remove duplicate calls to addActorPool.

Comment 11

[Jan Keromnes [:janx]]

Comment on attachment 8449582 [details] [diff] [review]
Remove duplicate calls to addActorPool.

Rebased, carrying over dcamp's r+, try: https://tbpl.mozilla.org/?tree=Try&rev=24f158a95777

Comment 12

[Jan Keromnes [:janx]]

Looks green to me!

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jan Keromnes [:janx] from comment #11)
> Comment on attachment 8449582 [details] [diff] [review]
> Remove duplicate calls to addActorPool.
> 
> Rebased, carrying over dcamp's r+, try:
> https://tbpl.mozilla.org/?tree=Try&rev=24f158a95777

Please try to avoid "all" runs on Try if you can. They contribute to massive backlog and are often excessive.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 15

[Jan Keromnes [:janx]]

Noted, I tend to use `git push-to-try -t ../mozilla-central/ -b do -p all -u all -t none` to schedule try runs.

In the case of this patch, what try configuration would you recommend?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

See the link in comment 14. Your patch only touched devtools, so that immediately rules out test suites other than m-oth, xpcshell, and mochitest-dt and Android/B2G. You need to consider what platforms/test suites are actually going to be affected by the code you're touching.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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