1310483 - URL constructor doesn't parse search params of custom protocol

Status: RESOLVED FIXED

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

Description

[Merih Akar]

Attachment: Testcase

Given custom protocol, URL contructor doesn't understand the search parameters part of the URL. For example, in Firefox you get the following:

> var u = new URL("doge:amaze?wow=true&such=false");
> u.protocol
> // "doge:"
> u.pathname
> // "amaze?wow=true&such=false"
> u.search
> // ""

whereas in Chrome, pathname and search part of the URL correctly extracted:

> u.pathname
> // "amaze"
> u.search
> // "?wow=true&such=false"

Comment 1

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

Baku, can you check what the spec says here?

Comment 2

[Junior [inactive]]

FWIW, nsSimpleURI only separates the spec to protocol, path and hash.

http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/netwerk/base/nsSimpleURI.cpp#203

Comment 3

[Andrea Marchesini [:baku]]

Spec says: If c is "?", set url’s query to the empty string, and state to query state.

Our implementation is wrong.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: URL.patch

Comment 5

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

Comment on attachment 8806799 [details] [diff] [review]
URL.patch

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

Sorry for the late feedback, forgot to submit it.
Found 2 bugs at first glance. Didn't look at the code too closely, but overall it looks good.

::: netwerk/base/nsSimpleURI.cpp
@@ +210,5 @@
>          return NS_ERROR_OUT_OF_MEMORY;
>      }
>  
> +    if (mIsQueryValid) {
> +        if (!result.Append(NS_LITERAL_CSTRING("#"), fallible) ||

? not #

@@ +406,5 @@
>  nsSimpleURI::GetPath(nsACString &result)
>  {
>      result = mPath;
> +    if (mIsQueryValid) {
> +        result += NS_LITERAL_CSTRING("#") + mQuery;

? not #

Comment 6

[Andrea Marchesini [:baku]]

Attachment: URL.patch

Comment 7

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

Comment on attachment 8808231 [details] [diff] [review]
URL.patch

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

There are a few issues with the patch, and it needs a lot more tests.
Also, did you do a try run? I just looking at some of the instances where we QI to nsIURL, and we might be breaking something with this [1]
We can either audit all 84 instances of "<nsIURL>" [2], or create another IDL called nsIURIWithQuery (or similar) that extends nsIURI - adds the filePath attribute, and nsIURL would extend this new IDL. I think this would be more elegant, and less prone to bugs.

[1] http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#124
[2] http://searchfox.org/mozilla-central/search?q=%3CnsIURL%3E&case=false&regexp=false&path=

::: netwerk/base/nsSimpleURI.cpp
@@ +453,5 @@
> +    // The query
> +    if (queryPos > -1) {
> +      nsresult rv =
> +        SetQuery(Substring(path, queryPos,
> +                           path.Length() - (hashPos > -1 ? hashPos : 0)));

This is wrong. It should be something like ((hashPos > -1) ? hashPos : path.Length()) - queryPos)

I caught this with a simple use case:
var url = new URL("scheme://test/bla/stuff?query#ref");
equal(url.search, "?query") # returned ?que
Before landing please add tests covering all branches, including the setters.

@@ +462,5 @@
> +
> +    // The ref
> +    if (hashPos > -1) {
> +      nsresult rv = SetRef(Substring(path, hashPos));
> +      if (NS_WARN_IF(NS_FAILED(rv))) {

Not sure we need the WARN_IF since neither SetQuery of SetRef return an error code except when changing a mutable URI, which can't happen here.

@@ +827,5 @@
> +  // Gracefully skip initial hash
> +  if (aQuery[0] == '?') {
> +      mQuery = Substring(aQuery, 1);
> +  } else {
> +      mQuery = aQuery;

You also need to check if the string includes #
Otherwise:
var url = new URL("scheme://test/bla/stuff?query#bla1");
url.search = "test#ref"
url.search # returns "test#ref"
url.href # returns "scheme://test/bla/stuff?test#ref#bla1"

Comment 8

[Andrea Marchesini [:baku]]

Attachment: URL.patch

> There are a few issues with the patch, and it needs a lot more tests.

The test is included in the patch. I added a test in test_url.html.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: URL.patch

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8808596 [details] [diff] [review]
URL.patch

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

::: netwerk/base/nsSimpleURI.cpp
@@ +440,4 @@
>      }
>  
> +    // Query cannot be after the Ref
> +    if (hashPos > 0 && queryPos > hashPos) {

this must be != -1

Comment 11

[Andrea Marchesini [:baku]]

Attachment: URL.patch

Comment 12

[Andrea Marchesini [:baku]]

Attachment: URL.patch

Check the test_url.html comments. There is 1 thing to fix, but it can be a follow up.

Comment 13

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

Comment on attachment 8809620 [details] [diff] [review]
URL.patch

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

Apart from a few nits, it looks great! r=me

::: dom/url/URL.cpp
@@ +606,2 @@
>    nsCOMPtr<nsIURL> url(do_QueryInterface(mURI));
> +  if (url) {

If it QIs to nsIURL it should definitely QI to nsIURIWithQuery, so you can remove this part.

::: dom/url/tests/test_url.html
@@ +409,5 @@
> +    url.pathname = "new/path?newquery#newhash";
> +    is(url.href, "scheme:path/to/file?query#hash");
> +
> +    url.search = "?newquery#newhash";
> +    // This is actually wrong. The correct result should be: scheme:path/to/file?newquery%23newhash#hash

Good point. It should be easy to fix it here.

@@ +418,5 @@
> +    is(url.href, "scheme:pa%00th/to/fi%00le?qu%00ery#ha%00sh");
> +
> +    // pathname cannot be overwritten.
> +    url.pathname = "new\0\npath";
> +    is(url.href, "scheme:pa%00th/to/fi%00le?qu%00ery#ha%00sh");

We don't need 2 tests that setting the pathname doesn't work :)

::: netwerk/base/nsSimpleURI.cpp
@@ +226,5 @@
>          }
>      } else {
>          MOZ_ASSERT(mRef.IsEmpty(), "mIsRefValid/mRef invariant broken");
>      }
> +

Remove whitespace only line.

@@ +816,5 @@
> +{
> +    NS_ENSURE_STATE(mMutable);
> +
> +    nsAutoCString query;
> +    nsresult rv = NS_EscapeURL(aQuery, esc_OnlyNonASCII, query, fallible);

use esc_Query instead of esc_OnlyNonASCII, and remove the search for hashPos.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef4a0fdca31
Implement nsIURIWithQuery for having query part in simple URI, r=valentin

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8809620 [details] [diff] [review]
URL.patch

This patch must be uplifted because it's needed for bug 1321719.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8809620 [details] [diff] [review]
URL.patch

aurora52+ as prerequisite for bug 1321719.

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

wontfix for beta 51, it seems like a pretty big change along with the work in the other bug.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7d6c64a21d6

Comment 20

[Chris Mills [:cmills]]

I've added a footnote to the compat tables on the relevant ref pages to make people aware of this bug:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils/search
https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils/pathname

I've also added a note to the Fx53 release notes page:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

Let me know if this look OK. Thanks!

Comment 21

[Merih Akar]

Actually, it was working properly for well-known procotols, so the example in the footnotes (http://z.com/x?a=true&b=false) works as expected even before this fix.

Comment 22

[JamesV]

Even after many update, it is still working fine for https://forex.z.com/hk/tc/whyzcomtrade/trustedtrading.html as well.