(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.

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.)

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) {}

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...

with this we'll unescape more lazily, mostly onLocationChange

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.

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.

(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".

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?

(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.

s/window.opener/content.opener/

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

(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.

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

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 "%".

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)

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.

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?

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 |"|.

updated to trunk

Comment on attachment 296424 [details] [diff] [review] patch v4 Will be interesting to see whether his has any perf impact.

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

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.

(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.

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)

Please file a new bug to avoid confusion.

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.