New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 789163 link

Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Omnibox does not unescape URL fragment

Reported by factormy...@gmail.com, Nov 28 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.29 Safari/537.36

Steps to reproduce the problem:
1. open example webpage
2. observe the browser url and the devtools console
3. the end of the url should look like: ∗#∗ but instead comes out as: ∗#%E2%88%97

Example:
(also live at https://factormystic.net/crbug/bad-hash-escaping.html)

```
<!DOCTYPE html>
<html>
<head>
<script type="text/javascript">
window.history.replaceState(null, null, "\u2217");
window.history.replaceState(null, null, "#\u2217");
if (document.location.hash != "#\u2217") {
    console.error("we have a problem");
}
</script>
</head>
</html>

```

BTW, I don't think this is specifically about window.history.replaceState, as document.location.hash = "#\u2217" appears to be equivalently regressed.

What is the expected behavior?
I do not expect that setting the document location hash will be escaped. "\u2217" was not escaped in prior versions of chrome, or in other browsers, or other portions of the url.

What went wrong?
"\u2217" (as a demo) is escaped in the hash portion of the url. This is behavior that has recently changed, and I do not expect or want it to be escaped in the hash or anywhere.

Did this work before? Yes it works correctly in 62.0.3202.29 (beta) for sure, not sure about between that and 64.0.3269.3 (dev)

Chrome version: 64.0.3269.3  Channel: dev
OS Version: 10.0
Flash Version:
 
crbug-hash-broken.png
159 KB View Download
crbug-hash-ok.png
53.0 KB View Download
Labels: Needs-Triage-M64
Cc: ranjitkan@chromium.org divya.pa...@techmahindra.com
Labels: -Pri-2 hasbisect-per-revision ReleaseBlock-Stable Triaged-ET M-63 OS-Linux OS-Mac Pri-1
Owner: mkwst@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on reported version 64.0.3269.3, beta 63.0.3239.59 and Dev 64.0.3280.0 using Windows 10, Ubuntu 14.04, Win10 hence providing bisect info

Bisect Info:
================
Good build: 63.0.3236.0
Bad build: 63.0.3237.7

You are probably looking for a change made after 507480 (known good), but no later than 507481 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/0a918e900a759b04f177cc5b8bca668140751136..f8f6ed59949be4451ee2f5443d8a313f102fde60

Reviewed-on: https://chromium-review.googlesource.com/668363

@Mike West: Please confirm the issue and help in re-assigning if it is not related to your change.

Note: Feel free to adjust the blocker and milestone if this should not be blocking for M-63

Thanks!
Cc: pbomm...@chromium.org gov...@chromium.org

Comment 4 by gov...@chromium.org, Nov 29 2017

M63 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge  into the release branch ASAP. Thank you.

Comment 5 by gov...@chromium.org, Nov 29 2017

Plan is to cut M63 Stable RC on this Friday (12/01) @ 5:00 PM PT. Please plan to have fix landed/merged to M63 before then. Thank you.

Comment 6 by mkwst@chromium.org, Nov 30 2017

Cc: elawrence@chromium.org est...@chromium.org
We intentionally changed our encoding of fragment identifiers to align with the spec and other vendors (and will be encoding more characters as discussed in https://github.com/whatwg/url/issues/344 and https://github.com/whatwg/url/pull/347).

* Chrome 63+, Firefox, and Safari all echo "we have a problem", as `window.location.hash` is populated with the percent-encoded values rather than the non-encoded values. I don't have Edge sitting in front of me to test, but I can check it when I get home.

* You're correct, though, that we're showing percent-encoded values in the fragment, but not the path. That's strange.

I don't feel like the change in the way that fragment identifiers are displayed is a stable blocker, and would prefer that we keep the web-facing change to align our behavior with other vendors. That said, it's not pretty, and it would be nice to fix it. CCing some folks who know more about URL display in the omnibox than I do, and who might have opinions.

If this is deemed a blocker, then reverting https://chromium-review.googlesource.com/c/668363/ would fix the display at the expense of regressing the percent-escaping.
> Chrome 63+, Firefox, and Safari all echo "we have a problem", as `window.location.hash` is populated with the percent-encoded values rather than the non-encoded values. I don't have Edge sitting in front of me to test, but I can check it when I get home.

I see. However there is still a variation. The url bar ui in other browsers does NOT display the percent encoded hash. Only Chrome 63 does. This still seems like a bug.
crbug-ui-edge.png
108 KB View Download
crbug-ui-firefox.png
86.6 KB View Download

Comment 8 by gov...@chromium.org, Nov 30 2017

Cc: candr...@chromium.org
+candrada@, could you pls check whether this repros on Android or not?
Re #7: 

> "I see. However there is still a variation"

Yes, this is as Mike notes when he says " You're correct, though, that we're showing percent-encoded values in the fragment, but not the path. That's strange. "

The fix for that in Chrome is small in size, but carries unknown risk at this late date.

--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -586,11 +586,10 @@ base::string16 FormatUrlWithAdjustments(
                              NonHostComponentTransform(unescape_rules),
                              &url_string, &new_parsed->query, adjustments);

-    // Ref.  This is valid, unescaped UTF-8, so we can just convert.
     if (parsed.ref.is_valid())
       url_string.push_back('#');
     AppendFormattedComponent(spec, parsed.ref,
-                             NonHostComponentTransform(net::UnescapeRule::NONE),
+                             NonHostComponentTransform(unescape_rules),
                              &url_string, &new_parsed->ref, adjustments);
   }

Labels: OS-Android
Re #8, yes, this reproduces on Android Chrome 64 (and does not repro on Android Chrome 62).
Cc: cma...@chromium.org gkihumba@chromium.org
Labels: OS-Chrome
Thank you  elawrence@.

Adding Chrome OS as well assuming this repros there as well.
Cc: mkwst@chromium.org pkasting@chromium.org
 Issue 790688  has been merged into this issue.
Labels: -ReleaseBlock-Stable
Status: Untriaged (was: Assigned)
I don't think this should be considered a Release blocker, but assigning to the proper node for triage.
Summary: Omnibox does not unescape URL fragment (was: document location hash incorrectly escaped)
Components: -Blink Blink>Network
Components: -Blink>Network UI>Browser>Omnibox
mkwst@ and elawrence@ - are you planning to make this fix?  I know you decided not to merge a fix, but I got the impression that you would fix it.
RE #17: I think we /should/ take a fix here, but I'd like to hear from Omnibox owners who are experts in this space before we change the behavior. I suppose we could just try the CL in #9 and wait for LGTMs?
As an omnibox owner, fixing this sgtm.

Looking to see how many tests I break. :) 
https://chromium-review.googlesource.com/c/chromium/src/+/822819
Hello! I'm a Wikipedia contributor, and last year there was a community survey among Wikipedians where changing the fragment appearance from #R.C3.A9sum.C3.A9 to #Résumé and from #.D0.9D.D0.BE.D0.B2.D1.8B.D0.B9... to #Новый... was the #3 wish out of 265: https://meta.wikimedia.org/wiki/2016_Community_Wishlist_Survey/Results.

This was implemented successfully by Wikimedia developers, only to be nullified by the recent Chrome 63.0.3239.84 release (https://chromereleases.googleblog.com/2017/12/stable-channel-update-for-desktop.html). And now #Новый looks like #%D0%9D%D0%BE%D0%B2%D1%8B%D0%B9.

According to the linked page, this was done out of some security considerations. This is fine, but it's unclear why the fragment can't be _displayed_ decoded in the address bar, as is the case with the path. This behaviour doesn't seem consistent.

So, in actuality (when sent to server, as well as copied to clipboard):
- path is encoded
- fragment is encoded

In the address bar:
- path is decoded
- fragment is encoded

For example (see the attachment):
- in actuality: https://en.wikipedia.org/wiki/R%C3%A9sum%C3%A9#R%C3%A9sum%C3%A9_evaluation
- in the address bar: https://en.wikipedia.org/wiki/Résumé#R%C3%A9sum%C3%A9_evaluation

In Wikipedia (which is, let me remind you, the 5th top site in the world according to Alexa), fragment links appear in the table of contents on Wikipedia, as well as the image links, so they are widespread there.

Firefox has fragment prettification in the address bar for a long time already, while Chrome has no.
Résumé - Wikipedia.png
19.9 KB View Download
Re #21: I expect that display will show decided values in 65 or even 64. See #20.
Re #22: Thank you for taking the time to answer that question.
Project Member

Comment 24 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8625da2e7215aaac6c0c76a636ab8483090943d

commit f8625da2e7215aaac6c0c76a636ab8483090943d
Author: Eric Lawrence <elawrence@chromium.org>
Date: Thu Dec 14 18:36:37 2017

Unescape fragment for display in Omnibox

A change in Chrome 63 causes Chrome to urlencode the fragment component
of URLs. The Chrome omnibox should decode the fragment component as it
does for other URL components and matching Firefox.

Bug:  789163 
Change-Id: If82d72445d904495c8d3e4129ab63be141745328
Reviewed-on: https://chromium-review.googlesource.com/822819
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524123}
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/url_formatter_unittest.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e486ef03b1649a5749cd875b03898b2b1600c055

commit e486ef03b1649a5749cd875b03898b2b1600c055
Author: Peter Laurens <peterlaurens@chromium.org>
Date: Thu Dec 14 22:59:00 2017

Revert "Unescape fragment for display in Omnibox"

This reverts commit f8625da2e7215aaac6c0c76a636ab8483090943d.

Reason for revert: This change breaks EGTests for iOS Simulator and Device, see:

https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2Fios-simulator-full-configs%2F1434%2F%2B%2Frecipes%2Fsteps%2Fios_chrome_web_egtests__iPhone_6s_Plus_iOS_10.0_%2F0%2Fstdout

Original change's description:
> Unescape fragment for display in Omnibox
> 
> A change in Chrome 63 causes Chrome to urlencode the fragment component
> of URLs. The Chrome omnibox should decode the fragment component as it
> does for other URL components and matching Firefox.
> 
> Bug:  789163 
> Change-Id: If82d72445d904495c8d3e4129ab63be141745328
> Reviewed-on: https://chromium-review.googlesource.com/822819
> Commit-Queue: Eric Lawrence <elawrence@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#524123}

TBR=pkasting@chromium.org,elawrence@chromium.org

Change-Id: I9572e8a5d66381478c1faca0f78a66b878a4b059
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  789163 
Reviewed-on: https://chromium-review.googlesource.com/828080
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524221}
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/url_formatter_unittest.cc

Cc: brajkumar@chromium.org
 Issue 795164  has been merged into this issue.
Project Member

Comment 27 by bugdroid1@chromium.org, Dec 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03f9a90d8a783f9d1a94935ac298338a1e694380

commit 03f9a90d8a783f9d1a94935ac298338a1e694380
Author: Eric Lawrence <elawrence@chromium.org>
Date: Sat Dec 16 04:48:11 2017

Reland of 'Unescape fragment for display in Omnibox'

The original landing broke EGTests for iOS Simulator and Device which
were not run by the CQ. This change includes updated EGTests.

TBR=pkasting@chromium.org

Bug:  789163 , 643458
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ie30afcb7bcba5affc7266d72a10b7f8dc0074314
Reviewed-on: https://chromium-review.googlesource.com/830093
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524591}
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/url_formatter_unittest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/ios/chrome/browser/web/push_and_replace_state_navigation_egtest.mm

Owner: elawrence@chromium.org
Status: Assigned (was: Untriaged)
Status: Fixed (was: Assigned)
Fixed in m65 via cl in c27. 
Labels: Merge-Request-64
Should we merge this fix to M-64?
Project Member

Comment 31 by sheriffbot@chromium.org, Jan 2 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
My recommendation is to wait until M65. If it's critical, can you please provide more justification for why this is needed for 64, and how safe this merge is overall? Since this has been present in M62 and change seems quite complex, my recommendation is to take it for M65. 
Re #32: The regression was introduced in M63 and discovered late in M63. It should be a fairly safe merge for 64, but since we don't seem to be getting a ton of complaints about this, I don't object to waiting to M65.
+1 on letting it M65 since it does not seem critical.
Labels: -Merge-Review-64 Merge-Rejected-64
 Issue 804867  has been merged into this issue.
So, it will be in 65, right?
Re #37, as noted in #29, yes, the fix landed in 65.0.3297.0.

Sign in to add a comment