1406763 - ipv6 literals for proxy hosts and noProxy have to be stored without square brackets

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As noticed in https://github.com/mozilla/geckodriver/issues/981 we do not correctly store the ipv6 literals for proxy hosts in the preferences. 

A proxy host definition like `[::1]:8080` should be added as host `::1` and port `8080`.

See also bug 1369827 comment 92 for the investigation from Valentin.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Btw. the same also applies to noProxy. I have a working patch locally which I will push later.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406763 - Refactor xpcshell proxy tests for fromJson and toJson.

To ensure that both methods are working as expected, they cannot be
used in combination for assertions.

Review commit: https://reviewboard.mozilla.org/r/188702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188702/

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

The WebDriver specification requires IPv6 addresses to be always added
with brackets for proxy hosts. But Firefox itself handles those without
brackets for both the proxy host, and the noProxy settings.

Review commit: https://reviewboard.mozilla.org/r/188704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188704/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917764 [details]
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188704/diff/1-2/

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8917763 [details]
Bug 1406763 - Refactor xpcshell proxy tests for fromJson and toJson.

https://reviewboard.mozilla.org/r/188702/#review194004

::: testing/marionette/test_session.js:232
(Diff revision 1)
> -  deepEqual({}, session.Proxy.fromJSON(undefined).toJSON());
> -  deepEqual({}, session.Proxy.fromJSON(null).toJSON());
> +  let p = new session.Proxy();
> +  deepEqual(p, session.Proxy.fromJSON(undefined));
> +  deepEqual(p, session.Proxy.fromJSON(null));

I had to look up what deepEqual [1] does, and this is OK I think.

  [1] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/toolkit/modules/ObjectUtils.jsm#123

::: testing/marionette/test_session.js:247
(Diff revision 1)
> +    Assert.throws(() => session.Proxy.fromJSON(
> +        {proxyType: "pac", proxyAutoconfigUrl: url}), InvalidArgumentError);

Apparently, and I only learnt this the other week, the last argument
to Assert.throws must be a regular expression.  I think this will
work if you put do /InvalidArgumentError/.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8917764 [details]
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

https://reviewboard.mozilla.org/r/188704/#review194006

::: commit-message-42472:1
(Diff revision 2)
> +Bug 1406763 - Strip brackes around IPv6 addresses for poxy hosts.

s/brackes/brackets/

::: commit-message-42472:1
(Diff revision 2)
> +Bug 1406763 - Strip brackes around IPv6 addresses for poxy hosts.

s/poxy/proxy/

::: testing/marionette/session.js:288
(Diff revision 2)
>            let entries = assert.array(json.noProxy);
> -          for (let entry of entries) {
> +          p.noProxy = entries.map(entry => {
>              assert.string(entry);
> +
> +            // Strip brackets from IPv6 addresses
> +            return entry.replace(/[\[\]]/g, "");

Maybe pull this out into a helper function so we don’t repeat the expression?

const stripBrackets = host => host.replace(/[\[\]]/g, "");

…

entries.map(assert.string);
p.noProxy = entries.map(stripBrackets);

::: testing/marionette/session.js:323
(Diff revision 2)
> +    let excludes = this.noProxy;
> +    if (excludes != null) {

I think we could save ourselves the weak null comparison here?

> if (excludes) { … }

::: testing/marionette/session.js:326
(Diff revision 2)
> +        // Add brackets around IPv6 addresses
> +        return entry.includes(":") ? `[${entry}]` : entry;

This is also repeated above.  Maybe pull this out to a helper?

Comment 7

[Andreas Tolfsen ❲:ato❳]

I was initially a bit concerned about the brittleness of regular
expressions and string.includes("[") calls, but I guess ":", "[",
and "]" are reserved characters in URLs and these always represent
either IPv6 addresses or ports.

Thanks for fixing this!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917763 [details]
Bug 1406763 - Refactor xpcshell proxy tests for fromJson and toJson.

https://reviewboard.mozilla.org/r/188702/#review194004

> Apparently, and I only learnt this the other week, the last argument
> to Assert.throws must be a regular expression.  I think this will
> work if you put do /InvalidArgumentError/.

Ouch! This will indeed be the reason for those strange error messages (class constructors must be invoked with new) in case of test failures. I fixed all the cases in this file, and filed bug 1408042 as follow-up for all other test files.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917764 [details]
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

https://reviewboard.mozilla.org/r/188704/#review194006

> Maybe pull this out into a helper function so we don’t repeat the expression?
> 
> const stripBrackets = host => host.replace(/[\[\]]/g, "");
> 
> …
> 
> entries.map(assert.string);
> p.noProxy = entries.map(stripBrackets);

Done.

> I think we could save ourselves the weak null comparison here?
> 
> > if (excludes) { … }

This will work, yes. But only because we do not want to add this entry for an empty list.

> This is also repeated above.  Maybe pull this out to a helper?

Done.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406763 - Fix broken Assert.throws() calls for error class.

Review commit: https://reviewboard.mozilla.org/r/188820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188820/

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917763 [details]
Bug 1406763 - Refactor xpcshell proxy tests for fromJson and toJson.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188702/diff/1-2/

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917764 [details]
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188704/diff/2-3/

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8917890 [details]
Bug 1406763 - Fix broken Assert.throws() calls for error class.

https://reviewboard.mozilla.org/r/188820/#review194136

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7f4ffe10569
Refactor xpcshell proxy tests for fromJson and toJson. r=ato
https://hg.mozilla.org/integration/autoland/rev/0ce17eb985f1
Strip brackets around IPv6 addresses for proxy hosts. r=ato
https://hg.mozilla.org/integration/autoland/rev/b85be3749e70
Fix broken Assert.throws() calls for error class. r=ato

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for linting failure at testing/marionette/session.js:317: Missing semicolon:

https://hg.mozilla.org/integration/autoland/rev/6e77e4951c77eb8462702555e45b063dc73b4199
https://hg.mozilla.org/integration/autoland/rev/da7917089c4ba756a1c5416dc8d1a8afaa3ae20f
https://hg.mozilla.org/integration/autoland/rev/98ae3bf088ce6b140702124d19f203e573627993

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b85be3749e704fe1037556e9eea72dfd0ab8c7e2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136580481&repo=autoland

>  TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/marionette/session.js:317:53 | Missing semicolon. (semi)

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The last try build clearly showed it but I missed it because the results section in mozreview doesn't seem to get auto-updated. I will get it fixed and relanded.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917764 [details]
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188704/diff/3-4/

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8917890 [details]
Bug 1406763 - Fix broken Assert.throws() calls for error class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188820/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8337396947a
Refactor xpcshell proxy tests for fromJson and toJson. r=ato
https://hg.mozilla.org/integration/autoland/rev/633b06e5fc74
Strip brackets around IPv6 addresses for proxy hosts. r=ato
https://hg.mozilla.org/integration/autoland/rev/ee681d6964d3
Fix broken Assert.throws() calls for error class. r=ato

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d8337396947a
https://hg.mozilla.org/mozilla-central/rev/633b06e5fc74
https://hg.mozilla.org/mozilla-central/rev/ee681d6964d3

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I verified that those changes are working fine locally after setting up a IPv6 network and squid proxy. So please uplift the test-only patch to beta. Thanks.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ec4cc626311
https://hg.mozilla.org/releases/mozilla-beta/rev/64117629c89a
https://hg.mozilla.org/releases/mozilla-beta/rev/503771cc0879