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.

RESOLVED FIXED in mozilla29

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: annevk, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla29
x86
Mac OS X
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Is this change going to be web compatible?  (I guess we need to see?)
(Reporter)

Comment 2

4 years ago
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.
Created attachment 8342585 [details] [diff] [review]
port0.patch
Attachment #8342585 - Flags: review?(ehsan)
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?
Flags: needinfo?(amarchesini)
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#1533

-1 means: no port set, so GetPort will return 0.
Flags: needinfo?(amarchesini)
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.
Attachment #8342585 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/53f77d87a3ab

comment added.
https://hg.mozilla.org/mozilla-central/rev/53f77d87a3ab
Assignee: nobody → amarchesini
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Created attachment 8350574 [details] [diff] [review]
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) ?
Attachment #8342585 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Keywords: dev-doc-needed
Resolution: FIXED → ---
Summary: Setting port to the empty string should set URL's port to "0" → 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.
Created attachment 8351260 [details] [diff] [review]
p.patch

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

It's not too bad. Boris, can you review this patch?
Attachment #8350574 - Attachment is obsolete: true
Attachment #8351260 - Flags: review?(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?
Attachment #8351260 - Flags: review?(honzab.moz)
Attachment #8351260 - Flags: review?(bzbarsky)
Attachment #8351260 - Flags: review+
Comment on attachment 8351260 [details] [diff] [review]
p.patch

Oh, and maybe add tests for <a> and location?
> 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.
> Testing location it's hard

Well, you'd have to do it in a subframe.  But OK.
Wait.  Where is <a> tested?  I don't see it in the patch...
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');
...
Oh, I see.  I thought that was testing a <link>.  Alright, then!
Looking into the patch...  Needs a bit more thinking.
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
Attachment #8351260 - Flags: review?(honzab.moz) → review+
Comment on attachment 8351260 [details] [diff] [review]
p.patch

Actually, I'd like to check the next version quickly.
Attachment #8351260 - Flags: review+ → review-
Created attachment 8355778 [details] [diff] [review]
p.patch
Attachment #8351260 - Attachment is obsolete: true
Attachment #8355778 - Flags: review?(honzab.moz)
> ' 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 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 ;)
Attachment #8355778 - Flags: review?(honzab.moz) → review+
Created attachment 8355798 [details] [diff] [review]
p.patch

https://tbpl.mozilla.org/?tree=Try&rev=8a09ca8cdeb8
Attachment #8355778 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff96fec5d593
https://hg.mozilla.org/mozilla-central/rev/ff96fec5d593
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.