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

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: annevk, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla29
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 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.

Comment 1

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

Comment 2

6 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.
(Assignee)

Comment 3

6 years ago
Posted patch port0.patch (obsolete) — Splinter Review
Attachment #8342585 - Flags: review?(ehsan)

Comment 4

6 years ago
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?

Updated

6 years ago
Flags: needinfo?(amarchesini)
(Assignee)

Comment 5

6 years ago
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 6

6 years ago
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/mozilla-central/rev/53f77d87a3ab
Assignee: nobody → amarchesini
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Comment 9

5 years ago
Posted patch p.patch (obsolete) — Splinter Review
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
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Keywords: dev-doc-needed
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Posted patch p.patch (obsolete) — Splinter Review
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?
(Assignee)

Comment 13

5 years ago
> 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...
(Assignee)

Comment 16

5 years ago
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-
(Assignee)

Comment 21

5 years ago
Posted patch p.patch (obsolete) — Splinter Review
Attachment #8351260 - Attachment is obsolete: true
Attachment #8355778 - Flags: review?(honzab.moz)
(Assignee)

Comment 22

5 years ago
> ' 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+
https://hg.mozilla.org/mozilla-central/rev/ff96fec5d593
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.