Closed Bug 1369827 Opened 7 years ago Closed 7 years ago

Allow optional port for [ftp|http|https|socks]Proxy keys in capabilities

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs,
URL
)

Details

(Whiteboard: [spec][17/08])

Attachments

(5 files)

Bug 1369827 - Upgrade webdriver crate to 0.30.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1369827 - Make proxy port an optional suffix for the host.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1369827 - Fix socksVersion key in proxy capabilities.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1369827 - Update geckodriver docs for proxy port changes.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1369827 - Vendoring in webdriver 0.30.
59 bytes, text/x-review-board-request
ato
: review+
Details 
As filed at https://github.com/mozilla/geckodriver/issues/764 we do not parse the *Proxy keys correctly so that an optional port is not recognized, but an error thrown:

{
  "error":"session not created",
  "message":"InvalidArgumentError: Expected [object Undefined] undefined to be an integer",
  "stacktrace":"..."
}

It shouldn't be so hard to fix so we could take if for the next release of geckodriver (bug 1369709).
Component: Marionette → geckodriver
Whiteboard: [geckodriver]
Hm, so I had a look at the code of geckodriver and found the following:

https://dxr.mozilla.org/mozilla-central/source/testing/geckodriver/src/capabilities.rs#153

Andreas, does that mean that with `Ok(true)` we indicate that every handling of proxies is done in Marionette via a forward from Geckodriver? If yes, I think I will fix it in Marionette itself for now instead of re-implementing all proxy related capability code in geckodriver. Downside would be that we cannot ship it for older releases of Firefox. 

What do you think?
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #1)

> [D]oes that mean that with `Ok(true)` we indicate that every
> handling of proxies is done in Marionette via a forward from
> Geckodriver? If yes, I think I will fix it in Marionette itself for
> now instead of re-implementing all proxy related capability code
> in geckodriver. Downside would be that we cannot ship it for older
> releases of Firefox.

I think jgraham should correct me here if I’m wrong, but I think the
reason we didn’t implement this in geckodriver at all is because the
specification is basically horrible at describing what the input for the
proxy configuration object should be.
Flags: needinfo?(ato)
James, mind giving feedback to Andreas' last comment?
Flags: needinfo?(james)
I think this is just a bug; the spec isn't amaziingly written but it's clear enough.
Flags: needinfo?(james)
We know that this is a bug, the remaining open question was why Marionette and not Geckodriver. But given that I don't want to implement all the logic in GeckoDriver, we should fix it in Marionette.
Blocks: webdriver
Component: geckodriver → Marionette
(In reply to Henrik Skupin (:whimboo) from comment #5)
> We know that this is a bug, the remaining open question was why Marionette
> and not Geckodriver. But given that I don't want to implement all the logic
> in GeckoDriver, we should fix it in Marionette.

We don’t want to do any validation of the proxy configuration object in
Marionette given that session negotiation will be removed soon.  We do however
of course need to parse it, so keep in mind that we should assume the input
from geckodriver is correct.

I wonder also if it would be possible to normalise the configuration in
geckodriver before it is passed to Marionette.  E.g. how are we supposed to
handle an httpProxy and an httpProxyPort field if the former already includes
the port?  (This is an example of what I mean by the fact that the specification
is unclear.)
I would say explicit over implicit, so that *ProxyPort has precedence. Should I file a webdriver spec issue?
No longer blocks: 1369709
Depends on: 1370959
Priority: -- → P1
When this gets fixed we have to also remove ftpProxyPort, httpProxyPort, sslProxyPort, and socksProxyPort from our capabilities.
Lets get this done first before we do the refactoring on bug 1370959.
Blocks: 1370959
No longer depends on: 1370959
This needs a fix in webdriver-rust first. I filed: https://github.com/mozilla/webdriver-rust/issues/115
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Whiteboard: [spec][17/08]
Blocks: 1391605
webdriver 0.30 has been released. So I will update this patch now.
Attachment #8897397 - Flags: review?(ato)
Attachment #8898781 - Flags: review?(ato)
Attachment #8897398 - Flags: review?(ato)
Attachment #8897399 - Flags: review?(ato)
Attachment #8897934 - Flags: review?(ato)
Comment on attachment 8897397 [details]
Bug 1369827 - Upgrade webdriver crate to 0.30.

https://reviewboard.mozilla.org/r/168714/#review175338

::: commit-message-6213f:1
(Diff revision 4)
> +Bug 1369827 - Upgrade webdriver create to 0.30.

s/create/crate/
Attachment #8897397 - Flags: review?(ato) → review+
Comment on attachment 8898781 [details]
Bug 1369827 - Vendoring in webdriver 0.30.

https://reviewboard.mozilla.org/r/170168/#review175340

::: commit-message-2b66f:1
(Diff revision 1)
> +Bug 1369827 - Vendoring in Webdriver 0.30.

s/Webdriver/webdriver/
Attachment #8898781 - Flags: review?(ato) → review+
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:33
(Diff revision 6)
>              self.os_name = self.marionette.execute_script(
>                  "return Services.sysinfo.getProperty('name')").lower()
>              self.os_version = self.marionette.execute_script(
>                  "return Services.sysinfo.getProperty('version')")
>  
> -    def test_mandated_capabilities(self):
> +    def tst_mandated_capabilities(self):

s/tst/test/g

::: testing/marionette/harness/marionette_harness/tests/unit/test_proxy.py:68
(Diff revision 6)
> -        self.assertEqual(result["httpProxy"], url, 'httpProxy was not set')
> -        self.assertEqual(result["httpProxyPort"], port, 'httpProxyPort was not set')
> -        self.assertEqual(result["sslProxy"], url, 'sslProxy url was not set')
> -        self.assertEqual(result["sslProxyPort"], port, 'sslProxyPort was not set')
> -        self.assertEqual(result["ftpProxy"], url, 'ftpProxy was not set')
> -        self.assertEqual(result["ftpProxyPort"], port, 'ftpProxyPort was not set')
> +        capabilities = {"proxy": {"proxyType": "direct"}}
> +
> +        self.marionette.start_session(capabilities)
> +        self.assertEqual(self.proxy.type, 0)
> +
> +    def test_proxy_type_manual_without_port(self):

Can you add some tests for proxy configuration _without_ port, and one passing invalid URLs?

::: testing/marionette/session.js:129
(Diff revision 6)
>     * @return {boolean}
>     *     True if proxy settings were updated as a result of calling this
>     *     function, or false indicating that this function acted as
>     *     a no-op.
>     */
>    init() {

Can you please document what errors this function can throw?

::: testing/marionette/session.js:130
(Diff revision 6)
>     *     True if proxy settings were updated as a result of calling this
>     *     function, or false indicating that this function acted as
>     *     a no-op.
>     */
>    init() {
> +    function set_proxy_host_and_port(protocol, hostname) {

Use camelcasing, please.

::: testing/marionette/session.js:130
(Diff revision 6)
>     *     True if proxy settings were updated as a result of calling this
>     *     function, or false indicating that this function acted as
>     *     a no-op.
>     */
>    init() {
> +    function set_proxy_host_and_port(protocol, hostname) {

Because the protocol argument is a free-form string, you should check that the preference actually exists first by calling Preferences.has.

::: testing/marionette/session.js:130
(Diff revision 6)
>     *     True if proxy settings were updated as a result of calling this
>     *     function, or false indicating that this function acted as
>     *     a no-op.
>     */
>    init() {
> +    function set_proxy_host_and_port(protocol, hostname) {

s/hostname/host/ as it may contain a port.

::: testing/marionette/session.js:131
(Diff revision 6)
>     *     function, or false indicating that this function acted as
>     *     a no-op.
>     */
>    init() {
> +    function set_proxy_host_and_port(protocol, hostname) {
> +      let proxy = new URL("http://" + hostname);

This throws a TypeError if it cannot be parsed.  This should
probably be converted to an InvalidArgumentError?

::: testing/marionette/session.js:134
(Diff revision 6)
>    init() {
> +    function set_proxy_host_and_port(protocol, hostname) {
> +      let proxy = new URL("http://" + hostname);
> +
> +      Preferences.set(`network.proxy.${protocol}`, proxy.hostname);
> +      Preferences.set(`network.proxy.${protocol}_port`, Number(proxy.port));

So I guess "new URL(…)" always guarantees that the hostname is valid.
As we have discussed in the past, number coercion through Number has
a few surprises.  For example, Number(null) and Number("") both
yield 0, which would be unwanted in this case.

Because the default port value of the URL type, when hostname does not
have a port, is "" this would always set the preference to 0.

I think a safer option here is to use parseInt and Number.isInteger
to check that it is not NaN or a float.

::: testing/marionette/session.js:176
(Diff revision 6)
> -      default:
> +      case null:
> +      case undefined:

Good catch!
Attachment #8897398 - Flags: review?(ato) → review-
Comment on attachment 8897399 [details]
Bug 1369827 - Fix socksVersion key in proxy capabilities.

https://reviewboard.mozilla.org/r/168718/#review175364
Attachment #8897399 - Flags: review?(ato) → review+
Comment on attachment 8897934 [details]
Bug 1369827 - Update geckodriver docs for proxy port changes.

https://reviewboard.mozilla.org/r/169220/#review175366

::: testing/geckodriver/CHANGES.md:18
(Diff revision 4)
>  [`MaximizeWindow`]: https://docs.rs/webdriver/0.29.0/webdriver/command/enum.WebDriverCommand.html#variant.MaximizeWindow
>  [`MinimizeWindow`]: https://docs.rs/webdriver/0.29.0/webdriver/command/enum.WebDriverCommand.html#variant.MinimizeWindow
>  [`SetWindowRect`]: https://docs.rs/webdriver/0.29.0/webdriver/command/enum.WebDriverCommand.html#variant.SetWindowRect
>  
>  ### Changed
> +- Fixed correct handling of `socksVersion`.

“Correct”?

::: testing/geckodriver/CHANGES.md:19
(Diff revision 4)
>  [`MinimizeWindow`]: https://docs.rs/webdriver/0.29.0/webdriver/command/enum.WebDriverCommand.html#variant.MinimizeWindow
>  [`SetWindowRect`]: https://docs.rs/webdriver/0.29.0/webdriver/command/enum.WebDriverCommand.html#variant.SetWindowRect
>  
>  ### Changed
> +- Fixed correct handling of `socksVersion`.
> +- Removed `ftpProxyPort`, `httpProxyPort`, `sslProxyPort`, and `socksProxyPort` because these are optional for `ftpProxy`, `httpProxy`, `sslProxy`, and `socksProxy`.

“… because _ports_ can be set through […] using ":<PORT>".”
Attachment #8897934 - Flags: review?(ato) → review+
Comment on attachment 8897934 [details]
Bug 1369827 - Update geckodriver docs for proxy port changes.

https://reviewboard.mozilla.org/r/169220/#review175366

> “Correct”?

Ok, replaced with: "Removed deprecated `socksProxyVersion` in favor of `socksVersion`."

> “… because _ports_ can be set through […] using ":<PORT>".”

Not really related to my patch but I fixed this as a side-along.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> s/tst/test/g

Ups, left over debug changes.

> Can you add some tests for proxy configuration _without_ port, and one passing invalid URLs?

I added most of the tests as Python tests for an easier porting to WPT later. So should I duplicate the tests or which parts should we cover here?

> Because the protocol argument is a free-form string, you should check that the preference actually exists first by calling Preferences.has.

It's not a free form string but hard-coded lines in the switch block below. So nothing can go wrong with calling this function.

> s/hostname/host/ as it may contain a port.

Indeed.

> So I guess "new URL(…)" always guarantees that the hostname is valid.
> As we have discussed in the past, number coercion through Number has
> a few surprises.  For example, Number(null) and Number("") both
> yield 0, which would be unwanted in this case.
> 
> Because the default port value of the URL type, when hostname does not
> have a port, is "" this would always set the preference to 0.
> 
> I think a safer option here is to use parseInt and Number.isInteger
> to check that it is not NaN or a float.

By default there is always port 0 selected for the proxy. There is no way to say port 80 should be the default. This should really up to the implementer of the test. So having 0 in all the uncovered cases seems to be perfectly fine to me.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> I added most of the tests as Python tests for an easier porting to WPT later. So should I duplicate the tests or which parts should we cover here?

So where are the tests for proxy config without port and invalid URLs?

> It's not a free form string but hard-coded lines in the switch block below. So nothing can go wrong with calling this function.

> So nothing can go wrong with calling this function.

I’ve heard that before…

I guess Preferencs.set will throw some kind of error if the
preference name does not exist, which in turn will get propagated to
a session not created error, and that this is sufficient.

> By default there is always port 0 selected for the proxy. There is no way to say port 80 should be the default. This should really up to the implementer of the test. So having 0 in all the uncovered cases seems to be perfectly fine to me.

If the user doesn’t give us a port, I’d say we shouldn’t set
it.  I see that the default value of network.proxy.socks_port is 0,
so this is setting it unnecessarily.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175836
Attachment #8897398 - Flags: review?(ato) → review-
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> So where are the tests for proxy config without port and invalid URLs?

Please see test_proxy.py which is part of this patch? We clearly need everything preferences related as unit test. Maybe I can move some over as xpcshell test only to avoid double coverage. Then we can have those tests in test_proxy.py which we then can port as wdspec tests via bug 1370959 later.

Some unit tests are also located in webdriver rust code itself, which we could duplicate?

> > So nothing can go wrong with calling this function.
> 
> I’ve heard that before…
> 
> I guess Preferencs.set will throw some kind of error if the
> preference name does not exist, which in turn will get propagated to
> a session not created error, and that this is sufficient.

If a preference name is not existent it's hidden, but no failure will be thrown. It just sets a new preference with this name and value. If it's a totally unknown one to Firefox it will just not be obeyed. 

Please note that all invocations of this method happen from inside `init()`, and the protocol is hard-coded.

> If the user doesn’t give us a port, I’d say we shouldn’t set
> it.  I see that the default value of network.proxy.socks_port is 0,
> so this is setting it unnecessarily.

I can use `Number.isInteger()` and only set the preference when it evals to true. This indeed will prevent exceptions if it's a float or NaN.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> Please see test_proxy.py which is part of this patch? We clearly need everything preferences related as unit test. Maybe I can move some over as xpcshell test only to avoid double coverage. Then we can have those tests in test_proxy.py which we then can port as wdspec tests via bug 1370959 later.
> 
> Some unit tests are also located in webdriver rust code itself, which we could duplicate?

I looked at test_proxy.py again and I can’t see any tests using an
invalid URL.  I see one test for proxyAutoconfigUrl using null as
input, however.

The kind of eventualities that I would think about testing for are
also "foo:", "foo.", "foo:123", "[::1]:invalid" as _hosts_.

Regarding the code duplication, I don’t care about that.

> If a preference name is not existent it's hidden, but no failure will be thrown. It just sets a new preference with this name and value. If it's a totally unknown one to Firefox it will just not be obeyed. 
> 
> Please note that all invocations of this method happen from inside `init()`, and the protocol is hard-coded.

Yes, and then someone comes along and (mistakenly?) changes the a
string or a string concatenation, and you suddenly have your tests
failing without explanation.

> I can use `Number.isInteger()` and only set the preference when it evals to true. This indeed will prevent exceptions if it's a float or NaN.

Using Number.isInteger is good and it implicitly covers the NaN
check, as Number.isInteger(NaN) returns false.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> I looked at test_proxy.py again and I can’t see any tests using an
> invalid URL.  I see one test for proxyAutoconfigUrl using null as
> input, however.
> 
> The kind of eventualities that I would think about testing for are
> also "foo:", "foo.", "foo:123", "[::1]:invalid" as _hosts_.
> 
> Regarding the code duplication, I don’t care about that.

I can surely add those.

> Yes, and then someone comes along and (mistakenly?) changes the a
> string or a string concatenation, and you suddenly have your tests
> failing without explanation.

So I will make it a public method on the Proxy class.
By further implementing tests I noticed a problem with the URL DOM API:

> new URL("http://foo:80");

Whenever a port is used which is the default one for the given protocol, the `.port` property of the returned object is empty. Olli, is that expected, or why don't we return 80 in those cases? If that is expected is there another existent class to parse hosts in the form of "hostname[:port]" without having to write my own parser? Thanks.
Flags: needinfo?(bugs)
https://url.spec.whatwg.org/#concept-basic-url-parser seems to say "Set url’s port to null, if port is url’s scheme’s default port, and to port otherwise." and default port 
https://url.spec.whatwg.org/#default-port
Why you need to know about port 80 when it is the default for http?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #51)
> Why you need to know about port 80 when it is the default for http?

We have to parse proxy settings. So if the user specifies "foo.bar.whatever:80" as proxy for HTTP (https://w3c.github.io/webdriver/webdriver-spec.html#proxy), we have to extract the hostname and port. To use URL() I have to fake a protocol and add it as prefix, but always using `http://` would fail if the given port is equal to the default port of the faked protocol. Maybe I should then use nsIURI?
Flags: needinfo?(bugs)
I don't recall what behavior nsIURI has. Would need to test or read the code. Necko folks would know.
Flags: needinfo?(bugs)
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review175342

> I can surely add those.

Are you satisfied with the new number of tests added to the xpcshell test? I also made the Python test to only check the returned capabilities, so we can easily convert them to wdspec tests.
(In reply to Olli Pettay [:smaug] from comment #53)
> I don't recall what behavior nsIURI has. Would need to test or read the
> code. Necko folks would know.

I fixed that by simply using URL() once more but just with another fake protocol in case port is empty. I didn't find any other easy solution beside URL and nsIURI which both behave identically.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

You’re making progress here.  I just have one concern about the way you determine if the hostname + port is valid by using "http:" and "https:" as prefixes for the new URL constructor.  See the inline issue.

::: testing/marionette/session.js:147
(Diff revision 8)
> -          Preferences.set("network.proxy.http_port", this.httpProxyPort);
> -        }
> -        if (this.sslProxy && this.sslProxyPort) {
> -          Preferences.set("network.proxy.ssl", this.sslProxy);
> -          Preferences.set("network.proxy.ssl_port", this.sslProxyPort);
> -        }
> -        if (this.ftpProxy && this.ftpProxyPort) {
> -          Preferences.set("network.proxy.ftp", this.ftpProxy);
> -          Preferences.set("network.proxy.ftp_port", this.ftpProxyPort);
> +          Preferences.set(`network.proxy.ftp`, this.ftpProxy);
> +          if (Number.isInteger(this.ftpProxyPort)) {
> +            Preferences.set(`network.proxy.ftp_port`, this.ftpProxyPort);
> +          }
> +        }
> +
> +        if (this.httpProxy) {
> +          Preferences.set(`network.proxy.http`, this.httpProxy);
> +          if (Number.isInteger(this.httpProxyPort)) {
> +            Preferences.set(`network.proxy.http_port`, this.httpProxyPort);
> +          }
> +        }
> +
> +        if (this.sslProxy) {
> +          Preferences.set(`network.proxy.ssl`, this.sslProxy);
> +          if (Number.isInteger(this.sslProxyPort)) {
> +            Preferences.set(`network.proxy.ssl_port`, this.sslProxyPort);
> +          }
>          }
> +
>          if (this.socksProxy) {
> -          Preferences.set("network.proxy.socks", this.socksProxy);
> -          Preferences.set("network.proxy.socks_port", this.socksProxyPort);
> +          Preferences.set(`network.proxy.socks`, this.socksProxy);
> +          if (Number.isInteger(this.socksProxyPort)) {
> +            Preferences.set(`network.proxy.socks_port`, this.socksProxyPort);
> +          }

None of these need to be template strings.

::: testing/marionette/session.js:205
(Diff revision 8)
>    static fromJSON(json) {
> +    // Parse hostname and optional port from host
> +    function fromHost(host) {
> +      assert.string(host);
> +
> +      if (host.indexOf("://") != -1) {

if (host.includes("://")) is more readable.

::: testing/marionette/session.js:211
(Diff revision 8)
> +        // Use the HTTP protocol to get the host parsed. If the port
> +        // is empty it could mean that it was the default port for HTTP.
> +        // To still keep the port number parse again with HTTPS instead.
> +        url = new URL("http://" + host);
> +        if (url.port == "") {
> +          url = new URL("https://" + host);
> +        }

Don’t you have the same problem with "foo:443" on :216?

Wouldn’t parsing it with the "http://" prefix and assuming port 0 if it is an empty string work better?

::: testing/marionette/test_session.js:237
(Diff revision 8)
> -    manual[proxy] = "foo";
> -    manual[proxy + "Port"] = 1234;
> +    for (let host of ["foo", "foo.bar:80", "127.0.0.1", "127.0.0.1:42",
> +        "[2001:db8::1]", "[2001:db8::1]:42"]) {

I think you ought to test port 443 as well here (the default TLS port).
Attachment #8897398 - Flags: review?(ato) → review-
Note that many proxy tests just landed in downstream WPT:
https://github.com/w3c/web-platform-tests/commit/efb6b0d739b810e4b7ea2ce80ca35bad9ecae383

Unsure if these have made their way to m-c yet, but worth being on
the outlook for.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> Don’t you have the same problem with "foo:443" on :216?
> 
> Wouldn’t parsing it with the "http://" prefix and assuming port 0 if it is an empty string work better?

> Don’t you have the same problem with "foo:443" on :216?

No, `foo:443` would correctly return the port number 443 for HTTP because it's not the default port. Line 215 would only be true for port 80, or if no port has been given at all. Line 216 would then only fail for port 443, but for that one we never hit this line.

> Wouldn’t parsing it with the "http://" prefix and assuming port 0 if it is an empty string work better?

That would mean a host as given with `foo:80` will end-up with `foo` and `0`, and we loose the port number. I don't actually know if port 0 for a proxy host means use the default one. I may have to ask some networking guys.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> > Don’t you have the same problem with "foo:443" on :216?
> 
> No, `foo:443` would correctly return the port number 443 for HTTP because it's not the default port. Line 215 would only be true for port 80, or if no port has been given at all. Line 216 would then only fail for port 443, but for that one we never hit this line.
> 
> > Wouldn’t parsing it with the "http://" prefix and assuming port 0 if it is an empty string work better?
> 
> That would mean a host as given with `foo:80` will end-up with `foo` and `0`, and we loose the port number. I don't actually know if port 0 for a proxy host means use the default one. I may have to ask some networking guys.

So here the description from MDN: `Port: 0 will cause server:port preference for that manual proxy type to be ignored. 1-65xxx is valid range`.
(In reply to Andreas Tolfsen ‹:ato› from comment #62)
> Note that many proxy tests just landed in downstream WPT:
> https://github.com/w3c/web-platform-tests/commit/
> efb6b0d739b810e4b7ea2ce80ca35bad9ecae383

When I look at this PR I don't see any proxy test at all. There is only the case covered when proxy is null.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> So here the description from MDN: `Port: 0 will cause server:port preference for that manual proxy type to be ignored. 1-65xxx is valid range`.

I think that I have to file a spec issue to always require a port and not making it optional. But would be good to know how other browsers are handling that before.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> I think that I have to file a spec issue to always require a port and not making it optional. But would be good to know how other browsers are handling that before.

The question is if we should maybe fallback to the default port of the given protocol in cases when the port is empty, or 0. Looks like that this is something Chrome is doing, and enforcing a port might not be what we want in the spec?
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> The question is if we should maybe fallback to the default port of the given protocol in cases when the port is empty, or 0. Looks like that this is something Chrome is doing, and enforcing a port might not be what we want in the spec?

I just filed it anyway: https://github.com/w3c/webdriver/issues/1049
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> I just filed it anyway: https://github.com/w3c/webdriver/issues/1049

I just talked with a networking guy and he proposed to use nsIURI to parse the host and port. The logic as I have used here is fine which means we would need the fallback. In case we have to use the default port `Services.io.getProtocolHandler(uri.scheme).defaultPort` could be used. I will update the patch tomorrow.
Attachment #8897398 - Flags: review?(ato)
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review177946

> I just talked with a networking guy and he proposed to use nsIURI to parse the host and port. The logic as I have used here is fine which means we would need the fallback. In case we have to use the default port `Services.io.getProtocolHandler(uri.scheme).defaultPort` could be used. I will update the patch tomorrow.

Using the default port when no port is given sounds reasonable, and we should indeed update the specification to include that information.

I understood your explanation of why :216 is never hit in the HTTPS case.  Resolving this issue, because it tracks that specifically.  Looking forward to your other update.
With https://github.com/w3c/webdriver/pull/1054 fixed we have to implement the following for socks proxies now:

> If the port is omitted and `scheme` has a default port, this is the implied port. Otherwise, the port is left undefined.

Given that socks doesn't have a default port we have to make sure that we return the following:

`foo` => `foo`
`foo:0` => `foo` (0 is invalid for Firefox)

I will update the patch in a moment to ensure that this is what we do.
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review179526

Looking good now!  Happy with the way this turned out.

::: testing/marionette/session.js:226
(Diff revision 11)
> +        throw new InvalidArgumentError(e.message);
> +      }
> +
> +      // If the port hasn't been set, use the default port of
> +      // the selected scheme (except for socks which doesn't have one).
> +      var port = parseInt(url.port);

let for correct scoping.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1395176 to add eslint rule for this.
Attachment #8897398 - Flags: review?(ato) → review+
Comment on attachment 8897398 [details]
Bug 1369827 - Make proxy port an optional suffix for the host.

https://reviewboard.mozilla.org/r/168716/#review179526

> let for correct scoping.
> 
> Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1395176 to add eslint rule for this.

Sorry, that's a left-over copy from scratchpad hacking. I will fix it.
Before I push to autoland I triggered another try job to verify that everything is still fine on other platforms than MacOS. 

Thank you Andreas for the in-depth review!
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/575d49903c8b
Upgrade webdriver crate to 0.30. r=ato
https://hg.mozilla.org/integration/autoland/rev/9c97da1f0cb3
Vendoring in webdriver 0.30. r=ato
https://hg.mozilla.org/integration/autoland/rev/c5268a608c2d
Make proxy port an optional suffix for the host. r=ato
https://hg.mozilla.org/integration/autoland/rev/246e008032d0
Fix socksVersion key in proxy capabilities. r=ato
https://hg.mozilla.org/integration/autoland/rev/fd57f9313456
Update geckodriver docs for proxy port changes. r=ato
Hi Valentin, I have a question in regards this topic and ipv6 addresses. How should IPv6 addresses actually be specified for proxy hosts? Is it with square brackets like `[::1]`, or `::1`?  I cannot really tell because we don't seem to have any sane documentation for that topic, and https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Mozilla_networking_preferences#Proxy only says: "IPv6 needs to be tested.". Thanks.
Flags: needinfo?(valentin.gosu)
I'm basing my response on these pieces of code:

proxyInfo.GetHost will return the same string that was set in the pref:
http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/netwerk/base/nsProtocolProxyService.cpp#733,2048,2079

And here service gets set to either proxyInfo.GetHost or nsIURI.GetAsciiHost
http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/extensions/auth/nsHttpNegotiateAuth.cpp#166,183,217

nsStandardURL::GetAsciiHost/Host() returns an IPv6 without the brackets...
http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/netwerk/base/nsStandardURL.h#380

... so I assume that in order for it to work properly, the pref should also _not_ have the brackets.

However, since this is rarely tested, it there may be situations where this is broken. It would be great for someone to actually try  it.
Flags: needinfo?(valentin.gosu)
Depends on: 1406763
Thank you Valentin. I filed bug 1406763 to get this fixed.
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: