WebDriver specification defines (https://www.w3.org/TR/webdriver/#interface) webdriver IDL attribute for navigator as boolean. If session is controlled by automation, getter in current implementation returns undefined. Considering that property descriptor is unconditionally defined on navigator prototype [Object.getOwnPropertyDescriptor(navigator.__proto__, "webdriver")], I believe it should return false if session is not controlled by automation. I'll be happy to submit a patch if change seems reasonable.
Brian Burg is the WebDriver expert but I agree that our implementation does not seem to match the spec: 1. We return undefined instead of false when not controlled by automation 2. The property is not enumerable but should be
(In reply to Sergey Shekyan from comment #0) > WebDriver specification defines (https://www.w3.org/TR/webdriver/#interface) > webdriver IDL attribute for navigator as boolean. If session is controlled > by automation, getter in current implementation returns undefined. > > Considering that property descriptor is unconditionally defined on navigator > prototype [Object.getOwnPropertyDescriptor(navigator.__proto__, > "webdriver")], I believe it should return false if session is not controlled > by automation. > > I'll be happy to submit a patch if change seems reasonable. Patches welcome. As far as I know, nobody ships a W3C-compatible WebDriver implementation based on WebKit, but this shouldn't stop you from making this particular piece better.
I believe there is no reason to not having [Unforgeable] attribute on the property, and was about to open an issue in the spec repo.
Created attachment 310567 [details] Patch
Current webdriver editor's draft changed to declare navigator.webdriver as non-configurable (https://w3c.github.io/webdriver/webdriver-spec.html#interface)
Attachment 310567 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 310570 [details] Patch
Comment on attachment 310570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310570&action=review This needs a layout test to check that: 1. The property is false instead of being undefined now 2. The property is not configurable (Assuming this part of your change is right) 3. The property is enumerable (as I think it should be as per the spec). > Source/WebCore/ChangeLog:8 > + Please add a link to the spec in your changelog. > Source/WebCore/Modules/webdriver/NavigatorWebDriver.idl:27 > + [Unforgeable, NotEnumerable, Custom] readonly attribute boolean webdriver; We no longer need custom code at all. [Custom] should be dropped and the implementation should be updated accordingly. Could you also point to the spec that says it should be Unforgeable? I do know that [NotEnumerable] is not as per spec and should be dropped but last time I checked, there was no [Unforgeable] in the spec.
Let me know if this is more work than expected and you'd like me to take over. I don't mind fixing this.
Not at all, have the test ready. But apparently removing [Custom] is not as easy as I expected and I hope to get some insights from browsing the code on the weekend.
Comment on attachment 310570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310570&action=review > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > JSValue JSNavigator::webdriver(ExecState&) const If you drop the [Custom], I believe you'll need to drop this method and add a new one: static bool NavigatorWebDriver::webdriver(Navigator&) const { return isControlledByAutomation(); }
Created attachment 310918 [details] Patch
(In reply to Chris Dumez from comment #11) > Comment on attachment 310570 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310570&action=review > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > > JSValue JSNavigator::webdriver(ExecState&) const > > If you drop the [Custom], I believe you'll need to drop this method and add > a new one: > static bool NavigatorWebDriver::webdriver(Navigator&) const > { > return isControlledByAutomation(); > } It was a little more than that, but I think it works now. What should we do about worker navigator?
Comment on attachment 310918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310918&action=review Patch looks good otherwise. > Source/WebCore/ChangeLog:5 > + Per WebDriver Specification at https://www.w3.org/TR/webdriver/#interface The description should come *after* the reviewed by line. > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:52 > + Frame* frame = navigator.frame(); auto* > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:53 > + if (frame || !frame->page()) This check seems wrong, should probably !frame. > LayoutTests/js/dom/navigator-webdriver.html:8 > +description( Just description("Check that navigator.webdriver has the right attributes."); > LayoutTests/js/dom/navigator-webdriver.html:12 > +if (window.testRunner) { We don't use curly brackets for one-liners.
(In reply to Sergey Shekyan from comment #13) > (In reply to Chris Dumez from comment #11) > > Comment on attachment 310570 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review > > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > > > JSValue JSNavigator::webdriver(ExecState&) const > > > > If you drop the [Custom], I believe you'll need to drop this method and add > > a new one: > > static bool NavigatorWebDriver::webdriver(Navigator&) const > > { > > return isControlledByAutomation(); > > } > > It was a little more than that, but I think it works now. What should we do > about worker navigator? I do not understand the question, we do not seem to expose navigator.webdriver to workers. The spec does not seem to say we should expose this to workers either. The specs has a partial interface for Navigator but not WorkerNavigator [1]. [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator
(In reply to Chris Dumez from comment #15) > (In reply to Sergey Shekyan from comment #13) > > (In reply to Chris Dumez from comment #11) > > > Comment on attachment 310570 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review > > > > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > > > > JSValue JSNavigator::webdriver(ExecState&) const > > > > > > If you drop the [Custom], I believe you'll need to drop this method and add > > > a new one: > > > static bool NavigatorWebDriver::webdriver(Navigator&) const > > > { > > > return isControlledByAutomation(); > > > } > > > > It was a little more than that, but I think it works now. What should we do > > about worker navigator? > > I do not understand the question, we do not seem to expose > navigator.webdriver to workers. The spec does not seem to say we should > expose this to workers either. The specs has a partial interface for > Navigator but not WorkerNavigator [1]. > > [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator Oh, I CCed you to the discussion at https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/rXbGoRohBgAJ . Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is likely be implemented.
(In reply to Sergey Shekyan from comment #16) > (In reply to Chris Dumez from comment #15) > > (In reply to Sergey Shekyan from comment #13) > > > (In reply to Chris Dumez from comment #11) > > > > Comment on attachment 310570 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review > > > > > > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > > > > > JSValue JSNavigator::webdriver(ExecState&) const > > > > > > > > If you drop the [Custom], I believe you'll need to drop this method and add > > > > a new one: > > > > static bool NavigatorWebDriver::webdriver(Navigator&) const > > > > { > > > > return isControlledByAutomation(); > > > > } > > > > > > It was a little more than that, but I think it works now. What should we do > > > about worker navigator? > > > > I do not understand the question, we do not seem to expose > > navigator.webdriver to workers. The spec does not seem to say we should > > expose this to workers either. The specs has a partial interface for > > Navigator but not WorkerNavigator [1]. > > > > [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator > > Oh, I CCed you to the discussion at > https://groups.google.com/a/chromium.org/forum/ > ?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/ > rXbGoRohBgAJ . > > Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is > likely be implemented. Ok. In any case, exposing this to workers would need to be addressed separately. Brian Burg should probably comment on wether we want to exposed navigator.webDriver to workers or not.
Created attachment 310926 [details] Patch
Hmm, looks like the GTK bot does not link: lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o):JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*, long, JSC::PropertyName): error: undefined reference to 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const' collect2: error: ld returned 1 exit status Seems odd. I bet that bot merely needs a clean build.
Comment on attachment 310926 [details] Patch r=me
(In reply to Chris Dumez from comment #17) > (In reply to Sergey Shekyan from comment #16) > > (In reply to Chris Dumez from comment #15) > > > (In reply to Sergey Shekyan from comment #13) > > > > (In reply to Chris Dumez from comment #11) > > > > > Comment on attachment 310570 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review > > > > > > > > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72 > > > > > > JSValue JSNavigator::webdriver(ExecState&) const > > > > > > > > > > If you drop the [Custom], I believe you'll need to drop this method and add > > > > > a new one: > > > > > static bool NavigatorWebDriver::webdriver(Navigator&) const > > > > > { > > > > > return isControlledByAutomation(); > > > > > } > > > > > > > > It was a little more than that, but I think it works now. What should we do > > > > about worker navigator? > > > > > > I do not understand the question, we do not seem to expose > > > navigator.webdriver to workers. The spec does not seem to say we should > > > expose this to workers either. The specs has a partial interface for > > > Navigator but not WorkerNavigator [1]. > > > > > > [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator > > > > Oh, I CCed you to the discussion at > > https://groups.google.com/a/chromium.org/forum/ > > ?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/ > > rXbGoRohBgAJ . > > > > Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is > > likely be implemented. > > Ok. In any case, exposing this to workers would need to be addressed > separately. Brian Burg should probably comment on wether we want to exposed > navigator.webDriver to workers or not. I don't think that would be necessary. This property exists to mitigate people using WebDriver for click fraud or other unintended purposes on arbitrary websites. By checking this the page content can perform a mitigation if the "user" is a script. Web content can check this in the main context and propagate to workers if needed.
Comment on attachment 310926 [details] Patch Attachment 310926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3796504 New failing tests: js/dom/navigator-webdriver.html fast/dom/navigator-detached-no-crash.html
Created attachment 310941 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310926 [details] Patch Attachment 310926 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3796515 New failing tests: js/dom/navigator-webdriver.html fast/dom/navigator-detached-no-crash.html
Created attachment 310944 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
I quickly looked at the test failures and they look expected. I think you merely need to rebaseline those: Tools/Scripts/run-webkit-tests --reset-results js/dom/navigator-webdriver.html fast/dom/navigator-detached-no-crash.html
Comment on attachment 310926 [details] Patch Attachment 310926 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3796529 New failing tests: js/dom/navigator-webdriver.html fast/dom/navigator-detached-no-crash.html
Created attachment 310954 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310926 [details] Patch Attachment 310926 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3796605 New failing tests: js/dom/navigator-webdriver.html
Created attachment 310956 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 311015 [details] Patch
Comment on attachment 311015 [details] Patch Clearing flags on attachment: 311015 Committed r217391: <http://trac.webkit.org/changeset/217391>
All reviewed patches have been landed. Closing bug.
(In reply to WebKit Commit Bot from comment #32) > Comment on attachment 311015 [details] > Patch > > Clearing flags on attachment: 311015 > > Committed r217391: <http://trac.webkit.org/changeset/217391> This appears to have broken the build on our internal clang static analyzer bot: Undefined symbols for architecture x86_64: "WebCore::JSNavigator::webdriver(JSC::ExecState&) const", referenced from: WebCore::jsNavigatorWebdriverGetter(JSC::ExecState&, WebCore::JSNavigator&, JSC::ThrowScope&) in JSNavigator.o ld: symbol(s) not found for architecture x86_64 I'm not sure why this hasn't been seen on any other bots, though, except GTK+ EWS (although trunk builders seem fine): lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o):JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*, long, JSC::PropertyName): error: undefined reference to 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const' collect2: error: ld returned 1 exit status Maybe it needs a clean build?
(In reply to David Kilzer (:ddkilzer) from comment #34) > (In reply to WebKit Commit Bot from comment #32) > > Comment on attachment 311015 [details] > > Patch > > > > Clearing flags on attachment: 311015 > > > > Committed r217391: <http://trac.webkit.org/changeset/217391> > > This appears to have broken the build on our internal clang static analyzer > bot: > > Undefined symbols for architecture x86_64: > "WebCore::JSNavigator::webdriver(JSC::ExecState&) const", referenced from: > WebCore::jsNavigatorWebdriverGetter(JSC::ExecState&, > WebCore::JSNavigator&, JSC::ThrowScope&) in JSNavigator.o > ld: symbol(s) not found for architecture x86_64 > > I'm not sure why this hasn't been seen on any other bots, though, except > GTK+ EWS (although trunk builders seem fine): > > lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/ > WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o): > JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*, > long, JSC::PropertyName): error: undefined reference to > 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const' > collect2: error: ld returned 1 exit status > > Maybe it needs a clean build? Yes, likely needs a clean build.
<rdar://problem/32479831>