Closed Bug 930450 Opened 11 years ago Closed 10 years ago

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.

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: annevk, Assigned: baku)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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?)
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.
Attached patch port0.patch (obsolete) — Splinter Review
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/mozilla-central/rev/53f77d87a3ab
Assignee: nobody → amarchesini
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attached 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
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.
Attached 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?
> 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-
Attached patch p.patch (obsolete) — Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/ff96fec5d593
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.