992244 - browser_dbg_addonactor.js isn't disconnecting the AddonThreadActor properly

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Dave Townsend [:mossop]]

Which leaves a debugger alive in chrome which can't be good

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

I've verified that this works manually, not sure of a way to test it though.

Comment 2

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

Comment on attachment 8402077 [details] [diff] [review]
patch

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

Interesting, I wonder if the debugger remaining active is responsible for some of the extra time it takes for running devtools mochitests. I see how this will work, but I think there is a broader problem here that we need to fix, as it should have been removeActorPool's duty to cleanup child actors. Let me attach a patch to show you what I mean.

Comment 3

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

Attachment: Disconnect actors by default when removing an actor pool

This is what I had in mind. AFAIK every use of removeActorPool was made under the impression that actor.disconnect() would be called for every actor in the pool, but it turns out that you need to specify an extra parameter that I bet almost nobody paid attention to. To cater to the common case removeActorPool should cleanup actor children by default, unless there is a particular reason not to.

The only case I've found during testing is protocol.js, which sort of deals with a lot of these things on its own, but I bet I could remove even that with some rearranging of pool.destroy() and pool.cleanup(). In that case we would simplify removeActorPool and get rid of its second argument.

Dave T., can you verify that this patch fixes the AddonThreadActor's problem?
Dave C., can you think of any reason for not wanting to disconnect actors in all cases?

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

Comment 4

[Dave Townsend [:mossop]]

(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 8402077 [details] [diff] [review]
> patch
> 
> Review of attachment 8402077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Interesting, I wonder if the debugger remaining active is responsible for
> some of the extra time it takes for running devtools mochitests.

I was a little hopeful of that myself, but the length of the browser-chrome runs with this patch didn't differ from the normal length  so I think not.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8402648 [details] [diff] [review]
Disconnect actors by default when removing an actor pool

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

Yeah this fixes it. I did wonder when I read that comment in webbrowser.js

Comment 6

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

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

Comment 7

[Carsten Book [:Tomcat]]

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