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

Status: RESOLVED FIXED

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

Description

[Hallvord R. M. Steen [:hallvors]]

Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-url-bogus.htm

Comment 1

[Josh Matthews [:jdm]]

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...

Comment 2

[Josh Matthews [:jdm]]

However, since http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/XMLHttpRequest/open-url-bogus.htm.ini exists we appear to not treat this as a malformed URI...

Comment 3

[Thomas Wisniewski]

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.

Comment 4

[Josh Matthews [:jdm]]

I think I suggested the wrong shortform for Valentin, sorry.

Comment 5

[Josh Matthews [:jdm]]

Actually Thomas wanted to work on this bug, hence the needinfo.

Comment 6

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

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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Let's fix the test bug, at least? "//[" should probably work.

Comment 8

[Simon Pieters [:zcorpan]]

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

Comment 9

[Thomas Wisniewski]

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.

Comment 10

[Thomas Wisniewski]

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:////////////").

Comment 11

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

Attachment: Remove unneeded expected fail in open-url-bogus.htm.ini

This does not appear to be tested in this test.

MozReview-Commit-ID: JCTK6BTiJSg

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52fbe8ec8c07
Remove unneeded expected fail in open-url-bogus.htm.ini. r=jdm

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52fbe8ec8c07