Last Comment Bug 461304 - Loading URL by pressing ENTER on already present URL in location bar doesn't maintain URL encoding
: Loading URL by pressing ENTER on already present URL in location bar doesn't ...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: Firefox 10
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
http://www.stockholm.se/-/Nyheter/?q=...
: 484244 532872 551955 (view as bug list)
Depends on: 393246
Blocks: 472669 673500 801287
  Show dependency treegraph
 
Reported: 2008-10-23 00:26 PDT by Kai de Leeuw
Modified: 2012-10-13 14:31 PDT (History)
18 users (show)
mbeltzner: blocking‑firefox3.5-
dietrich: wanted‑firefox3.6+
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add UTF-8 flag to nsDocShell::LoadURI (2.16 KB, patch)
2011-10-27 08:17 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Use UTF-8 flag in the browser (8.54 KB, patch)
2011-10-27 08:20 PDT, Masatoshi Kimura [:emk]
gavin.sharp: review-
Details | Diff | Splinter Review
Part 1: Add UTF-8 flag to nsDocShell::LoadURI, v2 (2.19 KB, patch)
2011-10-28 21:38 PDT, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Use UTF-8 flag in the browser, v2 (8.48 KB, patch)
2011-11-02 08:31 PDT, Masatoshi Kimura [:emk]
gavin.sharp: review+
Details | Diff | Splinter Review
An automated test (3.44 KB, patch)
2011-11-02 21:48 PDT, Masatoshi Kimura [:emk]
gavin.sharp: review+
Details | Diff | Splinter Review
Test, for checkin (3.11 KB, patch)
2011-11-03 08:55 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 1: Add UTF-8 flag to nsDocShell::LoadURI, for checkin (2.25 KB, patch)
2011-11-03 08:56 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 2: Use UTF-8 flag in the browser, for checkin (8.54 KB, patch)
2011-11-03 08:57 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Kai de Leeuw 2008-10-23 00:26:35 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081022 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081022 Minefield/3.1b2pre

When you load a page in Firefox the location bar makes some magic and "decodes" the URL, making %c3%b6 for instance appearing as "ö". All this is fine, but when I just go to the location bar and press ENTER the encoding gets screwed up.

What I think Firefox does wrong: 
It doesn't remember what encoding the page loaded had (UTF-8) and therefore just encodes it as ISO-8859-1 or something like this. It should "remember" the encoding.

Reproducible: Always

Steps to Reproduce:
1.Load the URL in the URL field, see that the textbox to the right contains "höjning"
2.Go to the location bar and press ENTER
3.See how the text box changes to h�jning
Actual Results:  
The URL encoding changed

Expected Results:  
The URL encoding stays the same
Comment 1 Kai de Leeuw 2008-10-23 12:36:19 PDT
According to my colleague who has a Mac this is a Windows only error.
Comment 2 José Jeria 2008-10-25 02:54:14 PDT
I see this too, steps to reproduce
1. load http://www.stockholm.se/-/Nyheter/?q=h%C3%B6jning&inkluderalokala=False
2. Now focus the url field and press ENTER

ACTUAL RESULTS
"höjning" in url changed to "h%F6jning" which led to no search results on the page

EXPECTED RESULTS
"höjning" should not change once you press enter in the url field
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-11 22:40:41 PST
Gavin, Mardak: any ideas?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-11 23:00:15 PST
Presumably this happens because network.standard-url.encode-query-utf8 is false, which seems to be bug 333859. I guess this is a duplicate of that bug. Some context is in bug 261929.

IE 7 has the same bug, but doesn't automatically decode visited URIs, so it's more difficult to hit (you need to paste in the UTF-8 string). I wonder whether we should avoid decoding the query part...
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-11 23:19:49 PST

*** This bug has been marked as a duplicate of bug 333859 ***
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-11 23:39:54 PST
Oops, misunderstood Gavin, who's proposing that we mitigate in Firefox.

The whole point of encoding the query strings is so that when someone types something like "höjning" into their location bar, it matches what they expect. If I'm understanding this right, though, when they then find the thing that looks as expected, hitting ENTER won't have predictable results.

To me that implies that not decoding the query string part will just mean that we won't get the right sort of smart location bar matches, which is a different problem.

I think the best solution is to have this fixed by bug 333859 ...
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-11 23:48:38 PST
(In reply to comment #6)
> The whole point of encoding the query strings is so that when someone types
> something like "höjning" into their location bar, it matches what they expect.
> If I'm understanding this right, though, when they then find the thing that
> looks as expected, hitting ENTER won't have predictable results.

If the site expects it's parameters to be UTF-8 encoded, and the character is in the query string, yeah.

> To me that implies that not decoding the query string part will just mean that
> we won't get the right sort of smart location bar matches, which is a different
> problem.

I don't think the awesomebar is affected by this either way, since we explicitly decode URIs there.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-12 14:51:03 PST
(In reply to comment #7)
> I don't think the awesomebar is affected by this either way, since we
> explicitly decode URIs there.

I was wrong here... we store the correctly encoded URI, assuming you reached the site by actually submitting the search or clicking a link rather than just entering the URL in the URL bar, but when you recall that entry in the awesomebar we decode it again, triggering this bug.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-13 16:56:19 PST
This isn't a regression, so as much as it pains me, it doesn't block. Mardak, can you think of anything that could be causing this on w32-only?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-13 18:10:42 PST
Looks like it's windows-only because LoadURI() takes a wstring, and somehow before that gets passed to NS_NewURI that is assumed to be in the platform native character set (which is UTF-8 on Linux and Mac)?
Comment 11 Masatoshi Kimura [:emk] 2009-01-12 15:04:01 PST
Bug 393246 will fix this. It will have fewer side-effects than bug 333859.
Bug 393246 was backouted because of bug 432836. But I don't think bug 432836 is a valid claim because even Firefox 2 does not "work" if the system codepage is not Windows-1252.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-20 15:52:33 PDT
*** Bug 484244 has been marked as a duplicate of this bug. ***
Comment 13 (mostly gone) XtC4UaLL [:xtc4uall] 2010-03-12 14:30:38 PST
*** Bug 551955 has been marked as a duplicate of this bug. ***
Comment 14 Jonathan Haas 2011-07-28 00:40:25 PDT
Instead of waiting for a fix for bug 393246 or bug 333859, I see the simple solution to just only decode URLs when encoding them again would lead to the same URL.

For example when we encode using the system charset, only decode URLs that are in the system charset, too. If we encode only in UTF-8, only decode when the source URL is in UTF-8, too.
Comment 15 Masatoshi Kimura [:emk] 2011-10-27 08:17:44 PDT
Created attachment 569979 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI
Comment 16 Masatoshi Kimura [:emk] 2011-10-27 08:20:01 PDT
Created attachment 569980 [details] [diff] [review]
Part 2: Use UTF-8 flag in the browser

We can be sure that the URI is encoded in UTF-8 if the value of location bar is not typed.
Comment 17 Olli Pettay [:smaug] (TPAC) 2011-10-28 15:54:00 PDT
Comment on attachment 569979 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI

I'd prefer if Boris could review this since he reviewed Bug 691690.
Comment 18 Boris Zbarsky [:bz] (TPAC) 2011-10-28 20:53:16 PDT
Comment on attachment 569979 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI

Should that be an else if, or just an if?
Comment 19 Masatoshi Kimura [:emk] 2011-10-28 21:38:04 PDT
Created attachment 570453 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI, v2

Oh, I don't recall why I wrote "else if" here.
Comment 20 Boris Zbarsky [:bz] (TPAC) 2011-10-28 21:56:37 PDT
Comment on attachment 570453 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI, v2

r=me
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-01 14:06:32 PDT
Comment on attachment 569980 [details] [diff] [review]
Part 2: Use UTF-8 flag in the browser

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="loadOneTab">
>+        <parameter name="aIsUTF8"/>

>       <method name="addTab">
>+        <parameter name="aIsUTF8"/>

Please don't add to the actual parameter lists - just make any consumers that need to pass this parameter use the argument-object form (looks like they already all are).

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>           function loadCurrent() {

>+            if (!this.valueIsTyped)
>+              flags |= Ci.nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;

Can you add a comment explaining the reasoning here? "If the value wasn't typed, we know that we decoded the value as UTF-8 (see losslessDecodeURI)" or somesuch.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>@@ -194,16 +194,17 @@ function openLinkIn(url, where, params) 

>+  var aIsUTF8               = params.isUTF8;

Looks like you only implemented support for this for where=="tab" or "current" - this won't work if where=="window" (and probably "save"). That's probably not a big deal but it's worth a comment at least.

Looks good otherwise, will r+ with those addressed.
Comment 22 Masatoshi Kimura [:emk] 2011-11-02 08:31:39 PDT
Created attachment 571337 [details] [diff] [review]
Part 2: Use UTF-8 flag in the browser, v2

Resolved review comments.
> Looks like you only implemented support for this for where=="tab" or "current" - this won't work if where=="window" (and probably "save"). That's probably not a big deal but it's worth a comment at least.
Yeah, I couldn't find a way to open the non-typed url to a new window or save it from the location bar (not from the dropdown which works already). Added a comment.
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-02 10:06:03 PDT
Comment on attachment 571337 [details] [diff] [review]
Part 2: Use UTF-8 flag in the browser, v2

Looks good. Can you write a test for this?
Comment 24 Masatoshi Kimura [:emk] 2011-11-02 21:48:43 PDT
Created attachment 571557 [details] [diff] [review]
An automated test
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-03 07:52:40 PDT
Comment on attachment 571557 [details] [diff] [review]
An automated test

>diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in

>+                 browser_bug461304.js \

Please name the test "browser_urlbarEnter.js" instead (we'd like to discourage naming test files browser_bugXXXXXX).

>diff --git a/browser/base/content/test/browser_bug461304.js b/browser/base/content/test/browser_bug461304.js

>+let gFocusManager = Cc["@mozilla.org/focus-manager;1"].
>+                    getService(Ci.nsIFocusManager);

I think you could remove this from this test (and the associated checks), since that's already covered in other tests.

>+function locationBarEnter(aEvent, aClosure) {
>+  setTimeout(function() {

You can use executeSoon for this instead of setTimeout. But I wonder whether you need to do this here - can you do it only for the cleanup and runNextTest call that is in runNextTest? E.g.:

function runNextTest() {
...
  addPageShowListener(function() {
    locationBarEnter(test.event, function() {
      test.check(tab);

      executeSoon(function () {
        // Clean up
        while (gBrowser.tabs.length > 1)
        gBrowser.removeTab(gBrowser.selectedTab)

        runNextTest();
      });

r=me with those addressed, thanks for the test!
Comment 26 Masatoshi Kimura [:emk] 2011-11-03 08:55:43 PDT
Created attachment 571660 [details] [diff] [review]
Test, for checkin

> But I wonder whether you need to do this here - can you do it only for the
> cleanup and runNextTest call that is in runNextTest? E.g.:
Without using executeSoon here, "current tab" case unexpectedly pass the test. I don't know the reason why.
Other points are addressed.
Comment 27 Masatoshi Kimura [:emk] 2011-11-03 08:56:53 PDT
Created attachment 571661 [details] [diff] [review]
Part 1: Add UTF-8 flag to nsDocShell::LoadURI, for checkin
Comment 28 Masatoshi Kimura [:emk] 2011-11-03 08:57:45 PDT
Created attachment 571662 [details] [diff] [review]
Part 2: Use UTF-8 flag in the browser, for checkin
Comment 29 Masatoshi Kimura [:emk] 2011-11-03 08:59:02 PDT
(In reply to Masatoshi Kimura [:emk] from comment #26)
> > But I wonder whether you need to do this here - can you do it only for the
> > cleanup and runNextTest call that is in runNextTest? E.g.:
> Without using executeSoon here, "current tab" case unexpectedly pass the
> test. I don't know the reason why.
"current tab" case unexpectedly pass the test even if unpatched.
Comment 30 Ed Morley [:emorley] 2011-11-05 03:59:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=41d5c15a4430
Comment 33 Masatoshi Kimura [:emk] 2011-11-08 21:31:45 PST
*** Bug 532872 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.