930450 - Setting port to the empty string should set URL's port to the default value. Setting it to "0" should set URL's port to 0.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=23535 for rationale.

This should work across URL / Location / HTMLAnchorElement / HTMLAreaElement. I.e. across URLUtils.

Comment 1

[(no longer active)]

Is this change going to be web compatible?  (I guess we need to see?)

Comment 2

[Anne (:annevk)]

Per arv in that bug it is fully Chrome-compatible. And apart from Location.port it is compatible with all other browsers. And we used to throw for Location.port so that seems like low-risk. However, if there are Gecko-specific code paths out there on the web we could be in trouble, as always.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: port0.patch

Comment 4

[(no longer active)]

Comment on attachment 8342585 [details] [diff] [review]
port0.patch

Review of attachment 8342585 [details] [diff] [review]:
-----------------------------------------------------------------

Why are you setting the port to -1 by default?

Comment 5

[Andrea Marchesini [:baku]]

https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#1533

-1 means: no port set, so GetPort will return 0.

Comment 6

[(no longer active)]

Comment on attachment 8342585 [details] [diff] [review]
port0.patch

Review of attachment 8342585 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  Please document what this -1 means.  r=me with that.

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53f77d87a3ab

comment added.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53f77d87a3ab

Comment 9

[Andrea Marchesini [:baku]]

Attachment: p.patch

This patch changes nsStandardURL. Scary...
https://tbpl.mozilla.org/?tree=Try&rev=3d7f17536289

bz, can you or somebody else review this changes (if green on try) ?

Comment 10

[Andrea Marchesini [:baku]]

Attachment: p.patch

https://tbpl.mozilla.org/?tree=Try&rev=53bcf54504c7

It's not too bad. Boris, can you review this patch?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8351260 [details] [diff] [review]
p.patch

>+  ok(url.port === '', 'URL.port is \'\'');

Use ise() instead, please.

r=me, though I'd like to have Honza look at this too.

Does this fix the behavior of location.port too, or does that need a separate bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8351260 [details] [diff] [review]
p.patch

Oh, and maybe add tests for <a> and location?

Comment 13

[Andrea Marchesini [:baku]]

> Does this fix the behavior of location.port too, or does that need a
> separate bug?

It covers location as well. <a> is tested. Testing location it's hard. Any time it changes, the content is changed as well and it's hard to keep track of the URL.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Testing location it's hard

Well, you'd have to do it in a subframe.  But OK.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Wait.  Where is <a> tested?  I don't see it in the patch...

Comment 16

[Andrea Marchesini [:baku]]

test_url_empty_port.html:

<a id="link" href="http://www.example.com:8080">foobar</a>
var link = document.getElementById("link");
is(link.port, 8080, 'URL.port is 8080');
...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  I thought that was testing a <link>.  Alright, then!

Comment 18

[Honza Bambas (:mayhemer)]

Looking into the patch...  Needs a bit more thinking.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8351260 [details] [diff] [review]
p.patch

Review of attachment 8351260 [details] [diff] [review]:
-----------------------------------------------------------------

I think with this patch nsIOService::AllowPort [1] should return false for port 0.

[1] http://hg.mozilla.org/mozilla-central/annotate/2cadd4910e5e/netwerk/base/src/nsIOService.cpp#l760

r+ with the comments addressed.

::: dom/base/test/test_url_empty_port.html
@@ +25,5 @@
>  
>    var url = new URL('http://www.example.com:8080');
>    is(url.port, 8080, 'URL.port is 8080');
>    url.port = '';
> +  ok(url.port === '', 'URL.port is \'\'');

but the spec says:

' If the given value is the empty string, set url's port to "0". '  (http://url.spec.whatwg.org/, [2])

I personally don't agree with the spec.  What we do here (with this patch) is IMO better and more logical.  What we do is that the actual port number after |url.port = ''| is 80, since internally it's at -1 ('use default port', see [1]) and according the scheme it is then converted to 80 when we actually create connection to the host.

[1] http://hg.mozilla.org/mozilla-central/annotate/2cadd4910e5e/dom/base/URL.cpp#l358
[2] https://github.com/whatwg/url/commit/53dae545e386f26208b8038de55a918ffe421ac8

::: netwerk/base/src/nsStandardURL.cpp
@@ +514,5 @@
>          // :password - we insert the ':' even if there's no actual password if "user:@" was in the spec
>          if (mPassword.mLen >= 0)
>              approxLen += 1 + encoder.EncodeSegmentCount(spec, mPassword,  esc_Password,      encPassword,  useEncPassword);
>          // mHost is handled differently below due to encoding differences
> +        NS_ABORT_IF_FALSE(mPort >= 0 || mPort == -1, "Invalid negative mPort");

just (mPort >= -1) ?

@@ +1529,5 @@
>      if ((port == mPort) || (mPort == -1 && port == mDefaultPort))
>          return NS_OK;
>  
> +    // ports must be >= 0
> +    if (port < 0 && port != -1) // -1 == use default

(port < -1) ?

// -1 == use default and 0 is allowed

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8351260 [details] [diff] [review]
p.patch

Actually, I'd like to check the next version quickly.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: p.patch

Comment 22

[Andrea Marchesini [:baku]]

> ' If the given value is the empty string, set url's port to "0". ' 
> (http://url.spec.whatwg.org/, [2])
> 
> I personally don't agree with the spec.  What we do here (with this patch)
> is IMO better and more logical.  What we do is that the actual port number
> after |url.port = ''| is 80, since internally it's at -1 ('use default
> port', see [1]) and according the scheme it is then converted to 80 when we
> actually create connection to the host.

Yes. we are moving in this direction with this patch. The spec will be updated once this patch is landed.
Basically the idea is url.port = '' => default port. url.port = '0' => port == 0.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8355778 [details] [diff] [review]
p.patch

Review of attachment 8355778 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
r=honzab but please push to try first before landing.

::: netwerk/base/src/nsIOService.cpp
@@ +772,1 @@
>          

nit: when you are here, please remove the whitespace

::: netwerk/base/src/nsStandardURL.cpp
@@ +587,5 @@
>      }
>      if (mHost.mLen > 0) {
>          i = AppendSegmentToBuf(buf, i, spec, mHost, &encHost, useEncHost);
>          net_ToLowerCase(buf + mHost.mPos, mHost.mLen);
> +        NS_ABORT_IF_FALSE(mPort >= 0 || mPort == -1, "Invalid negative mPort");

mPort >= -1 here as well ;)

Comment 24

[Andrea Marchesini [:baku]]

Attachment: p.patch

https://tbpl.mozilla.org/?tree=Try&rev=8a09ca8cdeb8

Comment 25

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff96fec5d593

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff96fec5d593

Comment 27

[Jean-Yves Perrier [:teoli]]

Doc updated:
I've added a line to describe this case: https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.port
and
https://developer.mozilla.org/en-US/Firefox/Releases/29