Prevent url.hostname from clearing the hostname

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Currently the following code clears the hostname for a url.
  var url = new URL("http://localhost/");
  url.hostname = "";
  alert(url.href); // returns "http:///"

From what I understand, setting an empty hostname should only be allowed for URLTYPE_NO_AUTHORITY.
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#1462

In Chromium, this code does not clear the hostname, and I think we should do the same. Our implementation of the url parser sometimes assumes the hostname is not null, and we've had to patch it a few times.

I think we should also emulate this behaviour, for URLTYPE_STANDARD and URLTYPE_AUTHORITY and have url.hostname = "" be a no-op.
Anne, do you think this is the behaviour we should aim for?
Flags: needinfo?(annevk)

Comment 2

5 years ago
Per http://url.spec.whatwg.org/#dom-url-hostname it is, yes.
Flags: needinfo?(annevk)
Assignee: nobody → valentin.gosu
Comment on attachment 8407927 [details] [diff] [review]
Prevent url.hostname from clearing the hostname

The patch contains makes SetHost return an error in case the hostname is empty.
Also includes test for bug 991471, as the old xpcshell test would fail with this change.
Attachment #8407927 - Flags: review?(mcmanus)
Comment on attachment 8407927 [details] [diff] [review]
Prevent url.hostname from clearing the hostname

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

::: dom/base/test/test_url.html
@@ -1,5 @@
>  
>  <!DOCTYPE HTML>
>  <html>
> -<!--
> -https://bugzilla.mozilla.org/show_bug.cgi?id=887364

I don't understand this change. help?

::: netwerk/base/src/nsStandardURL.cpp
@@ +1467,5 @@
> +    } else {
> +        if (flat.IsEmpty()) {
> +            // Setting an empty hostname is not allowed for
> +            // URLTYPE_STANDARD and URLTYPE_AUTHORITY.
> +            return NS_ERROR_UNEXPECTED;

comment 0 says this should be a nop, but this will throw an exception. which should it be?
Attachment #8407927 - Flags: review?(mcmanus)
In this case it's an exception in necko code, but a no-op in dom code (as the error code isn't checked)
Addressing comment I missed

(In reply to Patrick McManus [:mcmanus] from comment #5)
> > -<!--
> > -https://bugzilla.mozilla.org/show_bug.cgi?id=887364
> 
> I don't understand this change. help?
> 

test_url.html was used to check for only one bug/test. I added a few others, so I assumed that comment wasn't necessary anymore. Should I leave it?
Flags: needinfo?(mcmanus)
(In reply to Valentin Gosu [:valentin] from comment #7)
> Addressing comment I missed
> 
> (In reply to Patrick McManus [:mcmanus] from comment #5)
> > > -<!--
> > > -https://bugzilla.mozilla.org/show_bug.cgi?id=887364
> > 
> > I don't understand this change. help?
> > 
> 
> test_url.html was used to check for only one bug/test. I added a few others,
> so I assumed that comment wasn't necessary anymore. Should I leave it?

Thanks. I was mentally blocked expecting to see the changes to the test in that patch not just the comment change.
Flags: needinfo?(mcmanus)
Comment on attachment 8407927 [details] [diff] [review]
Prevent url.hostname from clearing the hostname

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

::: netwerk/base/src/nsStandardURL.cpp
@@ +1467,5 @@
> +    } else {
> +        if (flat.IsEmpty()) {
> +            // Setting an empty hostname is not allowed for
> +            // URLTYPE_STANDARD and URLTYPE_AUTHORITY.
> +            return NS_ERROR_UNEXPECTED;

just add a comment about this exception being ignored in at least one important place up the stack
Attachment #8407927 - Flags: review+

Comment 10

5 years ago
Valentin, thanks for doing this! There's a couple of other small URL bugs such as bug 960014 you might be interested in fixing.
Blocks: url
Attachment #8407927 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c223611fa19e
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.