407974 - should url bar autocomplete results decode UTF-8 encoded urls?

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Tomer Cohen :tomer]

Attachment: Screenshot of he.wikipedia.org as suggestion

I've just found that while the new address bar can show %-encoded URLs in the address bar, the suggestions panel right after it show them much like the address bar in Firefox 2.

Steps to reproduce:
a. enter any non-latin1 wikipedia site (he.wikipedia.org and ar.wikipedia.org are good examples. 
b. watch the address bar become readable for the whole address (the translation of  Main_Page (en) as the landing page name).
c. try to access that address again; you'll see the address encrypted in the suggestion panel.

Comment 1

[(not reading, please use [e-mail] instead)]

morphing bug slightly.

Tomer points out in the original bug report that Firefox 2 had the same issue.

Additionally, setting browser.urlbar.richResults to false, that this isn't a new problem with rich results.

Note that when doing autocomplete, if you were to match against the encoded url, you'd currently have to type the encoded text (%D7%A2...) to match the url, and that typing the decoded hebrew (עמוד_ראשי) would not work.

We have some additional bugs about encoding / decoding and escaping / unescaping with places and with the url bar.

Comment 2

[(not reading, please use [e-mail] instead)]

> We have some additional bugs about encoding / decoding and escaping /
> unescaping with places and with the url bar.

see bug #389465 – Places should decode URLs
see bug #391691 – escaped and unescaped versions of URI are both in history

Comment 3

[Ed Lee :Mardak]

Attachment: v1

This fixes things up in the back end so it'll also fix bug 413784, bug 391175, bug 415187, bug 389465.

This patch is applied to the current trunk with no dependencies.

Comment 4

[Ed Lee :Mardak]

Attachment: v1 w/ Erwan's unit test

Added Erwan's testcase from bug 413784.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 300796 [details] [diff] [review]
v1 w/ Erwan's unit test

so, r=me for the code change, looks ok. however, i'm super wary of regressing awesomeness this late in the cycle, so can you please confirm:

1. doesn't regress existing tests

2. doesn't regress perf - i'd like to know exactly how much this'll cost.

ensuring solvency on both issues will increase potential driver acceptance.

Comment 6

[Ed Lee :Mardak]

(In reply to comment #5)
> 1. doesn't regress existing tests
Unit tests for places pass.

> 2. doesn't regress perf - i'd like to know exactly how much this'll cost.
I've got a tryserver build building.

Comment 7

[Ed Lee :Mardak]

(In reply to comment #5)
> 2. doesn't regress perf - i'd like to know exactly how much this'll cost.
I'm printing out runtime numbers for FullHistorySearch with the default prefs (25max, 100chunk, 100timeout) on a debug build:

Typing out "mozilla" one letter at a time: (amount of time in FullHistorySearch)
Without patch: 4ms, 6ms, 6ms, 8ms, 6ms, 8ms, 8ms
With patch: 4ms, 7ms, 7ms, 7ms, 9ms, 7ms, 7ms

Adding on ".org" to "bugzilla.mozilla" one character at a time:
Without patch: 9ms, 9ms, 10ms, 9ms
With patch: 10ms, 10ms, 9ms, 10ms

Remember these are debug builds, so it's going to be significantly slower than a release build. But they're pretty small already.. so not much to worry about!

Comment 8

[Ed Lee :Mardak]

Comment on attachment 300796 [details] [diff] [review]
v1 w/ Erwan's unit test

Target Firefox3 beta3, blocking-firefox3+, passes unit tests, add unit test, no/minimal performance impact

Huge gain in functionality for international languages especially asian ones like Chinese and Japanese, etc. English users even benefit because now you can both see and search for "@", ":", and other escaped symbols.

Comment 9

[Ed Lee :Mardak]

Attachment: screenshot of trunk and v1

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 300796 [details] [diff] [review]
v1 w/ Erwan's unit test

a=beltzner

Comment 11

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.37; previous revision: 1.36
done

Comment 12

[Brett Wilson]

This patch isn't quite right, but will work in most cases. Not all sites use UTF-8 for the encoding, as assumed by this patch.

I think there is some code (maybe for the status bar) that will do some guessing. A better solution would be to store the encoding in the DB (the URL object knows it in many cases) so you know how to unescape it.

Comment 13

[Brett Wilson]

Also, what about IDN? That might be a different bug, but it should also work.

Comment 14

[Ed Lee :Mardak]

If so, we'll need to change how we store the URIs to begin with because right now nsNavHistory.cpp calls BindStatementURI which gets the UTF8 spec from the URI.

Comment 15

[Ed Lee :Mardak]

Actually, is that a problem at all? If the URI is not using UTF-8 encoding, the URI object knows how to give the correct UTF-8 encoding. We store the UTF-8 encoding in the database independent of the original encoding. (Both encodings should be valid.) What this patch does is decoding the value from the database.

Comment 16

[Brett Wilson]

See the origin charset:
  http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIURI.idl#223
I think but am not sure, if the origin charset is non UTF-8 for the URL when it was added to history, this will break.

I think to fix this would require storing URLs differently, which sucks and may not be practical.

Comment 17

[Simon Bünzli]

Could this patch have caused bug 415397?

Comment 18

[mmortal03]

Was this bug supposed to refer to text strings that were manually entered into the address bar?  I ask because currently, if I paste the following URL string into the address bar, http://forums.mozillazine.org/viewtopic.php%3fp%3d3279302#3279302 , the page cannot be found, because Firefox isn't decoding the Javascript encoded URL.

It would be very helpful if the address bar translated the address typed into it at the point of executing the Go button behavior, in this case, to the following: http://forums.mozillazine.org/viewtopic.php?p=3279302#3279302

I run into a site daily that doesn't translate their Javascript referral string URL that is in the form of the first example to that of the second example.  That has me manually copying the URL part of the referral string and pasting it into the following website to decode it, and then pasting the decoded URL back into the address bar: http://meyerweb.com/eric/tools/dencoder/

Comment 19

[Ed Lee :Mardak]

If you're typing in or copy/pasting stuff to enter in the location bar, then it's not related to this bug. The first url you provided is a valid url that requests a page viewtopic.php%3fp%3d3279302, but you probably wanted it to request the page viewtopic.php with query parameter p=....