Hey Gijs, within Bug 1670244 we want to ensure we never load a document with incorrect principal in the wrong type of webIsolated process. To comply with that rule we have to update certain callsites. One of those callistes is for example is the loading of the fallback window of chrome://devtools/content/shared/webextension-fallback.html within webextension.js.

Ultimately we end up within WindowGlobalActor::WindowInitializer where we do init.principal() = aWindow->GetPrincipal();.

I assume the easiest fix is to update the code in webextension.js and rather create a new window?? than falling back to the chomeGlobal this.fallbackWindow = this.chromeGlobal.content; ?

What's your take?

On test that illustrates the problem is:
./mach test devtools/server/tests/xpcshell/test_extension_storage_actor.js

I don't know what the point of webextension-fallback.html is and/or why we need it and/or what the correct principal is. I'm hoping Luca can provide some context.

If there is no use in having it, maybe we can remove it alltogether.

Hey Christoph,
I can just apply the patch attached to Bug 1670244 locally and then run the test file test_extension_storage_actor.js to be able to dig in a bit more detail into that check from WindowGlobalActor::WindowInitializer that you did mention in comment 0, is that right?

(In reply to :Gijs (he/him) from comment #2)

I don't know what the point of webextension-fallback.html is and/or why we need it and/or what the correct principal is. I'm hoping Luca can provide some context.

That fallback page is being used by the addon debugging toolbox when there is not extension page to attach to, which can happen in a couple of expected cases:

• the extension is being reloaded while the addon debugging toolbox was already attached
  • while the extension is being reloaded, there will be no extension page to attach and the devtools toolbox would break if we would switch it to another target while the extension is being restarted.
  • this allows the extension developers to put a breakpoint in the background page on a line that is only being executed as part of the extension startup and then hit reload on the extension to make the debugger panel to pause on that breakpoint
• the extension doesn't have a background page:
  • e.g. the extension could only have a browserAction/pageAction popup window and no background page and in that case there will be an extension page to attach to only once the popup page is being opened
    (the user will also have to toggle the devtools checkbox that prevents the popup windows to be automatically destroyed when they lose focus)

An additional note in case it may be useful to evaluate what would be the right way to handle it:

• the fallback page is never loaded in any webIsolated child process, it is only being used in the extension child process (as the webextension devtools target), or in the main process for those builds where the extension child process isn't yet supported (which at the moment does still include Fenix as a tear 1, and Thunderbird as a Tier 2).

Ideally, and our our reasoning is that, no load from a webIsolated child process, or also extension child process is loaded using the SystemPrincipal. Let's re-evaluate if we can do better here. For debugging it's best to apply this patch:
https://hg.mozilla.org/try/rev/a58a7634b561d5d60ef375121cc10be67d209206 modulo the allowlisting of docURI->SchemeIs("chrome") to trigger the assertion.

FWIW, I have those two tests on the radar which exhibit the behavior:

./mach test devtools/server/tests/xpcshell/test_extension_storage_actor_upgrade.js
./mach test devtools/server/tests/xpcshell/test_extension_storage_actor.js

Thanks for your help!

Thanks Christoph,
I'm adding a needinfo assigned to myself to get back to this asap and give it a look locally in a bit more detail.

Hi Alex,
I would like to hear your perspective about this, and if you have some ideas (or some other comments or additional useful context from your perspective).

do I recall correctly that during one of our recent chats you did briefly mention that (thanks to some of the fission related changes applied to or planned for the DevTools internals) we may be able to keep the Addon Debugging's DevTools toolbox open while reloading the extension without having to use this crude hack (the one based on navigating to a fallingback page url)?

If that is the case, then we may be able to cover at least the first one from list of scenarios that I did mention in comment 4 (reloading the extension while the toolbox is open, e.g. to be able to trigger breakpoints that would only be hit during the extension startup) , and we can evaluate how we may handle the second one (opening the toolbox on an extension with no background page open, e.g. an extension with only a sidebar or popup panel extension pages).

(not clearing my needinfo yet on purpose for now, because I still want to come back to this and take a more deeper look).

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #7)

do I recall correctly that during one of our recent chats you did briefly mention that (thanks to some of the fission related changes applied to or planned for the DevTools internals) we may be able to keep the Addon Debugging's DevTools toolbox open while reloading the extension without having to use this crude hack (the one based on navigating to a fallingback page url)?

Yes, and I'm looking forward removing this !
Bug 1675456 is probably a good step forward.
On the first hand, that bug about making webext being supported by the Watcher Actor is made complex because of this crude hack.
On the other hand, we can't imagine getting rid of the crude hack without having webext support in the watcher actor...
Chicken...egg... It means that we probably have to pile up yet another set of temporary hack just to be able to move forward.

Once we support the watcher actor, it means that it should be easy to support extension reload and picking up any new browser element.
Having said that, we might need bug 1698891, or some of its dependencies. But we are tentatively trying to land that for M7a and are actively working on that.

Do you know how you can identify that a browser element is part of a given addon?
Any useful flag on BrowsingContext? The watcher actor uses browsingContext.browserId, but I imagine webext may have many browser elements with distinct browserId's?

If that is the case, then we may be able to cover at least the first one from list of scenarios that I did mention in comment 4 (reloading the extension while the toolbox is open, e.g. to be able to trigger breakpoints that would only be hit during the extension startup) , and we can evaluate how we may handle the second one (opening the toolbox on an extension with no background page open, e.g. an extension with only a sidebar or popup panel extension pages).

And yes, this would still be an issue. Our famous watcher actor won't help much here.
DevTools expects to have at least one target front and for that we would need at least one debugable context available to spawn a target actor for it. For example, the webconsole always expects to have one context into which to evaluate JS. It can be anything, but we need something.
Can we force spawning the background page or something?

I will try to move forward with bug 1675456 and things would be cleared after that lands.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #4)

That fallback page is being used by the addon debugging toolbox when there is not extension page to attach to, which can happen in a couple of expected cases:

• the extension doesn't have a background page:
  • e.g. the extension could only have a browserAction/pageAction popup window and no background page and in that case there will be an extension page to attach to only once the popup page is being opened
    (the user will also have to toggle the devtools checkbox that prevents the popup windows to be automatically destroyed when they lose focus)

About that... I'm wondering what chrome is doing?
Does DevTools starts blank? Or is it opening only once the first document opens?

Thanks a lot Alex for the detailed answer, follows inline some answers to your questions.

(In reply to Alexandre Poirot [:ochameau] from comment #8)

...
Do you know how you can identify that a browser element is part of a given addon?
Any useful flag on BrowsingContext? The watcher actor uses browsingContext.browserId, but I imagine webext may have many browser elements with distinct browserId's?

Yes, an extension may definitely have different browser elements related to it, and so I assume distinct browserContext.browserId.

We may double-check that with Tomislav (:zombie), e.g. I see that we are using an WebExtensionPolicy property named browsingContextGroupId here in ExtensionPopups.jsm to make sure that we create browser elements related to the same extension, that may be what we are looking for.

If that is the case, then we may be able to cover at least the first one from list of scenarios that I did mention in comment 4 (reloading the extension while the toolbox is open, e.g. to be able to trigger breakpoints that would only be hit during the extension startup) , and we can evaluate how we may handle the second one (opening the toolbox on an extension with no background page open, e.g. an extension with only a sidebar or popup panel extension pages).

And yes, this would still be an issue. Our famous watcher actor won't help much here.
DevTools expects to have at least one target front and for that we would need at least one debugable context available to spawn a target actor for it. For example, the webconsole always expects to have one context into which to evaluate JS. It can be anything, but we need something.
Can we force spawning the background page or something?

I will try to move forward with bug 1675456 and things would be cleared after that lands.

once we have solved the issue with the addon being reloaded without the fallback page, then for this remaining scenario:

• the addon will already be running
• its WebExtensionPolicy instance will be active
• and an extension uuid (used as origin) will already be available for that extension

and so yes, I think that we may look into opting to load an empty background page (by loading moz-extension://uuid/_generated_background_page.html, which is the empty page that we use when the manifest.json file does only specify background scripts, or we may add a new one if we want to include a message like the one in the current fallback page).

But (as a side note) we should also double-check if that would trigger some other side effect that we would need to take care of (and we may prefer to use a separate page instead of the existing _generated_background_page.html if that is the case).

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

(In reply to Alexandre Poirot [:ochameau] from comment #8)

...
Do you know how you can identify that a browser element is part of a given addon?
Any useful flag on BrowsingContext? The watcher actor uses browsingContext.browserId, but I imagine webext may have many browser elements with distinct browserId's?

Yes, an extension may definitely have different browser elements related to it, and so I assume distinct browserContext.browserId.

We may double-check that with Tomislav (:zombie), e.g. I see that we are using an WebExtensionPolicy property named browsingContextGroupId here in ExtensionPopups.jsm to make sure that we create browser elements related to the same extension, that may be what we are looking for.

zombie, Could you confirm if using browsingContextGroupId is a good way to identify all the BrowsingContext's related to one precise Web Extension ?

DevTools code would now use JSWindow Actors to track WebExt browser elements.
We would need to filter out only the one related to the one add-on we are debugging.
Luca mentioned the browsingContextGroupId. Is that a good or the best option ?
DevTools code is typically filtering by browserId, but that may only work well for tab debugging...
Note that the current DevTools code related to WebExt (still using message managers) filters via documentPrincipal.addonId. I imagine we could use browsingContext.currentWindowGlobal.documentPrincipal.addonId. Is that a better option compared to browsingContextGroupId?

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

and so yes, I think that we may look into opting to load an empty background page (by loading moz-extension://uuid/_generated_background_page.html, which is the empty page that we use when the manifest.json file does only specify background scripts, or we may add a new one if we want to include a message like the one in the current fallback page).

Do you think we could easily write a POC for that? Because I'm wondering if that could somehow help bug 1675456...
(please avoid trying to write a POC if that's not trivial)

(In reply to Alexandre Poirot [:ochameau] from comment #11)

zombie, Could you confirm if using browsingContextGroupId is a good way to identify all the BrowsingContext's related to one precise Web Extension ?
[...]
Note that the current DevTools code related to WebExt (still using message managers) filters via documentPrincipal.addonId. I imagine we could use browsingContext.currentWindowGlobal.documentPrincipal.addonId. Is that a better option compared to browsingContextGroupId?

I don't think browsingContextGroupId maps exactly to what you were using previously:

• First, all browsing context in one tab (or popup, or...) will be in the same BCGroup, even iframes with an unrelated principal.
• Second, a moz-extension:// iframe inside another web page will not be in corresponding extension's BCGroup, though not sure devtools care about those iframes in the same way (currently, they get only the content script APIs, because before fission they were in the content process, but after fission, we might wanna change that and give them full extension API).

(caveat: I've read all comments in this bug, but I only understand about half of the things mentioned :)

Christoph, should this bug block shipping Fission? This bug blocks Extension DevTools meta bug 1587848, which we're currently tracking for Fission Milestone M8.

(In reply to Chris Peterson [:cpeterson] from comment #14)

Christoph, should this bug block shipping Fission? This bug blocks Extension DevTools meta bug 1587848, which we're currently tracking for Fission Milestone M8.

No, no need to block fission, it's a bug we should fix though, but no need to block on that.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)

No, no need to block fission, it's a bug we should fix though, but no need to block on that.

Thanks. In that case, I'll move this bug out of our Fission MVP bug list.

(In reply to Alexandre Poirot [:ochameau] from comment #8)

Yes, and I'm looking forward removing this !
Bug 1675456 is probably a good step forward.

Hey Alexandre, is this something that would be easier now that you've fixed bug 1675456?

Yes, bug 1675456 got a closer to being able to revisit how devtools handle WebExtensions.
Unfortunately, in that bug, I wasn't able to do all I wanted to do.
We still only create a unique "DevTools target" (i.e. WebExtensionTargetActor).
This target is still instantiated by the client side via WebExtensionDescriptorActor.getTarget.
Whereas I had in mind to start spawning many targets from the server side.
But I could not do that. Because all WebExtension documents are running in the same process.
So that I was blocked on bug 1741808 (which is meant to be landed ASAP, a final patch is in review) and may be also bug 1731740 (which I think will follow shortly after we tested this new codepath a bit on Nightly).

Spawning many targets from the server side is also called "server side target switching" (SSTS).
This is important here as it will allow us to easily destroy all the previous targets and create new ones when an add-on is reloaded.
Doing this should help us revisit the problematic webextension-fallback.html document and eventually get rid of it?
But it would still be helpful to have some document in case the add-on has no background page. Not having any particular context to debug could be problematic for us. All the DevTools assume there is something we can actively debug.

I just opened bug 1754452, which should be the ultimate work to unblock this issue.