1124600 - Percent-encoded encoded characters are unescaped in netmonitor

Status: NEW

(DevTools :: Netmonitor, defect, P3)

Description

[Elbart]

Attachment: percent 2015-01-21.png

Bug 1121826 reverted the changes of bug 473822 comment 14, and the bugs mentioned in the other comments are happening again, especially the URL-copying of bug 473822 comment 0.

In general, Firefox is handing percent encoded brackets differently, depending on where you look (made with Nightly 01-17):
http://i.imgur.com/ODh0xTK.png
A similar image made with Nighty 01-21 is attached.

Comparison with other browsers (made with Nightly 01-17):
http://i.imgur.com/kqDmRE4.png


Test-URLs:
http://example.org/?foo%5Ba%5D=bar&foo[b]=bar&foo%28c%29=bar&foo(d)=bar
http://example.org/?foo%5Ba%5D=bar
http://example.org/?foo[a]=bar

Comment 1

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

Interestingly, the dev-tools panel has the same issue as the url-bar. It unescapes URLs.
To illustrate this, open a page that contains console.log(window.location.href);
You will notice, that the page locations are correct, even though the devtools panel says otherwise.
- tested with nightly 38.0a1 (2015-01-22)

I think we need to fix both the devtools issue, and the URL-bar issue.

Comment 2

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

Hey Victor,
I notice there are several unescape(...) instructions in netmonitor-view.js
Can we get rid of them, or is there a reason they must stay?
IMO the network panel should display the exact URL we are loading.

Comment 3

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

I think we should also consider if percent decoding in the url-bar is actually necessary:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2329

I also did a try run to see if removing the decoding would break anything major:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a18b6568a66
Only the url-bar decoding tests it seems.

:dao, do you think we could get rid of this decoding? Other browsers don't do it, and at times it is very confusing (for example bug 956463 comment 0)

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Valentin Gosu [:valentin] from comment #3)
> Other browsers don't do it,

Really? At least Chrome decodes non-ascii characters on the Omnibar. Please do not uniformly stop decoding.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Valentin Gosu [:valentin] from comment #3)
> :dao, do you think we could get rid of this decoding?

No.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Valentin Gosu [:valentin] from comment #2)
> Hey Victor,
> I notice there are several unescape(...) instructions in netmonitor-view.js
> Can we get rid of them, or is there a reason they must stay?
> IMO the network panel should display the exact URL we are loading.

They're there because escaped strings are ugly and hard to read 99% of the time. Is there some exact specific use-case in which this is undesirable? If so, can we special-case it?

Comment 7

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

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> They're there because escaped strings are ugly and hard to read 99% of the
> time. Is there some exact specific use-case in which this is undesirable? If
> so, can we special-case it?

I think we can. I'll look to see if we already have code that does this, if not, I can write some.

Comment 8

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

Attachment: MozReview Request: Bug 1124600 - Percent-encoded characters are decoded in Firefox devtools r?vporof

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

Comment 9

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

(In reply to Valentin Gosu [:valentin] from comment #7)
> (In reply to Victor Porof [:vporof][:vp] from comment #6)
> > They're there because escaped strings are ugly and hard to read 99% of the
> > time. Is there some exact specific use-case in which this is undesirable? If
> > so, can we special-case it?
> 
> I think we can. I'll look to see if we already have code that does this, if
> not, I can write some.

I looked at the problem a bit more, and I don't think we can do this in a sane and consistent way.
Other devtools seem to get away with this, so I don't believe unescaping adds that much value - but it does get in the way in some corner cases.

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8725309 [details]
MozReview Request: Bug 1124600 - Percent-encoded characters are decoded in Firefox devtools r?vporof

https://reviewboard.mozilla.org/r/37417/#review34399

I don't think we should do this. Seems like a small enough problem with a fix that would not be worth it.

Comment 11

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

I don't personally see the advantage of keeping things as they are. In the case mentioned in comment 0 it is especially difficult to figure out the actual URL to which we send a request.
Other browsers avoid this problem by not unescaping at all.

Do you think we could change the UX to accommodate this use case? Such as show the real URL by default, and show the unescaped version on mouse-over? This would help both the "can't tell these URLs apart" and "this URL is hard to read" usecases.

Or, if we can't find an adequate solution for this problem, do you think maybe we could add a pref for this?

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Valentin Gosu [:valentin] from comment #11)
> 
> Do you think we could change the UX to accommodate this use case? Such as
> show the real URL by default, and show the unescaped version on mouse-over?
> This would help both the "can't tell these URLs apart" and "this URL is hard
> to read" usecases.

How about the other way? Show the escaped string by default then the real one on mouse over and other places where you'd click&select to copy.

Comment 13

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

I guess that's better that what we have at the moment. But I don't know the code well enough to make this happen.
Could you take the bug?

Comment 14

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

Victor, I'm assigning this to you at the moment, so we don't forget about it.
Feel free to pass it to someone else, or pass it back if you have a better suggestion for a solution. Thanks!

Comment 15

[Victor Porof [:vporof][:vp]]

I don't have time to work on this at the moment. I'll look around for a netmonitor maintainer.

Comment 16

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 17

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 20

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

I'm going to move this issue to Netmonitor.
Similar to bug 1026938 which concerns unescaping of URLs in the URL bar, this issue concerns the unescaping of URLs in netmonitor.
More specifically, it seems that the only way of ensuring that the correct unescaped path was sent to the server when loading http://example.org/?foo%5Ba%5D=bar is to look at the Raw request headers.

Comment 21

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

Clearing priority and severity so this gets reprioritized.

Comment 22

[Julian Descottes [:jdescottes]]

Valentin: Currently in the netmonitor, the URL in the requests list is rendered unescaped, but the preview in the "Headers" side panel is escaped. If we change this preview to be unescaped as well does it sound fine to you?

Comment 23

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

Yes, displaying the unescaped path in the Headers side panel would be best.

I also noticed that there's a mouseover Original & Decoded in the request list.