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

Issue 599096 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

AutocompleteMatch::GURLToStrippedGURL can make invalid stripped_url from valid destination url

Reported by dyaros...@yandex-team.ru, Mar 30 2016

Issue description

Chrome 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
 
Cc: ashej...@chromium.org
Components: UI>Browser>Omnibox

Comment 2 Deleted

(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?
Status: WontFix (was: Unconfirmed)
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.

Thanks, I've just suspected, that it was a bug.
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.
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?
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.

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

Comment 12 by bugdroid1@chromium.org, 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