918695 - [app manager] Clicking on debug when a toolbox is already open should only raise the toolbox

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P2)

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Only instanciate one singleton target per app

To show the toolbox we call gDevTools.showToolbox with a target object.
This function ensure raising the existing toolbox, if a toolbox for the same target 
object has already been opened.
But the issue is that we were creating one new target object, each time we
called getTargetForApp...
getTargetForApp is calling TargetFactor.forRemote, that has a similar behavior...
but the object we are passing to it (`options`) is always different:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/app-actor-front.js#225
Even if the actor on the device returns always the same actor,
we end up with a new distinct res.actor object.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Ensure dispatching tabDetached event for ContentAppActor when the app is closed/killed

That device patch is necessary in order to close the toolbox once the app is closed/killed.
It is also really important with the other patch attached to this bug...
It allows to clean up the target we keep in the `exitingTargets` map.
Without this, the keep getting a dead target once we killed (and relaunch) an app,
until we close the toolbox manually (it closes the target).

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 819844 [details] [diff] [review]
Only instanciate one singleton target per app

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

Looks good to me!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 819846 [details] [diff] [review]
Ensure dispatching tabDetached event for ContentAppActor when the app is closed/killed

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

::: toolkit/devtools/server/actors/webapps.js
@@ +851,5 @@
>          }
> +        let actor = this._appActorsMap.get(mm);
> +        if (actor) {
> +          // The ContentAppActor within the child process doesn't necessary
> +          // have to time to uninitialize itself when the app is closed/killed.

Is there any notification, like "you're about to be killed", that the ContentAppActor could watch for instead of duplicating this here?

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> ::: toolkit/devtools/server/actors/webapps.js
> @@ +851,5 @@
> >          }
> > +        let actor = this._appActorsMap.get(mm);
> > +        if (actor) {
> > +          // The ContentAppActor within the child process doesn't necessary
> > +          // have to time to uninitialize itself when the app is closed/killed.
> 
> Is there any notification, like "you're about to be killed", that the
> ContentAppActor could watch for instead of duplicating this here?

May be. Actually the existing code doesn't even try to dispatch tabDetached. The BrowserTabActor listen for TabClose and we haven't tried to listen for another event.
But I'm afraid that the content process won't be able to do anything in case of OOM kill or crash. So at the end it looks like we have to ensure sending this event from the parent process.

Comment 6

[Ed Morley [:emorley]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/e8f2b30e478e
remote:   https://hg.mozilla.org/integration/fx-team/rev/de99454fac7f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8f2b30e478e
https://hg.mozilla.org/mozilla-central/rev/de99454fac7f