Closed Bug 1406763 Opened 6 years ago Closed 6 years ago

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

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
57 Branch
Type:
enhancement

Tracking

(firefox57 fixed, firefox58 fixed)

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug,
URL
)

Details

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

Attachments

(3 files)

Bug 1406763 - Refactor xpcshell proxy tests for fromJson and toJson.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1406763 - Strip brackets around IPv6 addresses for proxy hosts.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1406763 - Fix broken Assert.throws() calls for error class.
59 bytes, text/x-review-board-request
ato
: review+
Details 
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.
Btw. the same also applies to noProxy. I have a working patch locally which I will push later.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: ipv6 literals for proxy hosts have to be stored without square brackets → ipv6 literals for proxy hosts and noProxy have to be stored without square brackets
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/.
Attachment #8917763 - Flags: review?(ato) → review+
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?
Attachment #8917764 - Flags: review?(ato) → review+
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 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 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 on attachment 8917890 [details]
Bug 1406763 - Fix broken Assert.throws() calls for error class.

https://reviewboard.mozilla.org/r/188820/#review194136
Attachment #8917890 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
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
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.
Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
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
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.
Status: RESOLVED → VERIFIED
Whiteboard: [spec][17/10] → [spec][17/10][checkin-needed-beta]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.