1327960 - Urlbar doesn't properly unescape ASCII url

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19)
STR_1:
1. Open url   data:text/html,<a href="http://example.org/?q">Link</a>

AR:  Text in urlbar "data:text/html,<a href="http://example.org/?q"%3ELink%3C/a%3E"
ER:  Text in urlbar "data:text/html,<a href="http://example.org/?q">Link</a>"

This is a regresion. Firefox 28 is unaffected, Nightly 2016-11-19 is affected.

Comment 1

[Stefan [:StefanG_QA]]

I was able to reproduce the issue on latest Nightly with the provided STR, but not in the latest FF release nor in the Aurora release.
I've used mozregression tool to narrow down the issue. Please find below the regression range where the issue was introduced.

Last good revision: 5e76768327660437bf3486554ad318e4b70276e1
First bad revision: 8921f4d2c3eed453dc90e58617d4ca7dd4ebeede
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e76768327660437bf
3486554ad318e4b70276e1&tochange=8921f4d2c3eed453dc90e58617d4ca7dd4ebeede

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1316450

Comment 2

[Marco Bonardo [:mak]]

(In reply to Stefan [:StefanG_QA] from comment #1)
> Looks like the following bug has the changes which introduced the regression:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1316450

Is this 100% confirmed? that bug doesn't look strictly related to encoding/decoding in the location bar and the pushlog is quite large.
It would sound more likely something like bug 1321705...

Could you please check again the regression range?

Comment 3

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: regression in urlbar

Comment 4

[Marcia Knous [:marcia]]

Tracking 53+ for this URL bar regression.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Marco Bonardo [::mak] from comment #2)
> Is this 100% confirmed? that bug doesn't look strictly related to
> encoding/decoding in the location bar and the pushlog is quite large.
> It would sound more likely something like bug 1321705...

Bug 1321705 only removed default-true configurations and assumed they are always true. It should not change behavior at all. Moreover, the reporter uses 20161119 Nightly whereas bug 1321705 has been fixed Dec 30.

Comment 6

[Masatoshi Kimura [:emk]]

I guess bug 1310483 caused the regression.

Comment 7

[Masatoshi Kimura [:emk]]

Bug 1310483 introduced escaping some characters of the query part. It follows the URL spec, but no other browsers support this behavior yet.

But even if we keep the new behavior, urlbar should display the decoded characters. It does for http(s) URLs. Why it doesn't for data URLs?

Comment 8

[Masatoshi Kimura [:emk]]

Hey, url.spec.whatwg.org is down.

Comment 9

[Masatoshi Kimura [:emk]]

Hm, actually we don't follow the spec. <"> should also be escaped, but we don't. Moreover, our current behavior is inconsistent. We only encode characters in the query part, but the spec requires to encode characters in the path part.

IMO we should revert the behavior at least until we migrate to RustURL.

Comment 10

[Anne (:annevk)]

To be clear, the URL Standard doesn't talk much about UI. How Firefox presents URLs to users is up to Firefox. While for readability decoding might be better, for copy-and-pasting and preserving the URL not doing that is likely better. Either way, most users are not going to understand them, so only displaying the host as Safari does is likely best (although what to do for data URLs I'm not sure).

Comment 11

[Masatoshi Kimura [:emk]]

Yeah, but comment #9 is not about UI. Our current URI parser is simply wrong and inconsistent.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Review commit: https://reviewboard.mozilla.org/r/102914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102914/

Comment 13

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

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

https://reviewboard.mozilla.org/r/102914/#review103792

This is a tough one. On one side we have the UI regression, on the other part, we should actually be encoding characters in the query.
The UI already seems to do some unescaping in the URL bar, as the space between `<a href` was parsed as %20.
So this would only be a temporary fix, and once we switched to RustURL we'd still have to solve the UI issue.

FYI, rust parses this as: data:text/html,<a href="http://example.org/?q%22%3ELink%3C/a%3E

I'll take this for now, just to preserve old behavior.
emk, can you add a test so we don't accidentally regress it?

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Valentin Gosu [:valentin] from comment #13)
> Comment on attachment 8824385 [details]
> Bug 1327960 - Don't unescape the query part of nsSimpleURI. .gosu
> 
> https://reviewboard.mozilla.org/r/102914/#review103792
> 
> This is a tough one. On one side we have the UI regression, on the other
> part, we should actually be encoding characters in the query.
> The UI already seems to do some unescaping in the URL bar, as the space
> between `<a href` was parsed as %20.
> So this would only be a temporary fix, and once we switched to RustURL we'd
> still have to solve the UI issue.
> 
> FYI, rust parses this as: data:text/html,<a
> href="http://example.org/?q%22%3ELink%3C/a%3E
> 
> I'll take this for now, just to preserve old behavior.
> emk, can you add a test so we don't accidentally regress it?

Looks like an existing test already covers this:
309 INFO TEST-UNEXPECTED-FAIL | dom/url/tests/test_url.html | undefined assertion name - got "scheme:path/to/file?newquery#newhash#hash", expected "scheme:path/to/file?newquery%23newhash#hash"

Before bug 1310483, the pathname setter (silently) failed when the new value contains a hash character. So we shouldn't land this patch as-is :(

Comment 15

[Masatoshi Kimura [:emk]]

> the pathname setter

Correction: the query setter

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102914/diff/1-2/

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Re-requesting review because this patch changes the behavior since the last r+'ed patch.

Adding a test to make sure the behavior will not change accidentally.

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102914/diff/2-3/

Comment 19

[Stefan [:StefanG_QA]]

Here is the reviewed and updated regression range if still needed:

Last good revision: 5e76768327660437bf3486554ad318e4b70276e1 (2016-11-15)
First bad revision: 79feeed4293336089590320a9f30a813fade8e3c (2016-11-16)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e76768327660437bf
3486554ad318e4b70276e1&tochange=79feeed4293336089590320a9f30a813fade8e3c

Comment 20

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

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

https://reviewboard.mozilla.org/r/102914/#review104140

::: dom/url/tests/test_url.html:415
(Diff revision 3)
>      is(url.href, "scheme:path/to/file?query#hash");
>  
> +    // fail if the new value contains '#' until we implement
> +    // a spec-compliant parser.
>      url.search = "?newquery#newhash";
> -    is(url.href, "scheme:path/to/file?newquery%23newhash#hash");
> +    is(url.href, "scheme:path/to/file?query#hash");

See other comment. It should not fail and the result should be ...?newquery#hash

::: netwerk/base/nsSimpleURI.cpp:828
(Diff revision 3)
>          mIsQueryValid = false;
>          mQuery.Truncate(); // invariant: mQuery should be empty when it's not valid
>          return NS_OK;
>      }
>  
> +    if (query.FindChar('#') != kNotFound) {

I don't think failing is the right move here. If we're not going to encode it (as Safari does) or leave it like it is (as Chrome does, leading to a URL with two hashes in it), we should at least cut off the hash, instead of failing.

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Valentin Gosu [:valentin] from comment #20)
> ::: dom/url/tests/test_url.html:415
> (Diff revision 3)
> >      is(url.href, "scheme:path/to/file?query#hash");
> >  
> > +    // fail if the new value contains '#' until we implement
> > +    // a spec-compliant parser.
> >      url.search = "?newquery#newhash";
> > -    is(url.href, "scheme:path/to/file?newquery%23newhash#hash");
> > +    is(url.href, "scheme:path/to/file?query#hash");
> 
> See other comment. It should not fail and the result should be
> ...?newquery#hash

Umm, I would not like to invent a new behavior here.
previous Firefox: "scheme:path/to/file?query#hash" (.search setter fails silently)
Chrome: "scheme:path/to/file?query#newhash#hash"
Edge: .search setter does not throw, but thereafter .href getter throws.
The spec, Safari, current Nightly: "scheme:path/to/file?query%23newhash#hash"

Comment 22

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102914/diff/3-4/

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

I employed Chrome's behavior because:
- It is simplest to implement.
- We have the same bug with Chrome anyway (bug 1326394).

Comment 24

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

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

https://reviewboard.mozilla.org/r/102914/#review106272

I guess this is OK for now. However, we will need to change this in the URL bar once we switch to a different parser.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cafc897f05e
Don't escape the query part of nsSimpleURI. r=valentin.gosu

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/7cafc897f05e

Comment 27

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 53.0a1 (2016-12-19) (64-bit) (Build ID: 20161219030207) by the following Comment 0's instruction!

This Bug is now Verified as Fixed on Latest Firefox Nightly 53.0a1 (2017-01-19) (64-bit)

Build ID: 20170119222621
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
OS: Linux 4.4.0-57-generic; elementary OS 0.4 (64 Bit)

Comment 28

[Alice0775 White]

[Tracking Requested - why for this release]: bug reproduce on 52.0b2


could you uplift this to beta52?

Comment 29

[Masatoshi Kimura [:emk]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

Approval Request Comment
[Feature/Bug causing the regression]: 1310483
[User impact if declined]: inconsistent URL bar behavior only in Firefox 52, the next ESR
[Is this code covered by automated tests?]: Yes. A test is added in the patch and an existing test covers.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change is minimal and it will only revert the old (Firefox 51 or earlier) behavior.
[String changes made/needed]: no string changes included.

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8824385 [details]
Bug 1327960 - Don't escape the query part of nsSimpleURI. .gosu

fix regression with uri escaping in beta52

Should be in 52.0b4.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b850284e545