1652016 - Target registered from webextension aren't unregistered when toolbox closes

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Ensure you have devtools.testing.enableServerWatcherSupport set to true
2. Navigate to data:text/html,<meta charset=utf8><script>console.log(Date.now())</script>
3. Open the console, there should be a message
4. Close the toolbox
5. Reopen it: the message isn't there anymore
6. Reload the page: the message does not appear either

---

From what I can see in my debugging session, in DevToolsFrameChild, _watchResources calls targetActor.watchTargetResources on the wrong actor.

It looks like _getTargetActorForWatcherActorID calls the TargetActorRegistry to retrieve the target actor matching a given browserId.
But in TargetActorRegistry, it seems like we still have actors from the first toolbox opening, and since getTargetActor returns the first target that has the same browserId than the one that is passed, we get the wrong one.

(It also seem that for a same "toolbox session", we can have multiple targets in the registry that share the same browserId, which makes getTargetActor unreliable?)

I'll continue to dig into this.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c).

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Note that this is only happening when there are DevTools extensions installed.
In ExtensionParent.jsm, requesting the inspectedWindowFront will create a target for the tab (toolkit/components/extensions/ExtensionParent.jsm#678-681,715,720-722)

And this target doesn't appear to be destroyed when the DevTools toolbox is closed.

Julian, I think you're more familiar than me with this code, would you know what we're missing/doing wrong?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

(asking daisuke too, who worked on this area in the past)

note that I'm really not familiar with the extension code

Could we shutdown the context from browser/components/extensions/parent/ext-devtools.js#165-168, when we close the DevToolsPage, or does the context still need to be alive for some reason?

Comment 4

[Julian Descottes [:jdescottes]]

Let's also ask Luca :) I was wondering the same, why shutdown (which destroys the target) wouldn't be called when closing devtools.
https://searchfox.org/mozilla-central/rev/82c04b9cad5b98bdf682bd477f2b1e3071b004ad/toolkit/components/extensions/ExtensionParent.jsm#687-713

(keeping the ni? because I wanted to test that myself, haven't had time yet)

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

I think I fixed it, and I'm about to push a patch to review for that.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1652016 - Destroy target created by WebExtension when destroying the toolbox. r=rpl,daisuke,jdescottes.

The problem has 2 folds.
The first one is that the shitdown function on DevToolsExtensionPageContextParent
wasn't called, which means the target was never destroyed. This is fixed by
calling context.shutdown instead of context.unload in closeProxyContext. This should
be fine as shutdown ends up calling unload in the end (ProxyContextParent.shutdown
directly calls ProxyContextParent.unload).

The second issue was that a single webextension could create 2 targets, and since
we only keep track of a single target, we would miss one. This is fixed by putting
the call to watchTargets in getCurrentDevTools in a promise, so subsequent calls
that might occur before the resulting promise isn't resolved don't end up calling
watchTargets a second time.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

I think I fixed it, and I'm about to push a patch to review for that.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80317e4673b6
Destroy target created by WebExtension when destroying the toolbox. r=rpl,daisuke,jdescottes.

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/80317e4673b6