[XHR2] open('GET', 'http:') should throw because the URL is invalid

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: hsteen, Assigned: valentin, Mentored)

Tracking

unspecified
mozilla54
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(URL)

Attachments

(1 attachment)

Comment 1

2 years ago
Take a look at nsXMLHttpRequest::Open in dom/base/nsXMLHttpRequest.cpp .
>1568   rv = NS_NewURI(getter_AddRefs(uri), url, nullptr, baseURI);
>1569 
>1570   if (NS_FAILED(rv)) {
>1571     if (rv ==  NS_ERROR_MALFORMED_URI) {
>1572       return NS_ERROR_DOM_SYNTAX_ERR;
>1573     }
>1574     return rv;
>1575   }
This makes me think it should already work...
Mentor: josh

Comment 3

2 years ago
Basically nsStandardURL::SetSpec considers it valid, setting the spec to the base URI of the document, and the protocol as http. It doesn't seem to check if there's a :// after the protocol for completeness, but I'm not sure if that's a good fix. Pinging valenting as per our discussion on IRC.
Flags: needinfo?(valentingratz.lpvoltaire)

Comment 4

2 years ago
I think I suggested the wrong shortform for Valentin, sorry.
Flags: needinfo?(valentingratz.lpvoltaire) → needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]

Comment 5

2 years ago
Actually Thomas wanted to work on this bug, hence the needinfo.
Flags: needinfo?(valentin.gosu)
I was just about to close the bug as invalid.
The problem here is that "http:" is not an absolute URL.
"http://" would be an absolute URL.

For example:
> new URL("http:foo.com", new URL("http://example.org/bar/")).href
> -> "http://example.org/bar/foo.com"

> new URL("http:", new URL("http://example.org/bar/")).href
> -> "http://example.org/bar/"

Side note: We do have a bug in our code as the following 2 calls should fail.
> new URL("http:") // without a base
> new URL("http://")
I filed bug 1275746 for that.

However, xhr.open will always use the current page as a base, so this test should be fixed. It also seems that all UAs fail this test - which is OK, since it's wrong.
Assignee: valentin.gosu → nobody
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]
Let's fix the test bug, at least? "//[" should probably work.

Comment 8

2 years ago
PR to fix the test at https://github.com/w3c/web-platform-tests/pull/3073

Comment 9

2 years ago
This bug has been fixed, partly by the patch in bug 1275746, and partly by the fact that the open-url-bogus tests have since been amended.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Depends on: 1275746
Resolution: --- → FIXED

Comment 10

2 years ago
The fixes in bug 1275746 were just backed out in bug 1305204, so two of the tests are failing again here (opening "ftp:" and "http:////////////").
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8835217 [details] [diff] [review]
Remove unneeded expected fail in open-url-bogus.htm.ini

This does not appear to be tested in this test.

MozReview-Commit-ID: JCTK6BTiJSg
Attachment #8835217 - Flags: review?(josh)
Assignee: nobody → valentin.gosu
Status: REOPENED → ASSIGNED

Updated

2 years ago
Attachment #8835217 - Flags: review?(josh) → review+
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fbe8ec8c07
Remove unneeded expected fail in open-url-bogus.htm.ini. r=jdm
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52fbe8ec8c07
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.