navigator.webdriver should no longer depend on the marionette.enabled preference
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(2 files)
As noticed on bug 1632556 the navigator.webdriver
preference is always false
. This is because internally it depends on the marionette.enabled
preference:
We should make it so that the value is retrieved from the enabled
property of the Marionette component. By that we could even get rid of the preference at all.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)
What we have to do in Navigator.cpp
is to replace the retrieval of the preference with using an instance of nsIMarionette
and checking its running attribute.
Olli, when I'm doing this change should I best cache the instance of Marionette similar to the addon manager and others? Or is it ok to create the instance again whenever the navigator.webdriver
property is used?
Assignee | ||
Updated•3 years ago
|
Comment 2•3 years ago
|
||
What would be the other use of nsIMarionette?
Or could the method just return nsCOMPtr<nsIMarionette> marionette = /some how get instance/;
return marionette->GetRunning();
nsIMarionette seems to be a service, so not caching it is probably fine.
This shouldn't be performance critical code, since the pref should be disabled by default.
Obviously there needs to be some pref to control the web exposed part
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/dom/webidl/Navigator.webidl#319
Assignee | ||
Comment 3•3 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2)
What would be the other use of nsIMarionette?
There is nothing else. Just checking the .running
attribute.
Or could the method just return nsCOMPtr<nsIMarionette> marionette = /some how get instance/;
return marionette->GetRunning();
That's what I already tried locally. Doing that I would certainly have to add a marionette.h
header file to expose the the interface for C++ code. That's what we currently don't have. If getting the interface is not that dramatic I'm happy to take that path. Also the navigator.webdriver
property will most likely only be used once.
nsIMarionette seems to be a service, so not caching it is probably fine.
No, it's not a service.
Obviously there needs to be some pref to control the web exposed part
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/dom/webidl/Navigator.webidl#319
That is interesting and I wasn't aware of it. So far I thought that it should not be possible to turn that feature off. I read through the following pages but have problems to understand what we actually have to implement:
https://github.com/w3c/webdriver/pull/1219/files
https://bugzilla.mozilla.org/show_bug.cgi?id=1169290
James, could you please check that part again? Should dom.webdriver.enabled
control if we expose the property on navigator? The following comment from Andreas irritates me a bit:
Making the attribute selectively present, which is unlike any other
web features are implemented, forces web authors to detect automation
in the following way:
var underAutomation = ("webdriver" in navigator) ? navigator.webdriver : false;
Following this patch, web authors can do this:
var underAutomation = navigator.webdriver;
Comment 4•3 years ago
•
|
||
nsIMarionette is used as a service, so it is a service :)
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/security/manager/ssl/nsCertOverrideService.cpp#726
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/browser/base/content/browser.js#259,274
But if the idea is to expose .webdriver property always, then better to ensure accessing its value is fast. But I guess there would be still some pref to tell whether nsIMarionette service is available. And usually no, so the .webdriver would return false.
So something like
if (pref_enabled) {
nsCOMPtr<nsIMarionette> marionette = do_GetService(NS_MARIONETTE_CONTRACTID);
if (marionette) {
bool marionetteRunning = false;
marionette->GetRunning(&marionetteRunning);
return marionetteRunning;
}
}
return false;
Always trying to access the service might be too slow.
If normal builds don't have marionette service, then one could of course also just cache the service in a member variable of Navigator and return false, if it is null.
Comment 5•3 years ago
|
||
Oh, I see, the pref is always on https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/modules/libpref/init/StaticPrefList.yaml#3338,3340
So then have
nsCOMPtr<nsIMarionetter> mMarionette in Navigator
Comment 6•3 years ago
|
||
The attribute is always present; when there's an active marionette session it should return true otherwise it should return false. I don't think it should depend on any pref.
Assignee | ||
Comment 7•3 years ago
|
||
As discussed with James last week the dom.webdriver.enabled
preference needs to be removed. It has been originally introduced to allow a quick and easy disabling of the web exposed navigator.webdriver
property in case of web-compat issues. This was landed in Firefox 60 via bug 1169290.
As such I will go ahead and remove any preferences which would allow users to disable the property, which should not be allowed.
Comment hidden (offtopic) |
Comment hidden (offtopic) |
Assignee | ||
Comment 10•3 years ago
|
||
The preference was meant to exist only temporarily when
the "navigator.webdriver" property was added by bug 1169290
in Firefox 60.
Assignee | ||
Comment 11•3 years ago
|
||
The "marionette.enabled" preference will be removed because it
should no longer be used to determine if Marionette enabled or not.
As such the enabled / running state can be retrieved via the
nsIMarionette XPCOM service.
Depends on D106778
Assignee | ||
Comment 12•3 years ago
|
||
I don't think we have to keep this bug confidential anymore. Also the right place is Core : DOM.
Comment 13•3 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2edc1a0a0b2 [dom] Remove dom.webdriver.enabled preference. r=smaug https://hg.mozilla.org/integration/autoland/rev/d056cc5c66cf [dom] Base navigator.webdriver on nsIMarionette::running instead of marionette.enabled perference. r=smaug
Comment 14•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2edc1a0a0b2
https://hg.mozilla.org/mozilla-central/rev/d056cc5c66cf
Description
•