:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Description • 17 years ago

(Feel free to resummarize, I'm not clear on exactly what the problem is.)

When I enter http://www.google.ca/search?q=%E9 into the URL bar, I'm redirected to a Google search page that has "é" in the search field (so "é" is what's submitted to the server). When I enter http://www.google.ca/search?q=%25E9 I get the same result, and the URL bar changes to display "http://www.google.ca/search?q=%E9". I would expect the second URL to result in a Google search page with "%E9" in the search box.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 1 • 17 years ago

I discovered this problem because LXR has a bug that requires you to double-escape quotes (you need to replace "%22" in the resulting URL with "%2522"), and the new URL bar doesn't let me do this ("%2522" ends up being treated the same as "%22", which fails).

(This hack is also harder to do now that the location bar displays unescaped strings, since the %22 becomes a ", but that doesn't really have anything to do with this bug.)

	Dão Gottwald [:dao] Assignee
	Comment 2 • 17 years ago

The problem is that if you enter "...q=%25E9" the location bar will decode it before it gets submitted. I'm not yet sure if there's only a problem with "%" -- if that's the case, I think this would be my fix:

 try {
   // try to decode as UTF-8
-  value = decodeURI(value);
+  value = decodeURI(value).replace(/%/g, "%25");
 } catch(e) {}

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 4 • 17 years ago

I'm a bit concerned that the unescaping we're doing to display in the URL bar is affecting the URL we're submitting. Perhaps we could keep the "value" separate from the "pretty value" that's displayed in the URL bar? I suppose that might introduce different problems...

	Dão Gottwald [:dao] Assignee
	Comment 5 • 17 years ago

with this we'll unescape more lazily, mostly onLocationChange

	Dão Gottwald [:dao] Assignee
	Comment 6 • 17 years ago

Due to some refactoring, patch v2 also fixes some minor bugs like inconsistent pageproxystates for new windows / blank tabs or losing the typed value after customizing the toolbar.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 7 • 17 years ago

Comment on attachment 287168 [details] [diff] [review]
patch v2

>Index: base/content/browser.js

>+function handleURLBarRevert() {

>-    if (url != "about:blank" || content.opener) {
>-      gURLBar.value = url;
>+    URLBarSetURI();
>+    if (gURLBar.value || content.opener)
>       gURLBar.select();

This breaks the fix for bug 370555, since URLBarSetURI() unconditionally replaces "about:blank" with "".

>   // Update the urlbar
>-  var url = getWebNavigation().currentURI.spec;
>   if (gURLBar) {
>-    gURLBar.value = url == "about:blank" ? "" : url;
>-    SetPageProxyState("valid");
>+    URLBarSetURI();

URLBarSetURI() calls SetPageProxyState("invalid") in the about:blank case, while this code calls SetPageProxyState("valid"); uncondtionally. I'm not sure this is actually a problem, but maybe you've already considered this.

	Dão Gottwald [:dao] Assignee
	Comment 9 • 17 years ago

(In reply to comment #7)
> URLBarSetURI() calls SetPageProxyState("invalid") in the about:blank case,
> while this code calls SetPageProxyState("valid"); uncondtionally. I'm not sure
> this is actually a problem, but maybe you've already considered this.

It fixes this inconsistent case:

- open a tab (pageproxystate is "valid")
- type (pageproxystate is "invalid")
- hit ESC (pageproxystate is "invalid")

With the fix, the state will always be "invalid".

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 10 • 17 years ago

Comment on attachment 287898 [details] [diff] [review]
patch v3

>Index: browser/base/content/browser.js

>+function URLBarSetURI(aURI) {

>+    if (spec == "about:blank") {
>+      // Replace "about:blank" with an empty string
>+      // only if there's no opener (bug 370555).
>+      if (!window.opener)
>+        spec = "";

window.opener isn't right either, it will be a ChromeWindow for windows opened using Ctrl+N, for example. You want content.opener like the original code, right?

>-          if (location && gURIFixup) {
>-            try {
>-              location = gURIFixup.createExposableURI(aLocationURI).spec;
>-            } catch (ex) {}

This code used to set the value of the location bar to "" if aLocationURI (and thus |location|) is null (e.g. when called from updateCurrentBrowser() in tabbrowser.xml). The new code in URLBarSetURI falls back to |getWebNavigation().currentURI;|. That's not much of a problem in itself, because currentURI is what updateCurrentBrowser() in tabbrowser.xml passes in anyways, but the previous code (and the code previous to it, see bug 169826) seemed to assume that currentURI can be null. It appears to be possible for nsDocShell::GetCurrentURI to return a null URI, but given the other existing consumers in browser.js that assume it's always non-null, it's probably safe to assume that that doesn't happen in practice, I guess. So all that to say I think this change is probably OK :)

>-          // Setting the urlBar value in some cases causes userTypedValue to
>-          // become set because of oninput, so reset it to its old value.
>-          browser.userTypedValue = userTypedValue;

I guess the idea is that this was bogus and can just be removed?

	Dão Gottwald [:dao] Assignee
	Comment 11 • 17 years ago

(In reply to comment #10)
> window.opener isn't right either, it will be a ChromeWindow for windows opened
> using Ctrl+N, for example. You want content.opener like the original code,
> right?

Err, yes. Stupid mistake.

> >-          // Setting the urlBar value in some cases causes userTypedValue to
> >-          // become set because of oninput, so reset it to its old value.
> >-          browser.userTypedValue = userTypedValue;
> 
> I guess the idea is that this was bogus and can just be removed?

Yeah, I don't see the point of that. I think the input event isn't even dispatched if you change the value property.

	Dão Gottwald [:dao] Assignee
	Comment 12 • 17 years ago

s/window.opener/content.opener/

	Yuri Khan
	Comment 14 • 17 years ago

This bug breaks the quick searches functionality, including the mozcharset=xxx workaround for the bug 258223. I traced the problem to this line near the end of function canonizeUrl, file browser.js:

gURLBar.value = getShortcutOrURI(url, aPostDataRef);

The function getShortcutOrURI returns a search URL correctly encoded using the charset specified in the mozcharset parameter (in my case, utf-8); then the assignment to gURLBar.value causes it to be decoded, then re-encoded using the local charset, which, in my case, happens to be windows-1251, leading to a failed search.

I work around it by replacing with

gURLBar.value = getShortcutOrURI(url, aPostDataRef).replace(/%/g, "%25");

while waiting for the fix.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

	Dão Gottwald [:dao] Assignee
	Comment 15 • 17 years ago

(In reply to comment #14)
> This bug breaks the quick searches functionality

Nope, that's bug 387723.

> I work around it by replacing with
> 
> gURLBar.value = getShortcutOrURI(url, aPostDataRef).replace(/%/g, "%25");

That's only a side effect.

	Dão Gottwald [:dao] Assignee
	Comment 16 • 17 years ago

Gavin, is there anything left in the patch that makes you feel uncomfortable?

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 17 • 17 years ago

I wanted to look over the refactoring again, but I did that on the previous patch so that's probably not necessary. The only issue I have with it are that I still don't understand why we need to special-case "%".

	Tuukka Tolvanen (sp3000)
	Comment 18 • 17 years ago

because what is being done is translating to an alternate equivalent encoded form, not decoding to literal form. say, in a backslashy regexpoid encoding world where a and \a both mean literal a, and \\ means literal \, this is the equivalent of aa\aa\\aa -> aaaa\\aa (encoded -> encoded), not aa\aa\\aa -> aaaa\aa (encoded -> literal). When encoded \x means literal x for every ., it rather does look like decoding to literals expect the escaper being special-cased.

(I so didn't read the whole patch but I'll assume that's about what's happening)

	Dão Gottwald [:dao] Assignee
	Comment 19 • 17 years ago

Yeah. Except that contrary to \, % doesn't escape literals but introduces encoding sequences. So when we change %25E9 to %E9, "E9" becomes such an encoding sequence.

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 20 • 17 years ago

Just to be sure I understand: when I enter "%2522" in the location bar, it's currently unescaped twice, right? Once by the location bar when I enter it, to become |%22|, which is then submitted to the URI loading code which unescapes it again to become |"|, which is what gets submitted. Then the URI loading code returns either the escaped string |%22|, or the literal string |"| (which is it?) and is displayed in the URL bar as |"|.

The proposed solution avoids unescaping "%25" because it would unescape to "%" which would then potentially be modified by the second unescaping in the URI loading code.

Is what I've said above correct?

	Dão Gottwald [:dao] Assignee
	Comment 21 • 17 years ago

Yes, we unescape multiple times ... this wasn't a problem for %E9, which isn't a valid unicode sequence. But %22 reveals this. However, even if we unescape only once, changing %2522 to %22 is a problem because the server will still read this as |"|.

	Dão Gottwald [:dao] Assignee
	Comment 23 • 17 years ago

updated to trunk

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 24 • 17 years ago

Comment on attachment 296424 [details] [diff] [review]
patch v4

Will be interesting to see whether his has any perf impact.

	Reed Loden [:reed]
	Comment 25 • 17 years ago

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.925; previous revision: 1.924
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.52; previous revision: 1.51
done

	Reed Loden [:reed]
	Comment 26 • 17 years ago

I don't see any bad perf impact, but this did cause the test for bug 410900 to fail for "App handler list populated". I downloaded an hourly to test this for myself, and it worked fine, so I'm guessing it's just a bug in the test, so I disabled it for now until Ryan can look at it.

	Dão Gottwald [:dao] Assignee
	Comment 27 • 17 years ago

(In reply to comment #26)
> I don't see any bad perf impact

Gavin meant a positive impact on pageload, but the difference is probably too small.

	Mycroft Project (CC)
	Comment 28 • 17 years ago

Apologies if this is actually a different bug...

Visit google.co.uk (etc)
Paste http://www2 and search.
Location bar reads:
http://www.google.co.uk/search?hl=en&q=http%253A%252F%252Fwww2&btnG=Google+Search&meta=

%253A should surely be %3A?
%252F should surely be %2F?

Place cursor in URL bar and hit enter.
Now searching for http%3A%2F%2Fwww2

(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011504 Minefield/3.0b3pre)

	Dão Gottwald [:dao] Assignee
	Comment 29 • 17 years ago

Please file a new bug to avoid confusion.

	Tony Chung [:tchung]
	Comment 30 • 17 years ago

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020418 Firefox/3.0b3.   i checked that http://www.google.ca/search?q=%2522 displays %22 in the search textbox, and http://www.google.ca/search?q=%22 returns ".

Please let me know otherwise if there was another way to verify this fix, and list the testcase here.