Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error on proxyAutoconfigUrl set to null #490

Closed
juangj opened this issue Feb 22, 2017 · 21 comments
Closed

Error on proxyAutoconfigUrl set to null #490

juangj opened this issue Feb 22, 2017 · 21 comments

Comments

@juangj
Copy link
Contributor

juangj commented Feb 22, 2017

Firefox Version

52.0 beta 8

Platform

I'm using Linux, but it probably happens on all of them

Steps to reproduce -

./geckodriver-0.14.0 -b firefox52b8/firefox

curl -d '{"desiredCapabilities": {"proxy": {"proxyAutoconfigUrl": null}}, "requiredCapabilities": {}}' http://localhost:4444/session

Firefox starts up correctly and Marionette appears to handle this just fine, but Geckodriver returns an error:

{
"error":"session not created",
"message":"Expected [object String] \"proxyType\" in [object Object] {\"proxyAutoconfigUrl\":null}",
"stacktrace":"stack backtrace:
   0:           0x4e0eed - backtrace::backtrace::trace::had1fe133e10f4f84
   1:           0x4e13b2 - backtrace::capture::Backtrace::new::hea6d366cd875d5b7
   2:           0x43dc86 - geckodriver::marionette::MarionetteSession::response::h65e4938ffab7411a
   3:           0x43b542 - <geckodriver::marionette::MarionetteHandler as webdriver::server::WebDriverHandler<geckodriver::marionette::GeckoExtensionRoute>>::handle_command::h847e3041a2edbf86
   4:           0x4098ff - std::panicking::try::do_call::h3c184502ab82dbaa
   5:           0x5801da - panic_unwind::__rust_maybe_catch_panic
                        at /buildslave/rust-buildbot/slave/stable-dist-rustc-musl-linux/build/obj/../src/libpanic_unwind/lib.rs:97
   6:           0x41c9e4 - <F as alloc::boxed::FnBox<A>>::call_box::h1e0e39839fbedd5b
   7:           0x576944 - alloc::boxed::{{impl}}::call_once<(),()>
                        at /buildslave/rust-buildbot/slave/stable-dist-rustc-musl-linux/build/obj/../src/liballoc/boxed.rs:605
                         - std::sys_common::thread::start_thread
                        at /buildslave/rust-buildbot/slave/stable-dist-rustc-musl-linux/build/obj/../src/libstd/sys_common/thread.rs:21
                         - std::sys::imp::thread::{{impl}}::new::thread_start
                        at /buildslave/rust-buildbot/slave/stable-dist-rustc-musl-linux/build/obj/../src/libstd/sys/unix/thread.rs:84"
}

This error does not occur on Firefox 51.0.1.

@andreastt
Copy link
Contributor

According to the specification it should be a string type.

@juangj
Copy link
Contributor Author

juangj commented Feb 22, 2017

It still seems wrong to launch Firefox and then return the error, doesn't it? Also, the other string capabilities in the "proxy" object don't cause this (e.g., "httpProxy").

If you're insistent that this is OK behavior, then I can take this over to Selenium and make sure language bindings never pass null, as they seem to sometimes do now.

@andreastt
Copy link
Contributor

We currently do capabilities parsing in Firefox itself, because that is where Marionette lives. We expect this to move to geckodriver soon-ish, so that Firefox is not started if the capabilities passed are invalid.

When I implemented capabilities parsing in Marionette, I tried to follow the specification as it were at the time with regards to parsing proxy settings. It seems to have improved somewhat since then, but it still says proxyAutoconfigUrl should be a string type. Other properties we get from JSON Objects that are null or undefined are ignored, so I do feel that the proxy configuration object as described in the spec right now are slightly out of sync with what we do elsewhere.

In my opinion, we should ignore all properties which values are null or undefined, so I will look into whether we need to raise a spec bug on this.

@andreastt andreastt reopened this Feb 22, 2017
minusnine added a commit to tebeka/selenium that referenced this issue Feb 27, 2017
The current state of using a Proxy is both well-defined and variously
implemented.

* Chrome and ChromeDriver uses just the HTTP field with a host:port.

* Firefox via GeckoDriver directly requires that the host be provided in
  the HTTP field and the port be provided in the HTTPPort field.

* Firefox via Selenium 2 uses just the HTTP field, with the caveat that
  connections to localhost are not proxied, by default. An additional
  preference is needed to unset this.

* Firefox via Selenium 3 and GeckoDriver fails because Selenium adds a
  whole bunch of "nulls" for unset values in the Proxy object, which
  Marionette does not like. mozilla/geckodriver#490

Fixes #41.
minusnine added a commit to tebeka/selenium that referenced this issue Feb 27, 2017
The current state of using a Proxy is both well-defined and variously
implemented.

* Chrome and ChromeDriver uses just the HTTP field with a host:port.

* Firefox via GeckoDriver directly requires that the host be provided in
  the HTTP field and the port be provided in the HTTPPort field.

* Firefox via Selenium 2 uses just the HTTP field, with the caveat that
  connections to localhost are not proxied, by default. An additional
  preference is needed to unset this.

* Firefox via Selenium 3 and GeckoDriver fails because Selenium adds a
  whole bunch of "nulls" for unset values in the Proxy object, which
  Marionette does not like. mozilla/geckodriver#490

Fixes #41.
@andreastt andreastt mentioned this issue Feb 28, 2017
@andreastt andreastt changed the title Geckodriver error when proxyAutoconfigUrl capability is null Error on proxyAutoconfigUrl set to null Feb 28, 2017
@thibaut-sticky
Copy link

Waiting for a fix or clarification of the spec, do you see any workaround about this?

@andreastt
Copy link
Contributor

The “workaround” is to not define the fields holding null values. E.g. {proxyAutoconfigUrl: null}{}.

@thibaut-sticky
Copy link

Too bad, it does not help.

11:10:27.751 WARN - Exception: java.util.HashMap cannot be cast to java.lang.String
11:10:27.766 INFO - Executing: [new session: Capabilities [{proxy={proxyAutoconfigUrl={}, httpProxy=192.168.0.23, sslProxyPort=65497, proxyType=MANUAL, httpProxyPort=65497, sslProxy=192.168.0.23}, marionette=true, browserName=firefox, version=, platform=ANY, firefox_profile=UEsDBBQACAgIAE1ZYUoAAAAAAAAAA...}]])

@andreastt
Copy link
Contributor

Well proxyAutoconfigUrl should certainly not be an object: proxy={proxyAutoconfigUrl={}

@thibaut-sticky
Copy link

Hum, sorry I read too quickly your previous message.

@andreastt
Copy link
Contributor

andreastt commented Mar 1, 2017

The point is that proxyAutoconfigUrl needs to be either a string or undefined (missing) currently.

minusnine added a commit to tebeka/selenium that referenced this issue Mar 12, 2017
The current state of using a Proxy is both well-defined and variously
implemented.

* Chrome and ChromeDriver uses just the HTTP field with a host:port.

* Firefox via GeckoDriver directly requires that the host be provided in
  the HTTP field and the port be provided in the HTTPPort field.

* Firefox via Selenium 2 uses just the HTTP field, with the caveat that
  connections to localhost are not proxied, by default. An additional
  preference is needed to unset this.

* Firefox via Selenium 3 and GeckoDriver fails because Selenium adds a
  whole bunch of "nulls" for unset values in the Proxy object, which
  Marionette does not like. mozilla/geckodriver#490

Fixes #41.
minusnine added a commit to tebeka/selenium that referenced this issue Mar 12, 2017
The current state of using a Proxy is both well-defined and variously
implemented.

* Chrome and ChromeDriver uses just the HTTP field with a host:port.

* Firefox via GeckoDriver directly requires that the host be provided in
  the HTTP field and the port be provided in the HTTPPort field.

* Firefox via Selenium 2 uses just the HTTP field, with the caveat that
  connections to localhost are not proxied, by default. An additional
  preference is needed to unset this.

* Firefox via Selenium 3 and GeckoDriver fails because Selenium adds a
  whole bunch of "nulls" for unset values in the Proxy object, which
  Marionette does not like. mozilla/geckodriver#490

Fixes #41.
@eyal919
Copy link

eyal919 commented Apr 18, 2017

Any updates on this one? any ETA? This issue with firefox and the proxy holding us from upgrading the grid and moving to selenium 3.
My Last test was using Firefox 52, selenium 3.3.1 and geckodriver 0.15.0 and with the next capabilities for the proxy:

  1. desiredCapabilities.proxy = {proxyType: 'direct'};
  2. desiredCapabilities.proxy = {
    proxyType: "manual",
    httpProxy: proxyUrl,
    sslProxy: proxyUrl
    };

Both didn't work since the invalidArgumentError

  1. message: 'InvalidArgumentError: Expected [object String] "{\"proxyAutoconfigUrl\":null,\"socksUsername\":null,\"socksPassword\":null,\"autodetect\":false,\"httpProxy\":null,\"proxyType\":\"DIRECT\",\"noProxy\":null,\"ftpProxy\":null,\"socksProxy\":null,\"hCode\":832656744,\"class\":\"org.openqa.selenium.Proxy\",\"sslProxy\":null}" to be an object (WARNING: The server did not provide any stacktrace information)\nCommand duration or timeout: 0 milliseconds\nBuild info: version: '3.3.1', revision: '5234b32', time: '2017-03-10 09:04:52 -0800'\nSystem info: host: 'xxx', ip: '172.25.8.100', os.name: 'Mac OS X', os.arch: 'x86_64', os.version: '10.12.3', java.version: '1.8.0_60'\nDriver info: driver.version: FirefoxDriver',

  2. message: 'InvalidArgumentError: Expected [object String] "{\"proxyAutoconfigUrl\":null,\"socksUsername\":null,\"socksPassword\":null,\"autodetect\":false,\"httpProxy\":\"localhost:8087\",\"proxyType\":\"MANUAL\",\"noProxy\":null,\"ftpProxy\":null,\"socksProxy\":null,\"hCode\":1558851061,\"class\":\"org.openqa.selenium.Proxy\",\"sslProxy\":\"localhost:8087\"}" to be an object (WARNING: The server did not provide any stacktrace information)\nCommand duration or timeout: 0 milliseconds\nBuild info: version: '3.3.1', revision: '5234b32', time: '2017-03-10 09:04:52 -0800'\nSystem info: host: 'xxx', ip: '172.25.8.100', os.name: 'Mac OS X', os.arch: 'x86_64', os.version: '10.12.3', java.version: '1.8.0_60'\nDriver info: driver.version: FirefoxDriver',_

@whimboo
Copy link
Collaborator

whimboo commented Apr 21, 2017

Which binding are you using? Given your capabilities you are not setting 'proxyAutoconfigUrl', but something actually sets proxyAutoconfigUrl and others to null. Because of that it's failing.

@eyal919
Copy link

eyal919 commented Apr 23, 2017

@whimboo
Probably selenium passing null values, you can also see that other variables are null such: socksUsername, ftpProxy and more.

I also see that there are few issues related to proxy (0.16.0) , could be same root cause:
#669, #657 ,

@alessandro-aglietti
Copy link

geckodriver wants lowercase proxyType so, intead of MANUAL, send manual ;)

@whimboo
Copy link
Collaborator

whimboo commented Aug 3, 2017

@andreastt, I had another look at this and actually we have different issues here with the originally filed code:

  1. The failure @juangj gets is not because proxyAutoconfigUrl is null, but there is no entry for proxyType in the capabilities. As such Marionette bails out. Shouldn't this be a required property when defining proxy settings? I would suggest to raise a better message when it is missing.

  2. With proxyType set to pac we indeed get a failure in checking the type of proxyAutoconfigUrl. This also happens for any other kind of proxy. Here proxyAutoconfigUrl should only be checked if proxyType is set to pac but not otherwise. So this property should be optional. I cannot see that we mark it that way in the spec. Should we update that?

  3. With both 1) and 2) fixed Marionette returns a valid session, but webdriver-rust fails now in extracting the data.

I would like to fix all the issues in https://bugzilla.mozilla.org/show_bug.cgi?id=1370959, but would need some feedback first from @andreastt to the above issues.

@andreastt
Copy link
Contributor

andreastt commented Aug 3, 2017 via email

@whimboo
Copy link
Collaborator

whimboo commented Aug 16, 2017

    desiredCapabilities.proxy = {proxyType: 'direct'};

@eyal919, this is a different issue and doesn't fit into here. But anyway I will let you know that I fixed that recently via bug 1387092. It will need a new geckodriver release, and Firefox 57.0.

    desiredCapabilities.proxy = {
    proxyType: "manual",
    httpProxy: proxyUrl,
    sslProxy: proxyUrl
    };

I'm currently refactoring all the code around manual proxy settings right now on bug 1369827. It will have the same target as the issue above.

Regarding the original issue here, my patch on bug 1391016 should improve the failure message. But we still need bug 1370959 fixed which moves all checks over to geckodriver.

@whimboo
Copy link
Collaborator

whimboo commented Sep 1, 2017

Please note that everything except the noProxy capability as has been discussed here is available now in Firefox 57.0 Nightly. To actually use it with geckodriver you will have to wait for the next release, or build it from source on your own.

If you still see problems please file a new issue. Thanks.

I will only keep this issue open to really fix the original issue as reported by @juangj. As it looks like by using curl directly we bypass the validate_proxy checks.

@andreastt
Copy link
Contributor

As it looks like by using curl directly we bypass the validate_proxy checks.

What do you mean by this? An HTTP request is an HTTP request.

@andreastt
Copy link
Contributor

Note that the capabilities JSON Object used in the curl request is now incorrect, however. It should be updated to use the WebDriver conforming capabilities negotiation.

I think this is correct, but I haven’t tested it:

curl -d '{"capabilities": {"alwaysMatch": {"proxy": {"proxyAutoconfigUrl": null}}}' http://localhost:4444/session

@whimboo
Copy link
Collaborator

whimboo commented Sep 5, 2017

There was a missing closing bracket in your last example but it clearly did it:

curl -d '{"capabilities": {"alwaysMatch": {"proxy": {"proxyAutoconfigUrl": null}}}}' http://localhost:4444/session

results in

{"value":{"error":"invalid argument","message":"proxyAutoconfigUrl was not a string",...

Something suspicious I still noticed is that if you specify an invalid capability object your are not always getting an exception. I filed bug 1396883 for that.

Otherwise I'm going to close this issue now. It's all fixed.

@whimboo whimboo closed this as completed Sep 5, 2017
@lock
Copy link

lock bot commented Aug 17, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. If you have run into an issue you think is related, please open a new issue.

@lock lock bot locked and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants