AutocompleteMatch::GURLToStrippedGURL can make invalid stripped_url from valid destination url
Reported by
dyaros...@yandex-team.ru,
Mar 30 2016
|
||
Issue descriptionChrome Version : 51.0.2690.0 (Developer Build) (64-bit) What steps will reproduce the problem? (1) input in omnibox https://www. (2) destination url https://www./ is valid (3) GURLToStrippedGURL returns url: https:/, which is invalid What is the expected result? expected a valid stripped_url
,
Mar 30 2016
(For some reason, I can not edit the issue). I came across strange behaviour in AutocompleteMatch::GURLToStrippedGURL, defined in components/omnibox/browser/autocomplete_match.cc. It can produce an invalid stripped_destination_url (GURL::is_valid() returns false) for valid input url and does strange url trimming. For example, if input url is https://www./ , then output is https:/ (notice one '/'). Looks like a bug. This behaviour can be reproduced, if "http://www." is inputed in omnibox. pkasting@chromium.org, could you please take a look?
,
Mar 30 2016
This is intentional. The resulting "URLs" are not guaranteed to be valid, should not be used, and, even if valid, sometimes result in fetching a different page than the original URL. I'm sorry the comments don't make this clear; they kind of hint at it around here: https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox/browser/autocomplete_match.h&sq=package:chromium&l=196-197 Please feel free to send me or pkasting@ a code review to improve the comments. If you can explain why you wanted to do with GURLToStrippedGURL(), perhaps we can point you at a better approach for your problem.
,
Mar 30 2016
Thanks, I've just suspected, that it was a bug.
,
Mar 30 2016
It would be really nice to have a comment here, where the member is declared. https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox/browser/autocomplete_match.h&l=349 because, it's not clear, that you have to look at GURLToStrippedGURL comment.
,
Mar 30 2016
What sort of comment are you wanting? The comment there already notes this is for dupe-finding, which should imply that it's not for e.g. navigating to. Or, to rephrase: what were you thinking this member should be for, that it's not actually for?
,
Mar 30 2016
It was my understanding, that this member could be used as canonical form of destination url. So I've tried to use it as an url. But, in fact, it's not - it's precomputed hash to compare destination urls. Smth like mpear..'s "The resulting "URLs" are not guaranteed to be valid, should not be used, and, even if valid, sometimes result in fetching a different page than the original URL" would be great.
,
Mar 30 2016
I'm still a bit mystified. The comment on |destination_url| directly above this item explicitly says it is the canonical URL. Isn't that sufficient? I mean, it's not too hard to add more comments, but I'm a little reluctant to restate the same information on successive members. Also, Mark's text sounds more forbidding than we want; "should not be used" isn't really true (since why would we have this if we can't use it for anything? when in fact it is used, for dupe-finding), and "sometimes result in fetching a different page" implies that we ever fetch with these, which should never be true. Please don't stop pushing for an improvement here if there is one that really adds clarity and doesn't muddle things further. I'm just trying to make sure any changes we make really are improvements.
,
Mar 30 2016
This issue arised, because it was quite surprising, that stripped_destination_url might be invalid for valid destination_url. Now I know, that it's just a hash, it's ok. But I was expecting it to be a meaningful url, when in fact, it's not. Maybe I shouldn't have.
,
Mar 30 2016
Well, it is a URL, basically. Honestly, I kind of consider it a bug that it transforms "https://www./" into "https://" or similar, since in that case "www." is really a fully-qualified hostname and not a prefix. But it's a bug that I can't imagine causing any problems. I don't think "hash" is a correct description, is what I guess I'm saying. It's the destination URL with some pieces of the string removed. I don't know how to describe it better than that.
,
Apr 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ff3b19b795770b4da5f755661f5eb6110e6891c commit 0ff3b19b795770b4da5f755661f5eb6110e6891c Author: mpearson <mpearson@chromium.org> Date: Wed Apr 06 00:19:40 2016 Update Comment by stripped_destination_url for clarity BUG= 599096 Review URL: https://codereview.chromium.org/1866513002 Cr-Commit-Position: refs/heads/master@{#385342} [modify] https://crrev.com/0ff3b19b795770b4da5f755661f5eb6110e6891c/components/omnibox/browser/autocomplete_match.h |
||
►
Sign in to add a comment |
||
Comment 1 by ashej...@chromium.org
, Mar 30 2016Components: UI>Browser>Omnibox