1700739 - Make devtools.inspectedWindow.reload injectedScript and userAgent options Fission-compatible

Status: NEW

(WebExtensions :: Developer Tools, task, P3)

Description

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

I'm pretty sure it is not Fission compatible that the devtools.inspectedWindow.reload API method when used with the injectedScript option isn't currently Fission compatible.

The changes to make it compatible are going to be all on the devtools actor side, in particular this part in devtools/server/actors/addon/webextension-inspected-window.js is the one that would work with OOP frames.

The test case that covers that isn't skipped in Fission, but the iframes currently used in that test case are all from the same origin and so they do still pass with fission enabled:

• https://searchfox.org/mozilla-central/rev/368607c4cd5be547021945e4ae60e8eb4365b3c4/browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js#259
• https://searchfox.org/mozilla-central/rev/368607c4cd5be547021945e4ae60e8eb4365b3c4/browser/components/extensions/test/browser/file_inspectedwindow_reload_target.sjs#49-54

It may be also worth to double-check again the behavior on more recent Chrome builds, I'm not sure that when I did looked to the behavior of that option in Chrome they were already running iframe for different origin in different child processes (and so if Chrome does currently only run the injectedScript in same origin frames we may close this as wontfix for now if we prefer, because we would still be compatible with the behavior on Chrome).

Comment 1

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

I did set this as a task for now, because as I did mention in comment 0 I think that it may be worth to double-check the behavior on chrome first.

I added it as a blocker for the fission-webext meta, but I didn't added it as a blocker for Bug 1587848 (the test case is technically not failing but also not really covering the scenario that would break with fission enabled, if we will have to fix the API method to work with fission enabled then we will also have to update the test to make sure it does really cover the feature when fission is enabled).

As an additional side note, this injectedScript option was used by only one devtools extension at the time (I think it was one of the angular devtools extensions), most other extensions were using just content script to inject code into the loaded frames targeted by the devtools extension).

Comment 2

[Chris Peterson [:cpeterson]]

Tomislav says this bug doesn't need to block Fission MVP.

Comment 3

[Chris Peterson [:cpeterson]]

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

Tomislav says this bug doesn't need to block Fission MVP.

Correction: this bug was not in the list of bugs I asked Tomislav about. Waiting for his needinfo here.

Comment 4

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

I did notice that userAgent (another option supported by devtools.inspectedWindow.reload) is also going to be affected by the same issue described in comment 0 for injectedScript.

I've also briefly discussed about this with :ochameau from the DevTools team (in D108994), Alex provided some very helpful details about how we may adapt that part to make it fission compatible, which I'm quoting here to don't lose track of these initial pointers:

For userAgent it should be simplier, TargetConfigurationCommand will help you have such setting to be applied to the next document, even if a new BrowsingContext spawns:
https://searchfox.org/mozilla-central/source/devtools/shared/commands/target-configuration/target-configuration-command.js#50
For now, it doesn't support userAgent, but it easily can. This Command and related actor is meant to allow tuning any attribute of
BrowsingContext or DocShell.
Note that Nicolas will soon allow to toggle BrowsingContext attribute from the parent process:
https://bugzilla.mozilla.org/show_bug.cgi?id=1690100
For now, it only toggle them from the content processes.

Regarding injectedScript, I'm afraid you would have to use the same principles as TargetConfigurationActor and involved "watcher data entry", in > order to convey your script to the content process, before the page starts loading. You might look at the patch introducing
TargetConfigurationActor to see how that's done:
https://bugzilla.mozilla.org/show_bug.cgi?id=1690698

we can look into this once we verified what is the behavior on Chrome and assigned a priority accordingly (and ask for more details and some guidance from the DevTools team once we are actively working on this).

Comment 5

[Alexandre Poirot [:ochameau]]

userAgent may be fixed by bug 1706098.
injectedScript would still require dedicated fix highlighted in comment 4.

Comment 6

[Tomislav Jovanovic :zombie]

I tested in Chrome, and it's weird, the docs say it will be injected "in every iframe"
https://developer.chrome.com/docs/extensions/reference/devtools_inspectedWindow/

But as Luca guessed, it's only injected into same-origin iframes. This looks consistent with how tabs.executeScript works in Chrome, which is similarly under-documented.

I think I'd like to get this fixed to keep it consistent with non-fission Firefox, but can't really say it needs to be a blocker.

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Tomislav Jovanovic :zombie from comment #6)

I think I'd like to get this fixed to keep it consistent with non-fission Firefox, but can't really say it needs to be a blocker.

In that case, I will move this bug from Fission MVP to Future.

Comment 8

[Alexandre Poirot [:ochameau]]

Some refinement in the suggested implementation here.

I think that the reload codepath should be merged into the existing reload method we have in DevTools:
https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/devtools/shared/commands/target/target-command.js#789
Or, follow the same implementation, with having a method on the parent process which does the reload:

await this.descriptorFront.reloadDescriptor({ bypassCache });

will call the following code in the server:
https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/devtools/server/actors/descriptors/process.js#172

That, instead of currently doing the reload from the content process:
https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/devtools/server/actors/addon/webextension-inspected-window.js#391

Doing the reload from the parent process is now important in DevTools because the Target Actor is now going to be instantiated for each WindowGlobal. Meaning that the WebExtensionInspectedWindowActor will be instantiated once for each WindowGlobal. Meaning its reload request is doomed to fail as reloading the WindowGlobal will destroy itself and create a new instance, associated with the next WindowGlobal.

It should be easily to implement reload with ignoreCache and userAgent.
But injectedScript would require something special.
As suggested in comment 4, it should be passed as a watcher data entry.

More concretely, DescriptorActor.reloadDescriptor, should probable pass the injected script, like how the TargetConfigurationActor pass its config, like this:
https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/devtools/server/actors/target-configuration.js#176
And then, interpret it from the content process:
https://searchfox.org/mozilla-central/rev/a9db89754fb507254cb8422e5a00af7c10d98264/devtools/server/actors/targets/target-actor-mixin.js#77-87
(you may look at bug 1690698 to see all what's needed to implement a new data entry in DevTools)
This addWatcherDataEntry method will be called for each WindowGlobal before it starts loading. So at the perfect timing to evaluate the injected script. You may access any common target actor (the target actor, the webconsole actor, or the old inspected window actor if needed), but you would have to instantiate them as the frontend will only be able to communicate with this target actor a bit later.