Closed
Bug 996055
Opened 10 years ago
Closed 10 years ago
Prevent url.hostname from clearing the hostname
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: valentin, Assigned: valentin)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.68 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Anne, do you think this is the behaviour we should aim for?
Flags: needinfo?(annevk)
Comment 2•10 years ago
|
||
Per http://url.spec.whatwg.org/#dom-url-hostname it is, yes.
Flags: needinfo?(annevk)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → valentin.gosu
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
In this case it's an exception in necko code, but a no-op in dom code (as the error code isn't checked)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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•10 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
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8407927 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2c276c3aec8a
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c223611fa19e
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c223611fa19e
Status: NEW → RESOLVED
Closed: 10 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.
Description
•