1960319 - Use asynchronous messaging to allow the parent process to update the "navigator.webdriver" status in the content process

Status: RESOLVED FIXED

(Remote Protocol :: Agent, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

(In reply to :Gijs (he/him) from bug 1945048 comment #9)

The performance profile suggests at least one child is hung for 800+ms waiting to ask marionette if it's running, apparently from the webpage accessing Navigator.webdriver. That seems bad.

But of course, the reason it then takes a long time is that the parent is also hung.

Based on this comment we should probably move to async messaging to not unnecessarily block the content process as well when the parent process is hanging. We have two of these calls for Remote Agent and Marionette:

https://searchfox.org/mozilla-central/rev/917fb41190af26c3536ecc883212fc1e6710aa51/remote/components/RemoteAgent.sys.mjs#519
https://searchfox.org/mozilla-central/rev/917fb41190af26c3536ecc883212fc1e6710aa51/remote/components/Marionette.sys.mjs#289

We could use sendAsyncMessage instead and setup a listener for the response.

Since only the internal logic changes, there's no need to update Navigator.webidl.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Hi Olli. Do you know if switching from sync to async for navigator.webdriver would have any impact on the website? What would happen if the parent process is hanging and the web page is checking for that property? Would it still be blocked or return false?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Well, the API needs to know synchronously whether the webdriver is enabled, no? What could navigator.webdriver return if the messaging was async? But, parent process could send a message to all the relevant content processes telling that webdriver is enabled, and that could be cached then on content process side.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

That could be one option and we probably could do that via Services.ppmm.

One question through regarding the webidl property. If the underlying implementation changes to an async call, how would this property need to be updated to reflect that? Is it actually visible to webidl how a certain property is implemented? Isn't it always forced to be sync? If that's the case we would definitely have to use the proposed method that the parent process informs all content processes about the current or updated value.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

If the underlying implementation is async, then webidl side would likely need to return a promise, which is resolved once the return value is known.

If we don't want to / can't change the webidl method to return a promise, then an ugly way to make the underlying implementation async would be to spin event loop, but that should be avoided whenever possible (because it causes other issues). And sending some message to all the content processes about webdriver sounds like the right approach anyhow.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Thanks! And yes, we cannot change the webidl property so letting the parent process inform content processes seems to be the right approach then. Not sure if a default value is needed on the content side in case the parent hasn't updated it yet, but if yes it should be false.

One benefit would as well be that the value is no longer cached on the content side and the parent process can at any time inform about the state so that it is reflected correctly. At the moment we set the value to true whenever a Remote Protocol is running. But that is not what the WebDriver spec actually wants. Only when at least one WebDriver session is active the value should be true.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Attachment: Bug 1960319 - [remote] Use shared data to sync the WebDriver active status with content processes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e1a1ed8c68c
[remote] Use shared data to sync the WebDriver active status with content processes. r=webdriver-reviewers,jdescottes,nika

Comment 8

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/1e1a1ed8c68c