strange behavior when entering URL with escaped percent character %25

VERIFIED FIXED in Firefox 3 beta3

Status

()

defect
P3
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Gavin, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

Comment 2

12 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) {}
Flags: blocking-firefox3?
Assignee

Updated

12 years ago
Assignee: nobody → dao
Flags: blocking-firefox3? → blocking-firefox3+
Assignee

Comment 3

12 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #286264 - Flags: review?(gavin.sharp)
Assignee

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3 M10
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...
Assignee

Comment 5

12 years ago
Posted patch patch v2 (obsolete) — Splinter Review
with this we'll unescape more lazily, mostly onLocationChange
Attachment #286264 - Attachment is obsolete: true
Attachment #287168 - Flags: review?(gavin.sharp)
Attachment #286264 - Flags: review?(gavin.sharp)
Assignee

Comment 6

12 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.
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.
Attachment #287168 - Flags: review?(gavin.sharp) → review-
Assignee

Comment 8

12 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #287168 - Attachment is obsolete: true
Attachment #287898 - Flags: review?(gavin.sharp)
Assignee

Comment 9

12 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".
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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?
Attachment #287898 - Flags: review?(gavin.sharp) → review-
Assignee

Comment 11

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

Comment 12

12 years ago
Posted patch patch v4 (obsolete) — Splinter Review
s/window.opener/content.opener/
Attachment #287898 - Attachment is obsolete: true
Attachment #288061 - Flags: review?(gavin.sharp)
Assignee

Updated

12 years ago
Blocks: 388372
Assignee

Updated

12 years ago
Blocks: 394667
Priority: -- → P3
Whiteboard: [has patch][need review gavin]
Duplicate of this bug: 405038
Summary: strange behavior when entering URL with escaped "%" character → strange behavior when entering URL with escaped percent character %25
Assignee

Updated

12 years ago
Blocks: 408891

Comment 14

12 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
Assignee

Comment 15

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

Comment 16

12 years ago
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)
Assignee

Comment 19

12 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.
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?
Assignee

Comment 21

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

Updated

12 years ago
Duplicate of this bug: 410429

Updated

12 years ago
Blocks: 410726
No longer blocks: 379053
Assignee

Comment 23

12 years ago
Posted patch patch v4Splinter Review
updated to trunk
Attachment #288061 - Attachment is obsolete: true
Attachment #296424 - Flags: review?(gavin.sharp)
Attachment #288061 - Flags: review?(gavin.sharp)
Comment on attachment 296424 [details] [diff] [review]
patch v4

Will be interesting to see whether his has any perf impact.
Attachment #296424 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [has patch][need review gavin] → [has patch]
Assignee

Updated

12 years ago
Keywords: regression
Assignee

Updated

12 years ago
Blocks: 366797
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch]
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.
Assignee

Comment 27

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

Updated

12 years ago
Blocks: 387723
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 29

12 years ago
Please file a new bug to avoid confusion.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Assignee

Updated

12 years ago
Depends on: 412458
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.
Status: RESOLVED → VERIFIED
Assignee

Updated

11 years ago
No longer blocks: 425480
Assignee

Updated

11 years ago
Blocks: 425480
You need to log in before you can comment on or make changes to this bug.