1754452 - Instantiate WebExtension top level target from the server side -- support server side target switching for webext

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Alexandre Poirot [:ochameau]]

Bug 1675456 added the minimal support for the watcher actor to debug web extension.
The main goal was to support watching for resources on the server side so that we could drop legacy resource watcher.
But there is one thing left to complete the migration to JSWindowActor and embrace the full support of the watcher actor.
We should support "server side target switching".
It means that all targets should be spawn by the server, including the top level one. It means that the WebExtensionTarget should be spawn by the server instead of the client.
While doing that, we might also apply the "every frame target" pattern to web extension. Intead of having one magical target debugging many documents, we would spawn one target per document.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Spawn many target actor for WebExtension - EFT and server targets for WebExtensions.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 9271795 [details]
Bug 1754452 - [devtools] Spawn many target actor for WebExtension - EFT and server targets for WebExtensions.

This patch is actually only 50% of the work.
It enables "Server side target switching" (and may be also EFT?) for the TabDescriptor spawn by WebExt codebase.

But another patch is necessary also against the WebExtensionDescriptor, to address this other disable of server side target switching:
https://searchfox.org/mozilla-central/rev/6da1ebe13b260efabd88eb98dec5fa8ee65987b2/devtools/client/fronts/descriptors/webextension.js#99-115
(which also relates to EFT)

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Instantiate one target per WindowGlobal for Web Extensions documents.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Fix running about:debugging tests individually, clicking on addon's widgets.

We have to register them in the navbar, otherwise the clickOnAddonWidget helper will fail.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Stop relying on TargetMixin.isWebExtension.

Instead, query the simpler and more reliable descriptor front's isWebExtensionDescriptor attribute.

Simplify the logic around target's name attribute by making the server return the addon name right away.

TargetMixin.isWebExtension is kept in tree solely for backward compat reasons.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Remove now-unused WebExtensionTargetActor.

WebExtensionTargetActor.getTarget is being removed and removes most of the now-useless
codebase involving connectToFrame and message manager.

This allows to simplify a bit the ParentProcessTargetActor which required
some tweaks to be subclassed correctly.

We can also remove the consoleAPIListenerOptions which was specific to
this special target actor retrieving all document's console messages.
Now each individual target actor, for each document will handle the messages distinctly.

Note that we weren't using WebExtensionDescriptorFront.connect.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Select the hovered document when using the node picker on Web Extensions.

Before, we used to have a unique target, using internal frame switching.
Now that we have one real target actor per document, we should rather select
each newly hovered target at the TargetCommand level, so that the inspector's
markup view will switch to this document.

We are applying this logic only to Web Extensions as all targets in a tab
will be a children of the top level one, so that node picking's hovering
will always be able to select the hovered node in the markup view.
We may later extand this logic to the browser toolbox, once it supports one target per WindowGlobal.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Consider all resources with browsingContextID set to -1 as related to the top level target.

When we process resources from the parent process,
it is challenging to figure out what is the current top level window global.
In web extensions, the top level window global can change over time if there is a background page or not.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Use an immutable attribute instead of actorID to identify targets in the iframe dropdown.

With Web Extension, when the targets are destroyed, their front is destroyed
before being notified to the toolbox, which makes their actorID being null.
We should be using a safer attribute to uniquely identify them.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-eventpage.zip

This attached test extension is an MV2 extension that includes:

• an event page (which is the "non-persistent background page")
• a browserAction popup

The event page isn't subscribing any API event and so once terminated (e.g. on idle of by clicking the "Terminate background script" button from the about:debugging card) it is never going to be auto-respawned.

The browserAction popup includes a button, when the button is clicked the background page is being force re-spawned (through a call to browser.runtime.getBackgroundPage API method).

This should make it easier to manually test scenarios around the background page being terminated and respawned while the addon debugging toolbox is already connected.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-popup-and-nobg.zip

This attached test extension is an MV2 extension that includes a browserAction popup but no background page.

Comment 12

[Phabricator Automation]

Comment on attachment 9421524 [details]
Bug 1754452 - [devtools] Fix running about:debugging tests individually, clicking on addon's widgets.

Revision D220539 was moved to bug 1921946. Setting attachment 9421524 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9421525 [details]
Bug 1754452 - [devtools] Stop relying on TargetMixin.isWebExtension.

Revision D220540 was moved to bug 1921947. Setting attachment 9421525 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9421764 [details]
Bug 1754452 - [devtools] Use an immutable attribute instead of actorID to identify targets in the iframe dropdown.

Revision D220669 was moved to bug 1921948. Setting attachment 9421764 [details] to obsolete.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1754452 - [devtools] Automatically select any incoming Web Extension target when we are on the fallback document.

This helps mitigate the existence of the fallback document by automatically moving away from it
as soon as we have a meaningful content to debug.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: test-sidebar-and-nobg.zip

This attached test extension is an MV2 extension that includes:

• no background page
• a sidebar panel loading an extension page that is then creating two more sub frames:
  • the first subframe is going to load another extension page from the same extension
  • the second subframe is going to load a webpage (https://wikipedia.org, because it is one that load fine as is in a subframe)

This test extension is meant to help with testing corner cases related to reloading the addons with the addon debugging toolbox still open, in particular to reproduce the scenario where the first extension page loaded right away after the extension is reloaded isn't the background page (but the sidebar panel extension page).

STR for the issue "leaked targets on reloading an extension with an extension sidebar panel"

So far I managed to hit this issue only with an extension with a sidebar panel (even if I'm not sure yet how the sidebar panel contributes to be able to hitting the issue based on what it seems to be the underlying issue).

STR:

• start a Firefox instance with all the stack of patches attached to this bug applied
• install this test extension temporarily from about:debugging
• open the sidebar through the Firefox menu, pressing Alt-V and then clicking on View -> Sidebar -> test-sidebar-nobg
• open the addon debugging toolbox
• open the frame selector and confirm that "/sidebar.html" and "/subframe.html" are listed in the frame selector popup
• reload the addon while the addon debugging toolbox is still open
• open the frame selector again:
  • Actual behavior: the frame selector popup shows two times both "/sidebar.html" and "/subframe.html" (and that will increase further on each addon reload)
  • Expected behaviror: the frame selector popup should show only once the "/sidebar.html" and "/subframe.html" targets

The same exception can also be hit without the changes attached to this bugzilla issue, but in that case the side-effect of the issue surfaces differently, with the frame selector panel disappearing instead of the list frame selector popup to be growing because of the old and new target actors be all listed there, as in the STR described above.

In the browser console the following error is being logged right before this issue is being hit:

console.error: (new TypeError("can't access property \"name\", policy is null", "resource://devtools/server/actors/targets/window-global.js", 629))
TypeError: can't access property "name", policy is null: get title@resource://devtools/server/actors/targets/window-global.js:629:7
form@resource://devtools/server/actors/targets/window-global.js:751:7
destroyTargetActor@resource://devtools/server/connectors/js-process-actor/ContentProcessWatcherRegistry.sys.mjs:293:30
onNewTargetActor/<@resource://devtools/server/connectors/js-process-actor/ContentProcessWatcherRegistry.sys.mjs:233:37
newListener@resource://devtools/shared/event-emitter.js:169:27
_emit@resource://devtools/shared/event-emitter.js:242:32
emit@resource://devtools/shared/event-emitter.js:186:18
emit@resource://devtools/shared/event-emitter.js:330:18
destroy@resource://devtools/server/actors/targets/window-global.js:867:10
_onDocShellDestroy@resource://devtools/server/actors/targets/window-global.js:1143:14
observe@resource://devtools/server/actors/targets/window-global.js:1085:12

Preventing that exception from being hit (e.g. by just adding optional changing to this line) seems to prevent the issue as described in the STR from being hit, and so it seems to suggest that hitting this exception may be preventing us from removing the target actors successfully (and then entries for the old and new target actors to be piling up in the frame selector).

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef356d6def00
[devtools] Consider all resources with browsingContextID set to -1 as related to the top level target. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/e30c612a79b5
[devtools] Instantiate one target per WindowGlobal for Web Extensions documents. r=rpl,devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/517344243483
[devtools] Automatically select any incoming Web Extension target when we are on the fallback document. r=extension-reviewers,devtools-reviewers,willdurand,bomsy
https://hg.mozilla.org/integration/autoland/rev/e764e06e70f5
[devtools] Select the hovered document when using the node picker on Web Extensions. r=devtools-reviewers,nchevobbe

Comment 18

[Phabricator Automation]

Comment on attachment 9421526 [details]
Bug 1754452 - [devtools] Remove now-unused WebExtensionTargetActor.

Revision D220541 was moved to bug 1928510. Setting attachment 9421526 [details] to obsolete.

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ef356d6def00
https://hg.mozilla.org/mozilla-central/rev/e30c612a79b5
https://hg.mozilla.org/mozilla-central/rev/517344243483
https://hg.mozilla.org/mozilla-central/rev/e764e06e70f5