1295636 - Make clear that SetHostPort will not reset the port if the host parameter does not have a port number and add new function that does the opposite

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

If uri had a port number and the parameter of SetHostPort does not have a port number it should set port number to default. At least the function name suggest that.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1295636.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Baku might recall why SetHostPort behaves as it behaves.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1292522#c9

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781623 [details] [diff] [review]
bug_1295636.patch

But this changes the behavior of most of the users of SetHostPort.
Would it make sense to have enum param for SetHostPort telling whether missing port means reseting to default, or whether the old port should be reused?

Comment 4

[Andrea Marchesini [:baku]]

We introduced SetHostPort for URL API. In this API if you do:

var a = new URL("http://example.com:443");
a.host = "example.net";
alert(a);

It will should show: "http://example.net:443".

The spec says:

The host attribute’s setter must run these steps:
    [...]
If the given value for the host attribute’s setter lacks a port, context object’s url’s port will not change. This can be unexpected as host attribute’s getter does return a port so one might have assumed the setter to always "reset" both.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Yeah, that is about URL spec, but SetHostPort is about our internal URL, and based on bugs its current meaning isn't clear.
But ok, URL API needs SetHostPorts current behavior, but ::SetDomain needs different behavior.
And nothing in the name SetHostPort hints that it keeps the old port.
Either we need to rename the method or add some enum param or something.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1295636.patch

I have added a new function and some comments so that we do not get another bug because of this confusing function name.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1295636.patch

Sorry, I have only change some comments.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781899 [details] [diff] [review]
bug_1295636.patch

I guess fine. Still error prone, but I see that hostPort is attribute after all, so passing enum isn't possible.

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8781899 [details] [diff] [review]
> bug_1295636.patch
> 
> I guess fine. Still error prone, but I see that hostPort is attribute after
> all, so passing enum isn't possible.

That is why I did it this way.

At least now there is a comment :(

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8781899 [details] [diff] [review]
bug_1295636.patch

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

::: netwerk/base/nsStandardURL.cpp
@@ +1847,5 @@
>      return NS_OK;
>  }
>  
> +// This function is different than SetHostPort in that the port number will be
> +// reset as well if aValue parameter does not contain a port port number. 

Trailing whitespace.

@@ +1857,5 @@
> +  nsresult rv = SetPort(-1);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = SetHostPort(aValue);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +}

Wouldn't you get a "not all control paths return a value" warning here?
This is why I don't like NS_ENSURE_SUCCESS very much.
Just return rv here.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1295636.patch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1627e53676b
SetHostPort should reset the port if the host parameter does not have a port number. r=valentin, r=smaug

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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