1706098 - Set customUserAgent BrowsingContext property in the parent process instead of in webextension-inspected-window.js

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, task)

Comment 1

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

Hey Nicolas,
If I'm reading this bug correctly, this and Bug 1700739 may be a duplicate of each other (or this one a subset of Bug 1700739),
is that right?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I guess this one will be a subset as I'm only focusing on the userAgent property.
This also depends on Bug 1704738

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

After reading the code a bit, I think that's already the case?

The customUserAgent is set from CustomizedReload#start and CustomizedReload#stop.
Those methods are called from the WebExtensionInspectedWindowActor#reload methods, which is run through Services.tm.dispatchToMainThread(delayedReload) (webextension-inspected-window.js#473), which I guess from the name is run in parent process.

We could make sure of that by adding an assertion in BrowsingContext::SetCustomUserAgent to make it fail if it's not called from parent process.

Comment 4

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)

After reading the code a bit, I think that's already the case?

The customUserAgent is set from CustomizedReload#start and CustomizedReload#stop.
Those methods are called from the WebExtensionInspectedWindowActor#reload methods, which is run through Services.tm.dispatchToMainThread(delayedReload) (webextension-inspected-window.js#473), which I guess from the name is run in parent process.

We could make sure of that by adding an assertion in BrowsingContext::SetCustomUserAgent to make it fail if it's not called from parent process.

The actor and utils provided by webextension-inspected-window.js are going to be executed in the same process where the extensions are being executed, and so it would run in the extension child process in the desktop builds and in the parent process in the GeckoView builds (at the moment, but in the long run GeckoView should also run the extension in a separate child extension process).

Services.tm.dispatchToMainThread does dispatch the given function on the main thread of the same process where the call has been originated (and so it will be in the main thread of the child extension process in the desktop builds).

I think it may be reasonable to meet (e.g. me, you and Alex?) to discuss a bit about how we may re-organize/re-architecture this part to better fit into the new DevTools architecture, wdyt?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Ah sorry, I misinterpreted "thread" for "process". thanks for pointing that out.
So setting the flag itself would be quite easy to do, since:

1. The reload method is already a method of a command
2. with Bug 1704738, we have another command to be able to set the user agent on the parent process
   3 and since commands are available in other commands, we could use targetConfigurationCommand.updateConfiguration with a custom user agent before calling reload on the WebExtensionInspectedWindow front (https://searchfox.org/mozilla-central/rev/b6f52976b562008c9d9ceeda22907e1eda506c8e/devtools/shared/commands/inspected-window/inspected-window-command.js#102)

we can then call reload, and the goal would be to call updateConfiguration another time once the reload did happen.
This is not conveyed by the reload method which will return early (as stated in https://searchfox.org/mozilla-central/rev/b6f52976b562008c9d9ceeda22907e1eda506c8e/devtools/server/actors/addon/webextension-inspected-window.js#470-472).
I guess the reason was to not have a pending request for too long while the server is waiting for the actual reload. We may have an event being emitted by the actor once the reload happened (or was interrupted), that we could listen to in the reload command to then reset the user agent.

Does that make sense Luca?

Comment 6

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

Thanks for the detailed description Nicolas, this plan does seem to make sense to me.

Once we will have adapted the userAgent options, based on that work we can then look into implementing something similar for injectedScript too (and we can be one of us from the Add-ons Team if you had to move to something else in the meantime, I think that adapting userAgent should make it clear to us how we should do the same for injectedScript).

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

So the fact that we are resetting the user agent as soon as the reload is done was puzzling me a bit, since it could lead to weird situation (say an iframe who's added later on would have a user agent different from the top-level document).
So I created a test web extension with a button that does reload the page with a custom user agent and checked how it was behaving on Chrome.
As far as I can tell, the user agent persists on Chrome, even after reloading or navigating (even to a new browsing context, which you can have when navigating to twitter.com).
For me it makes sense, this gave the ability to the custom devtools panel to start a user agent simulation, and I guess it's up for the panel to stop it, managing the user agent state by itself.
Closing DevTools then set the user agent back to its original value right away.

I'm not sure how we want to proceed in such case. Should we match Chrome's implementation? Do we want to first fix our current implementation and then do the actual thing we wanted to do in that bug, or can we do both at once directly here?

Comment 8

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)

So the fact that we are resetting the user agent as soon as the reload is done was puzzling me a bit, since it could lead to weird situation (say an iframe who's added later on would have a user agent different from the top-level document).
So I created a test web extension with a button that does reload the page with a custom user agent and checked how it was behaving on Chrome.
As far as I can tell, the user agent persists on Chrome, even after reloading or navigating (even to a new browsing context, which you can have when navigating to twitter.com).
For me it makes sense, this gave the ability to the custom devtools panel to start a user agent simulation, and I guess it's up for the panel to stop it, managing the user agent state by itself.
Closing DevTools then set the user agent back to its original value right away.

ah, I see, thanks for looking into that and spotting the chrome incompatibility.

Personally I think that you are right:
keeping the userAgent for the entire "DevTools Toolbox session" (in other word: "until that toolbox is closed") would be less surprising.

It would be nice if the change to the userAgent applied by the extension would be more explicitly visible to the user (with the current UI I think it is going to be visible for the user only if it does switch to the responsive design mode), but I believe that the behavior on Chrome is not really that different (the user would have to switch explicitly to a different settings panel from what I recall), and so I'd say that we can proceed with aligning the behavior of the API without blocking on any UI change (which may be a nice enhancement but not really mandatory).

I'm not sure how we want to proceed in such case. Should we match Chrome's implementation? Do we want to first fix our current implementation and then do the actual thing we wanted to do in that bug, or can we do both at once directly here?

Based on a quick look on bugzilla it doesn't seem that this chrome incompatibilities has ever been reported to us, and so given that we are also going to match the behavior on Chrome, I would also be ok if we want to do it both at once (adapting for fission + align the behavior with chrome), especially if it does make things simpler for you.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

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

Personally I think that you are right:
keeping the userAgent for the entire "DevTools Toolbox session" (in other word: "until that toolbox is closed") would be less surprising.

Great :)

It would be nice if the change to the userAgent applied by the extension would be more explicitly visible to the user (with the current UI I think it is going to be visible for the user only if it does switch to the responsive design mode), but I believe that the behavior on Chrome is not really that different (the user would have to switch explicitly to a different settings panel from what I recall), and so I'd say that we can proceed with aligning the behavior of the API without blocking on any UI change (which may be a nice enhancement but not really mandatory).

I filed Bug 1706570 for this

Based on a quick look on bugzilla it doesn't seem that this chrome incompatibilities has ever been reported to us, and so given that we are also going to match the behavior on Chrome, I would also be ok if we want to do it both at once (adapting for fission + align the behavior with chrome), especially if it does make things simpler for you.

Perfect, I think I have a patch ready and I updated the test so it covers a lot more and check that the feature works with Fission + remote frames.
I'll add you as a reviewer on it :)

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

So my patch is not done yet, devtools/shared/commands/inspected-window/tests/browser_webextension_inspected_window.js is failing with it.
This is because we don't accept new reload call when a previous reload isn't finished yet.
This does not match Chrome implementation and I filed Bug 1706622 for it.
If we want to keep the current implementation, we would have to have a way in the client to know if a reload is still pending so we wouldn't set the new user agent on the parent process. We don't send such information at the moment as the WebExtensionInspectedWindowActor#reload method returns right away, independently of the navigation status.
One way to work around this would be to send an event from the actor when the reload is done (or aborted) that we could listen to in the client, and manage a "reloadPending" state.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1706098 - [devtools] Make inspectedWindow.reload with userAgent implementation match Chrome's one and add support for Fission. r=ochameau,rpl.

Our implementation was setting a custom user agent on the document
before reloading it, and then resetting it as soon as the document
was loaded.
In Chrome, once the webextension sets the user agent, it persists
across reload and navigations, and is reset only when the toolbox
is closed.
This patch is adding similar behaviour to our implementation.

We also set the currentUserAgent flag on the browsing context from
the parent process instead of content. This is re-using the
targetConfigurationCommand, which also handle for us resetting the
user agent when the toolbox closes.

The existing test is expanded so it includes a remote iframe to ensure
the feature is supported on Fission. It also check the value of the user
agent through navigator.userAgent, and not only through the request header.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20d89921f646
[devtools] Make inspectedWindow.reload with userAgent implementation match Chrome's one and add support for Fission. r=ochameau,rpl,devtools-backward-compat-reviewers.

Comment 13

[Alexandru Michis [:malexandru]]

Backed out changeset 20d89921f646 (Bug 1706098) for causing bc failures in browser_ext_devtools_inspectedWindow_reload.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/b5df4a931575bc33a237052dbfdc3da2630e53c1
Push with failure, failure log.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82d59bab21da
[devtools] Make inspectedWindow.reload with userAgent implementation match Chrome's one and add support for Fission. r=ochameau,rpl,devtools-backward-compat-reviewers.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1706098 - [devtools] Cleanup browser_ext_devtools_inspectedWindow_reload.js. r=rpl.

Remove unused assignment in test.

Depends on D112980

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cc4d8836101
[devtools] Cleanup browser_ext_devtools_inspectedWindow_reload.js. r=extension-reviewers,rpl.

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/82d59bab21da

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8cc4d8836101