Closed Bug 1632821 Opened 4 years ago Closed 3 years ago

navigator.webdriver should no longer depend on the marionette.enabled preference

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
88 Branch
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:

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/dom/base/Navigator.cpp#2097-2099

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.

Priority: -- → P2
Blocks: 1593343

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/dom/base/Navigator.cpp#2097-2099

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?

Flags: needinfo?(bugs)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

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

Flags: needinfo?(bugs)

(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;
Group: mozilla-employee-confidential
Flags: needinfo?(james)

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.

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.

Flags: needinfo?(james)

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.

See Also: → 1169290
Blocks: 1695626

The preference was meant to exist only temporarily when
the "navigator.webdriver" property was added by bug 1169290
in Firefox 60.

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

I don't think we have to keep this bug confidential anymore. Also the right place is Core : DOM.

Group: mozilla-employee-confidential
Component: Marionette → DOM: Core & HTML
Product: Testing → Core
Version: Version 3 → unspecified
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
See Also: → 1696425
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: