URL constructor doesn't parse search params of custom protocol

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: merihakar, Assigned: baku)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

3 years ago
Posted file 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"

Updated

3 years ago
Component: General → DOM: Core & HTML
Product: Firefox → Core
Baku, can you check what the spec says here?
Flags: needinfo?(amarchesini)
Assignee

Comment 3

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

Our implementation is wrong.
Flags: needinfo?(amarchesini)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Assignee

Comment 4

3 years ago
Posted patch URL.patch (obsolete) — Splinter Review
Attachment #8806799 - Flags: feedback?(valentin.gosu)
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 #
Attachment #8806799 - Flags: feedback?(valentin.gosu) → feedback+
Assignee

Comment 6

3 years ago
Posted patch URL.patch (obsolete) — Splinter Review
Attachment #8806799 - Attachment is obsolete: true
Attachment #8808231 - Flags: review?(valentin.gosu)
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"
Attachment #8808231 - Flags: review?(valentin.gosu) → feedback+
Assignee

Comment 8

3 years ago
Posted patch URL.patch (obsolete) — Splinter Review
> 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.
Attachment #8808231 - Attachment is obsolete: true
Attachment #8808595 - Flags: review?(valentin.gosu)
Assignee

Comment 9

3 years ago
Posted patch URL.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8808595 - Attachment is obsolete: true
Attachment #8808595 - Flags: review?(valentin.gosu)
Attachment #8808596 - Flags: review?(valentin.gosu)
Assignee

Comment 10

3 years ago
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
Assignee

Comment 11

3 years ago
Posted patch URL.patch (obsolete) — Splinter Review
Attachment #8808596 - Attachment is obsolete: true
Attachment #8808596 - Flags: review?(valentin.gosu)
Attachment #8809449 - Flags: review?(valentin.gosu)
Assignee

Comment 12

3 years ago
Posted patch URL.patchSplinter Review
Check the test_url.html comments. There is 1 thing to fix, but it can be a follow up.
Attachment #8809449 - Attachment is obsolete: true
Attachment #8809449 - Flags: review?(valentin.gosu)
Attachment #8809620 - Flags: review?(valentin.gosu)
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.
Attachment #8809620 - Flags: review?(valentin.gosu) → review+

Comment 14

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef4a0fdca31
Implement nsIURIWithQuery for having query part in simple URI, r=valentin

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aef4a0fdca31
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1323683
Depends on: 1326520
Assignee

Comment 16

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

This patch must be uplifted because it's needed for bug 1321719.
Attachment #8809620 - Flags: approval-mozilla-aurora?
Comment on attachment 8809620 [details] [diff] [review]
URL.patch

aurora52+ as prerequisite for bug 1321719.
Attachment #8809620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1327960
wontfix for beta 51, it seems like a pretty big change along with the work in the other bug.
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!
Reporter

Comment 21

2 years ago
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.

Updated

2 years ago
Depends on: 1344558
You need to log in before you can comment on or make changes to this bug.