Closed Bug 1169290 Opened 10 years ago Closed 7 years ago

Add NavigatorAutomationInformation WebIDL interface and expose it when Marionette is active

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: automatedtester, Assigned: ato)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, pi-marionette-server, pi-marionette-spec, Whiteboard: [dev-doc-needed][wptsync upstream])

Attachments

(11 files)

Bug 1169290 - Improve getPrefVal readability.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Define log level conversion table in function scope.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Convert MarionetteComponent to a class.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Tell running state from whether server is alive.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Fire remote-active observer notification in component.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Handle -marionette flag in observe function.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Reintroduce marionette.enabled pref.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Make Marionette component safe to load in child process.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Allow nsIMarionette to be initialised from C++.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1169290 - Add navigator.webdriver attribute.
59 bytes, text/x-review-board-request
impossibus
: review+
bzbarsky
: review+
Details
Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref.
59 bytes, text/x-review-board-request
bzbarsky
: review+
impossibus
: review+
Details
We should give sites the ability to figure out if a bot is visiting them if they felt the need to block certain features.
Blocks: webdriver
Keywords: ateam-marionette-server
Summary: Implement navigator.webdriver to show when Marionette is enabled or not. → Expose navigator.webdriver IDL attribute when Marionette is active
Priority: -- → P2
Summary: Expose navigator.webdriver IDL attribute when Marionette is active → Add NavigatorAutomationInformation WebIDL interface and expose it when Marionette is active
The WebDriver standard defines a NavigatorAutomationInformation WebIDL interface [1] that is meant to be exposed when WebDriver is active in the user agent. This property defines a standard way for a co-operating user agent to inform a web document that it is controlled by WebDriver. Gecko’s WebDriver implementation is backed by Marionette, and we would want to expose the navigator.webdriver getter when it is activated. Marionette is active only when a -marionette flag is passed to Firefox, e.g. it cannot be enabled or disabled at runtime through a preference [2]. Marionette [3] is implemented as a chrome/system JS module and shipped with Firefox. It gets activated in an XPCOM component after the system notification session-windows-restored fires [4]. I’m not sure if this is relevant, but it implements the nsIMarionette IDL interface with a "running" attribute [5] to indicate that it is enabled. This is used in Firefox frontend code to trigger a UX code path that turns the address bar orange to warn the user that the browser is under remote control. peterv: I tried reading the WebIDL documentation [6] but I’m unsure exactly what would be the right way to implement this. Since navigator.webdriver is meant to just return a static boolean true when invoked, the implementation part to this would seem straight forward, but it would be great to have some input from you how we should go about this and what you think is the appropriate way to implement this. Thanks in advance! [1] https://w3c.github.io/webdriver/webdriver-spec.html#interface [2] In fact, it is considered a security risk because addons can modify preferences. [3] https://searchfox.org/mozilla-central/source/testing/marionette [4] https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js [5] https://searchfox.org/mozilla-central/source/testing/marionette/components/nsIMarionette.idl [6] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
Flags: needinfo?(peterv)
OS: Unspecified → All
Hardware: Unspecified → All
I think in the current state of our WebIDL parser you want a partial Navigator interface with the webdriver attribute. Put a Func on the webdriver attribute that calls a C++ function that just returns the value of nsIMarionette's running attribute. Probably easiest to put the C++ function somewhere in Navigator.cpp.
Flags: needinfo?(peterv)
Assignee: nobody → ato
Depends on: 1432894
No longer depends on: 1432894
FWIW there is some contention in [1] towards whether the navigator.webdriver attribute should always be exposed. The argument is that selectively exposing it is not comparable to other web features that are either enabled/disabled. I don’t think the issue necessarily blocks us from implementing or shipping this, as it would be backwards compatible to always expose it. [1] https://github.com/w3c/webdriver/issues/1214
Attachment #8946286 - Flags: review?(peterv)
Attachment #8946286 - Flags: review?(mjzffr)
Let’s wait for https://github.com/w3c/webdriver/issues/1214 to get resolved before exposing the WebIDL attribute.
Comment on attachment 8946277 [details] Bug 1169290 - Improve getPrefVal readability. https://reviewboard.mozilla.org/r/216232/#review223136
Attachment #8946277 - Flags: review?(mjzffr) → review+
Comment on attachment 8946278 [details] Bug 1169290 - Define log level conversion table in function scope. https://reviewboard.mozilla.org/r/216234/#review223138
Attachment #8946278 - Flags: review?(mjzffr) → review+
Comment on attachment 8946279 [details] Bug 1169290 - Convert MarionetteComponent to a class. https://reviewboard.mozilla.org/r/216236/#review223156 ::: testing/marionette/components/marionette.js:307 (Diff revision 1) > + if (outer) { > + throw Cr.NS_ERROR_NO_AGGREGATION; > + } > + > + let marionette = new MarionetteComponent(); > + return marionette.QueryInterface(iid); This is passing `iid` to a getter, so I don't see how it's used. Typo?
Attachment #8946279 - Flags: review?(mjzffr) → review+
Comment on attachment 8946280 [details] Bug 1169290 - Tell running state from whether server is alive. https://reviewboard.mozilla.org/r/216238/#review223158
Attachment #8946280 - Flags: review?(mjzffr) → review+
Comment on attachment 8946281 [details] Bug 1169290 - Fire remote-active observer notification in component. https://reviewboard.mozilla.org/r/216240/#review223162
Attachment #8946281 - Flags: review?(mjzffr) → review+
Comment on attachment 8946282 [details] Bug 1169290 - Handle -marionette flag in observe function. https://reviewboard.mozilla.org/r/216242/#review223164
Attachment #8946282 - Flags: review?(mjzffr) → review+
Comment on attachment 8946283 [details] Bug 1169290 - Reintroduce marionette.enabled pref. https://reviewboard.mozilla.org/r/216244/#review223168
Attachment #8946283 - Flags: review?(mjzffr) → review+
Comment on attachment 8946284 [details] Bug 1169290 - Make Marionette component safe to load in child process. https://reviewboard.mozilla.org/r/216246/#review223170
Attachment #8946284 - Flags: review?(mjzffr) → review+
Comment on attachment 8946285 [details] Bug 1169290 - Allow nsIMarionette to be initialised from C++. https://reviewboard.mozilla.org/r/216248/#review223172
Attachment #8946285 - Flags: review?(mjzffr) → review+
I have submitted https://github.com/w3c/webdriver/pull/1219 to change the spec after I found out both Safari and Chrome expose it unconditionally.
Comment on attachment 8946279 [details] Bug 1169290 - Convert MarionetteComponent to a class. https://reviewboard.mozilla.org/r/216236/#review223156 > This is passing `iid` to a getter, so I don't see how it's used. Typo? The getter returns the XPCOMUtils.generateQI factory, so the argument ends up being passed to the generated function, not the getter.
Whiteboard: [dev-doc-needed]
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review223636 ::: testing/web-platform/tests/webdriver/tests/interface.html:27 (Diff revision 2) > -if ("webdriver" in navigator) { > - test(() => assert_true(navigator.webdriver), "navigator.webdriver is always true"); > +if (navigator.webdriver) { > + test(() => assert_true(navigator.webdriver), "navigator.webdriver is true when webdriver-active is set"); This test doesn't make sense to me. Isn't it tautologically going to pass?
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review223636 > This test doesn't make sense to me. Isn't it tautologically going to pass? If the attribute isn’t exposed, navigator.webdriver will be undefined ant fail the false assertion on :30. Unfortunately this test is run using Marionette in WPT, so navigator.webdriver will always be true in automation. I’m not sure there is a good workaround for that? If the test is run manually, without automation, it is still expected to pass when navigator.webdriver is false. All we’re trying to check here is that (1) the attribute exists, (2) its value is a boolean, and (3) that it matches the WebIDL definition.
I have corrected the last patch to highlight that this is a _configurable_ (i.e. not [Unforgeable]) attribute addition, addressing :bz’s comment from [1]. [1] https://groups.google.com/d/msg/mozilla.dev.platform/GRNpjC4MuH8/L-pWP6AtAgAJ
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review223638 ::: dom/webidl/Navigator.webidl:312 (Diff revision 2) > partial interface Navigator { > [Pref="security.webauth.webauthn", SecureContext, SameObject] > readonly attribute CredentialsContainer credentials; > }; > + > +partial interface Navigator { This needs to have spec links here and in the header, like every other partial interface in this file.
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review223636 > If the attribute isn’t exposed, navigator.webdriver will be > undefined ant fail the false assertion on :30. > > Unfortunately this test is run using Marionette in WPT, so > navigator.webdriver will always be true in automation. I’m not > sure there is a good workaround for that? > > If the test is run manually, without automation, it is still expected > to pass when navigator.webdriver is false. All we’re trying to > check here is that (1) the attribute exists, (2) its value is a > boolean, and (3) that it matches the WebIDL definition. Ah, assert_false asserts === false. Alright, that's not a complete tautology, then. ;)
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review223636 > Ah, assert_false asserts === false. Alright, that's not a complete tautology, then. ;) I’ve added another assertion before this which explicitly checks that the attribute is present. Because this is a bit of a special test I’ve also added a comment explaining the true/false outcome.
Priority: P2 → P1
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review224318 ::: testing/web-platform/tests/webdriver/tests/interface.html:34 (Diff revision 4) > - var idlArray = new IdlArray(); > - [].forEach.call(document.querySelectorAll("script[type=text\\/plain]"), function(node) { > - if (node.className == "untested") { > - idlArray.add_untested_idls(node.textContent); > - } else { > - idlArray.add_idls(node.textContent); > +// When test is run in automation navigator.webdriver is likely to > +// be true because WebDriver controls the browser instance. To that > +// extent, this test is a bit special. It should also be possible to > +// run the test manually, when WebDriver is not active, and so either > +// true/false outcome is OK. > +if (navigator.webdriver) { Is there any other condition that would indicate automation or lack thereof to avoid the tautology in the test? Maybe an environment variable?
Attachment #8946286 - Flags: review?(mjzffr) → review+
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review224714 ::: dom/webidl/Navigator.webidl:312 (Diff revision 4) > partial interface Navigator { > [Pref="security.webauth.webauthn", SecureContext, SameObject] > readonly attribute CredentialsContainer credentials; > }; > + > +// https://w3c.github.io/webdriver/webdriver-spec.html#interface Please list this at the top of the file too, with the other spec links. ::: dom/webidl/Navigator.webidl:313 (Diff revision 4) > [Pref="security.webauth.webauthn", SecureContext, SameObject] > readonly attribute CredentialsContainer credentials; > }; > + > +// https://w3c.github.io/webdriver/webdriver-spec.html#interface > +partial interface Navigator { Probably better to match the spec more closely, assuming the spec has a reason it's not using a mixin: Navigator implements NavigatorAutomationInformation; [NoInterfaceObject] interface NavigatorAutomationInformation { ... }
Attachment #8946286 - Flags: review?(bzbarsky) → review+
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review224714 > Probably better to match the spec more closely, assuming the spec has a reason it's not using a mixin: > > Navigator implements NavigatorAutomationInformation; > > [NoInterfaceObject] > interface NavigatorAutomationInformation { > ... > } Thanks for the great feedback!
Comment on attachment 8946286 [details] Bug 1169290 - Add navigator.webdriver attribute. https://reviewboard.mozilla.org/r/216250/#review224318 > Is there any other condition that would indicate automation or lack thereof to avoid the tautology in the test? Maybe an environment variable? I checked with jgraham and we don’t have any way to know if the automation in use is WebDriver-based or not. I’m afraid this is the best we can do.
Comment on attachment 8949765 [details] Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref. https://reviewboard.mozilla.org/r/219068/#review224820 ::: commit-message-b2000:3 (Diff revision 1) > +Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref. r?bz,maja_zf > + > +In the off chance exposing navigator.webdriver turns out s/In/On/
Attachment #8949765 - Flags: review?(bzbarsky) → review+
Comment on attachment 8949765 [details] Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref. https://reviewboard.mozilla.org/r/219068/#review224838
Attachment #8949765 - Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e92ad5128f3c Improve getPrefVal readability. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/348c46e5edd1 Define log level conversion table in function scope. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/103ae77be7b8 Convert MarionetteComponent to a class. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/0f9e1fe6e905 Tell running state from whether server is alive. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/2365c2d010cb Fire remote-active observer notification in component. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/fd299ab7f1c7 Handle -marionette flag in observe function. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/5f35bfdac21d Reintroduce marionette.enabled pref. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/fb6cc1b9e730 Make Marionette component safe to load in child process. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/05abef0a4f0a Allow nsIMarionette to be initialised from C++. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/9ddfd2155429 Add navigator.webdriver attribute. r=bz,maja_zf https://hg.mozilla.org/integration/autoland/rev/47a5027e0b51 Guard navigator.webdriver behind dom.webdriver.enabled pref. r=bz,maja_zf
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9482 for changes under testing/web-platform/tests
Whiteboard: [dev-doc-needed] → [dev-doc-needed][wptsync upstream]
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Can't merge web-platform-tests PR due to failing upstream tests
(In reply to Web Platform Test Sync Bot from comment #101) > Can't merge web-platform-tests PR due to failing upstream tests What action do I need to take here? Why did it fail to upstream the tests when it earlier reported that upstream status checks passed?
Flags: needinfo?(james)
Merging PR failed: 405 {u'documentation_url': u'https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button', u'message': u'Base branch was modified. Review and try the merge again.'}
The sync bot had a bug in this case. Various issues are known, and we are going to improve the messages when there are actual upstream test failures.
Flags: needinfo?(james)
Blocks: 1467453
We should update https://developer.mozilla.org/en-US/docs/Web/API/Navigator/webdriver and/or the compat table on that page about our support of this.
Keywords: dev-doc-needed
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: