Make devtools.inspectedWindow.reload injectedScript and userAgent options Fission-compatible
Categories
(WebExtensions :: Developer Tools, task, P3)
Tracking
(Fission Milestone:Future)
Fission Milestone | Future |
People
(Reporter: rpl, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-perf-stability-mvp)
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).
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 1•4 years ago
|
||
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•4 years ago
|
||
Tomislav says this bug doesn't need to block Fission MVP.
Comment 3•4 years ago
|
||
(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.
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 4•4 years ago
|
||
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).
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Updated•3 years ago
|
Comment 5•3 years ago
|
||
userAgent
may be fixed by bug 1706098.
injectedScript
would still require dedicated fix highlighted in comment 4.
Comment 6•3 years ago
|
||
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•3 years ago
|
||
(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.
Updated•3 years ago
|
Comment 8•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Description
•