Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949023002:
[WIP] Fix history.replaceState() not propagating back favicons (Closed)
|
Created:
3 years, 6 months ago by mastiz Modified:
3 years, 4 months ago Reviewers:
pkotwicz CC:
chromium-reviews, pam+watch_chromium.org, jam, browser-components-watch_chromium.org, sync-reviews_chromium.org, darin-cc_chromium.org, sky, Charlie Reis, Nate Chapin Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix history.replaceState() not propagating back favicons
When a page URL replaces another one in history, the transition can be
treated similarly to redirect chains such that the favicons are
associated to all page URLs. This makes sense because the actual page
content doesn't really change.
The special handling is needed because favicons might not have been
reported for the first URL, prior to the replace, since WebContents
calls WebContentsObserver::DidUpdateFaviconURL() late during the page
load.
BUG=741340, 736254
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated. #Patch Set 3 : Rebased. #Patch Set 4 : Rebased. #Patch Set 5 : Focus on history.replaceState() #Messages
Total messages: 22 (9 generated)
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
pkotwicz@: consider this a RFC/WIP, I'm trying to understand the situation better.
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
Scott is a better reviewer than I. The idea behind the CL sounds reasonable to me.
https://codereview.chromium.org/2949023002/diff/1/chrome/browser/favicon/cont... File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right): https://codereview.chromium.org/2949023002/diff/1/chrome/browser/favicon/cont... chrome/browser/favicon/content_favicon_driver_browsertest.cc:417: IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, What is android/i-os specific here? https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... File chrome/browser/history/history_tab_helper.cc (right): https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... chrome/browser/history/history_tab_helper.cc:85: // TODO / DONOTSUBMIT(mastiz): if we really go with IsSameDocument, the ? https://codereview.chromium.org/2949023002/diff/1/components/history/core/bro... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2949023002/diff/1/components/history/core/bro... components/history/core/browser/history_backend.cc:475: RedirectList redirects = request.redirects; This is worth a comment. Also, avoid the potential unnecessary copy of redirects here. By that I mean do something like: RedirectList redirects; if (your-new-code) { } else { redirects = request.redirects; } https://codereview.chromium.org/2949023002/diff/1/components/history/core/bro... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2949023002/diff/1/components/history/core/bro... components/history/core/browser/history_service.cc:384: replaced_entry_url, true)); If you're going to pass around values, shouldn't you use std::move to avoid unnecessary copying?
https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... File chrome/browser/history/history_tab_helper.cc (right): https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... chrome/browser/history/history_tab_helper.cc:85: // TODO / DONOTSUBMIT(mastiz): if we really go with IsSameDocument, the On 2017/06/22 15:26:39, sky wrote: > ? Sorry scott, the "RFC" in the title was trying to emphasize that this CL is not in a state to be landed, or even properly reviewed. I'm looking for guidance and people with expertise in the area of client-side redirects, including meta-refresh tags and javascript-initiated (reflected in browser tests). This particular TODO is one of the key questions, as per whether either of IsSameDocument() or DidReplaceEntry() is appropriate for this purpose.
Description was changed from ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=498436 ========== to ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=498436 ==========
sky@chromium.org changed reviewers: + creis@chromium.org
On 2017/06/22 20:31:10, mastiz wrote: > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > File chrome/browser/history/history_tab_helper.cc (right): > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > chrome/browser/history/history_tab_helper.cc:85: // TODO / DONOTSUBMIT(mastiz): > if we really go with IsSameDocument, the > On 2017/06/22 15:26:39, sky wrote: > > ? > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is not > in a state to be landed, or even properly reviewed. > > I'm looking for guidance and people with expertise in the area of client-side > redirects, including meta-refresh tags and javascript-initiated (reflected in > browser tests). This particular TODO is one of the key questions, as per whether > either of IsSameDocument() or DidReplaceEntry() is appropriate for this purpose. +creis Hopefully Charlie is a good source to help answer this question, and if not he hopefully knows who to point you at.
On 2017/06/22 21:15:16, sky wrote: > On 2017/06/22 20:31:10, mastiz wrote: > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > DONOTSUBMIT(mastiz): > > if we really go with IsSameDocument, the > > On 2017/06/22 15:26:39, sky wrote: > > > ? > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is > not > > in a state to be landed, or even properly reviewed. > > > > I'm looking for guidance and people with expertise in the area of client-side > > redirects, including meta-refresh tags and javascript-initiated (reflected in > > browser tests). This particular TODO is one of the key questions, as per > whether > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > purpose. > > +creis > > Hopefully Charlie is a good source to help answer this question, and if not he > hopefully knows who to point you at. creis@: friendly ping. And please keep in mind that it's a request for clarification, not an actual patch that is ready to be reviewed. See paragraph above from my previous reply to scott.
creis@chromium.org changed reviewers: + japhet@chromium.org
On 2017/06/27 08:39:01, mastiz wrote: > On 2017/06/22 21:15:16, sky wrote: > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > DONOTSUBMIT(mastiz): > > > if we really go with IsSameDocument, the > > > On 2017/06/22 15:26:39, sky wrote: > > > > ? > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is > > not > > > in a state to be landed, or even properly reviewed. > > > > > > I'm looking for guidance and people with expertise in the area of > client-side > > > redirects, including meta-refresh tags and javascript-initiated (reflected > in > > > browser tests). This particular TODO is one of the key questions, as per > > whether > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > purpose. > > > > +creis > > > > Hopefully Charlie is a good source to help answer this question, and if not he > > hopefully knows who to point you at. > > creis@: friendly ping. And please keep in mind that it's a request for > clarification, not an actual patch that is ready to be reviewed. See paragraph > above from my previous reply to scott. Sorry about that-- I'm pretty overloaded at the moment, trying to catch up on a large number of reviews after traveling last week. We could wait for Nate to get back? Or we could set up a VC and I can try to share whatever I can. https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... File chrome/browser/history/history_tab_helper.cc (right): https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... chrome/browser/history/history_tab_helper.cc:85: // TODO / DONOTSUBMIT(mastiz): if we really go with IsSameDocument, the On 2017/06/22 20:31:09, mastiz wrote: > On 2017/06/22 15:26:39, sky wrote: > > ? > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is not > in a state to be landed, or even properly reviewed. > > I'm looking for guidance and people with expertise in the area of client-side > redirects, including meta-refresh tags and javascript-initiated (reflected in > browser tests). This particular TODO is one of the key questions, as per whether > either of IsSameDocument() or DidReplaceEntry() is appropriate for this purpose. I think japhet@ is probably the best person to ask about that area, but he's OOO at the moment. I'm loosely familiar.
Description was changed from ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=498436 ========== to ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=498436,736254 ==========
On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > On 2017/06/27 08:39:01, mastiz wrote: > > On 2017/06/22 21:15:16, sky wrote: > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > DONOTSUBMIT(mastiz): > > > > if we really go with IsSameDocument, the > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > ? > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL > is > > > not > > > > in a state to be landed, or even properly reviewed. > > > > > > > > I'm looking for guidance and people with expertise in the area of > > client-side > > > > redirects, including meta-refresh tags and javascript-initiated (reflected > > in > > > > browser tests). This particular TODO is one of the key questions, as per > > > whether > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > > purpose. > > > > > > +creis > > > > > > Hopefully Charlie is a good source to help answer this question, and if not > he > > > hopefully knows who to point you at. > > > > creis@: friendly ping. And please keep in mind that it's a request for > > clarification, not an actual patch that is ready to be reviewed. See paragraph > > above from my previous reply to scott. > > Sorry about that-- I'm pretty overloaded at the moment, trying to catch up on a > large number of reviews after traveling last week. We could wait for Nate to > get back? Or we could set up a VC and I can try to share whatever I can. > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > File chrome/browser/history/history_tab_helper.cc (right): > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > chrome/browser/history/history_tab_helper.cc:85: // TODO / DONOTSUBMIT(mastiz): > if we really go with IsSameDocument, the > On 2017/06/22 20:31:09, mastiz wrote: > > On 2017/06/22 15:26:39, sky wrote: > > > ? > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is > not > > in a state to be landed, or even properly reviewed. > > > > I'm looking for guidance and people with expertise in the area of client-side > > redirects, including meta-refresh tags and javascript-initiated (reflected in > > browser tests). This particular TODO is one of the key questions, as per > whether > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > purpose. > > I think japhet@ is probably the best person to ask about that area, but he's OOO > at the moment. I'm loosely familiar. Friendly ping for japhet@, thanks.
On 2017/07/05 06:32:11, mastiz wrote: > On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > > On 2017/06/27 08:39:01, mastiz wrote: > > > On 2017/06/22 21:15:16, sky wrote: > > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > DONOTSUBMIT(mastiz): > > > > > if we really go with IsSameDocument, the > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > ? > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL > > is > > > > not > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > client-side > > > > > redirects, including meta-refresh tags and javascript-initiated > (reflected > > > in > > > > > browser tests). This particular TODO is one of the key questions, as per > > > > whether > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > > > purpose. > > > > > > > > +creis > > > > > > > > Hopefully Charlie is a good source to help answer this question, and if > not > > he > > > > hopefully knows who to point you at. > > > > > > creis@: friendly ping. And please keep in mind that it's a request for > > > clarification, not an actual patch that is ready to be reviewed. See > paragraph > > > above from my previous reply to scott. > > > > Sorry about that-- I'm pretty overloaded at the moment, trying to catch up on > a > > large number of reviews after traveling last week. We could wait for Nate to > > get back? Or we could set up a VC and I can try to share whatever I can. > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > DONOTSUBMIT(mastiz): > > if we really go with IsSameDocument, the > > On 2017/06/22 20:31:09, mastiz wrote: > > > On 2017/06/22 15:26:39, sky wrote: > > > > ? > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL is > > not > > > in a state to be landed, or even properly reviewed. > > > > > > I'm looking for guidance and people with expertise in the area of > client-side > > > redirects, including meta-refresh tags and javascript-initiated (reflected > in > > > browser tests). This particular TODO is one of the key questions, as per > > whether > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > purpose. > > > > I think japhet@ is probably the best person to ask about that area, but he's > OOO > > at the moment. I'm loosely familiar. > > Friendly ping for japhet@, thanks. Sorry, I'm just back from vacation and digging through the queue... Just to warn you, I'm pretty clueless outside of content/. However, DidReplaceEntry and IsSameDocument seem like distinct bits that shouldn't be mixed if we can avoid it. You can have DidReplaceEntry without IsSameDocument (e.g., location.replace() to a different document) and you can have IsSameDocument without DidReplaceEntry (e.g., history.pushState(), setting location.href after onload completes, etc.). It seems like the bug here may be that DidReplaceEntry is incorrect for some same-document navigations?
On 2017/07/11 17:40:07, Nate Chapin wrote: > On 2017/07/05 06:32:11, mastiz wrote: > > On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > > > On 2017/06/27 08:39:01, mastiz wrote: > > > > On 2017/06/22 21:15:16, sky wrote: > > > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > > DONOTSUBMIT(mastiz): > > > > > > if we really go with IsSameDocument, the > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > ? > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this > CL > > > is > > > > > not > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > > client-side > > > > > > redirects, including meta-refresh tags and javascript-initiated > > (reflected > > > > in > > > > > > browser tests). This particular TODO is one of the key questions, as > per > > > > > whether > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for > this > > > > > purpose. > > > > > > > > > > +creis > > > > > > > > > > Hopefully Charlie is a good source to help answer this question, and if > > not > > > he > > > > > hopefully knows who to point you at. > > > > > > > > creis@: friendly ping. And please keep in mind that it's a request for > > > > clarification, not an actual patch that is ready to be reviewed. See > > paragraph > > > > above from my previous reply to scott. > > > > > > Sorry about that-- I'm pretty overloaded at the moment, trying to catch up > on > > a > > > large number of reviews after traveling last week. We could wait for Nate > to > > > get back? Or we could set up a VC and I can try to share whatever I can. > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > DONOTSUBMIT(mastiz): > > > if we really go with IsSameDocument, the > > > On 2017/06/22 20:31:09, mastiz wrote: > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > ? > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL > is > > > not > > > > in a state to be landed, or even properly reviewed. > > > > > > > > I'm looking for guidance and people with expertise in the area of > > client-side > > > > redirects, including meta-refresh tags and javascript-initiated (reflected > > in > > > > browser tests). This particular TODO is one of the key questions, as per > > > whether > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > > purpose. > > > > > > I think japhet@ is probably the best person to ask about that area, but he's > > OOO > > > at the moment. I'm loosely familiar. > > > > Friendly ping for japhet@, thanks. > > Sorry, I'm just back from vacation and digging through the queue... > > Just to warn you, I'm pretty clueless outside of content/. However, > DidReplaceEntry and IsSameDocument seem like distinct bits that shouldn't be > mixed if we can avoid it. You can have DidReplaceEntry without IsSameDocument > (e.g., location.replace() to a different document) and you can have > IsSameDocument without DidReplaceEntry (e.g., history.pushState(), setting > location.href after onload completes, etc.). > > It seems like the bug here may be that DidReplaceEntry is incorrect for some > same-document navigations? For context related to your last question, please also take a look at https://codereview.chromium.org/2972793002/ In general, it looks like the old code attempts to cover the same-document case, but it does so rather (IMO) poorly, by stripping out the ref in the former history_backend.cc:2103. That could be improved by adopting IsSameDocument() instead, but it wouldn't have user-visible impact IIUC (modulo leaked URLs). Taking one step further, the goal here was to handle DidReplaceEntry cases to a different page, based on the assumption that the favicon is the same for both.
On 2017/07/11 17:54:38, mastiz wrote: > On 2017/07/11 17:40:07, Nate Chapin wrote: > > On 2017/07/05 06:32:11, mastiz wrote: > > > On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > > > > On 2017/06/27 08:39:01, mastiz wrote: > > > > > On 2017/06/22 21:15:16, sky wrote: > > > > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > > > DONOTSUBMIT(mastiz): > > > > > > > if we really go with IsSameDocument, the > > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > > ? > > > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that > this > > CL > > > > is > > > > > > not > > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > > > client-side > > > > > > > redirects, including meta-refresh tags and javascript-initiated > > > (reflected > > > > > in > > > > > > > browser tests). This particular TODO is one of the key questions, as > > per > > > > > > whether > > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for > > this > > > > > > purpose. > > > > > > > > > > > > +creis > > > > > > > > > > > > Hopefully Charlie is a good source to help answer this question, and > if > > > not > > > > he > > > > > > hopefully knows who to point you at. > > > > > > > > > > creis@: friendly ping. And please keep in mind that it's a request for > > > > > clarification, not an actual patch that is ready to be reviewed. See > > > paragraph > > > > > above from my previous reply to scott. > > > > > > > > Sorry about that-- I'm pretty overloaded at the moment, trying to catch up > > on > > > a > > > > large number of reviews after traveling last week. We could wait for Nate > > to > > > > get back? Or we could set up a VC and I can try to share whatever I can. > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > DONOTSUBMIT(mastiz): > > > > if we really go with IsSameDocument, the > > > > On 2017/06/22 20:31:09, mastiz wrote: > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > ? > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this CL > > is > > > > not > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > client-side > > > > > redirects, including meta-refresh tags and javascript-initiated > (reflected > > > in > > > > > browser tests). This particular TODO is one of the key questions, as per > > > > whether > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for this > > > > purpose. > > > > > > > > I think japhet@ is probably the best person to ask about that area, but > he's > > > OOO > > > > at the moment. I'm loosely familiar. > > > > > > Friendly ping for japhet@, thanks. > > > > Sorry, I'm just back from vacation and digging through the queue... > > > > Just to warn you, I'm pretty clueless outside of content/. However, > > DidReplaceEntry and IsSameDocument seem like distinct bits that shouldn't be > > mixed if we can avoid it. You can have DidReplaceEntry without IsSameDocument > > (e.g., location.replace() to a different document) and you can have > > IsSameDocument without DidReplaceEntry (e.g., history.pushState(), setting > > location.href after onload completes, etc.). > > > > It seems like the bug here may be that DidReplaceEntry is incorrect for some > > same-document navigations? > > For context related to your last question, please also take a look at > https://codereview.chromium.org/2972793002/ > > In general, it looks like the old code attempts to cover the same-document case, > but it does so rather (IMO) poorly, by stripping out the ref in the former > history_backend.cc:2103. That could be improved by adopting IsSameDocument() > instead, but it wouldn't have user-visible impact IIUC (modulo leaked URLs). > > Taking one step further, the goal here was to handle DidReplaceEntry cases to a > different page, based on the assumption that the favicon is the same for both. Hrm. I think creis may in fact be better qualified here. My understanding of DidReplaceEntry is based on the should_replace_current_entry bit that the renderer process sends to the browser process, but in looking at https://codereview.chromium.org/2972793002/, it's clear that the browser process produces something subtly different to generate DidReplaceEntry, and I'm unfamiliar with its subtleties.
Description was changed from ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=498436,736254 ========== to ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=741340 ==========
On 2017/07/11 18:02:54, Nate Chapin wrote: > On 2017/07/11 17:54:38, mastiz wrote: > > On 2017/07/11 17:40:07, Nate Chapin wrote: > > > On 2017/07/05 06:32:11, mastiz wrote: > > > > On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > > > > > On 2017/06/27 08:39:01, mastiz wrote: > > > > > > On 2017/06/22 21:15:16, sky wrote: > > > > > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > > > > DONOTSUBMIT(mastiz): > > > > > > > > if we really go with IsSameDocument, the > > > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > > > ? > > > > > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that > > this > > > CL > > > > > is > > > > > > > not > > > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > > > > client-side > > > > > > > > redirects, including meta-refresh tags and javascript-initiated > > > > (reflected > > > > > > in > > > > > > > > browser tests). This particular TODO is one of the key questions, > as > > > per > > > > > > > whether > > > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for > > > this > > > > > > > purpose. > > > > > > > > > > > > > > +creis > > > > > > > > > > > > > > Hopefully Charlie is a good source to help answer this question, and > > if > > > > not > > > > > he > > > > > > > hopefully knows who to point you at. > > > > > > > > > > > > creis@: friendly ping. And please keep in mind that it's a request for > > > > > > clarification, not an actual patch that is ready to be reviewed. See > > > > paragraph > > > > > > above from my previous reply to scott. > > > > > > > > > > Sorry about that-- I'm pretty overloaded at the moment, trying to catch > up > > > on > > > > a > > > > > large number of reviews after traveling last week. We could wait for > Nate > > > to > > > > > get back? Or we could set up a VC and I can try to share whatever I > can. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > DONOTSUBMIT(mastiz): > > > > > if we really go with IsSameDocument, the > > > > > On 2017/06/22 20:31:09, mastiz wrote: > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > ? > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that this > CL > > > is > > > > > not > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > > client-side > > > > > > redirects, including meta-refresh tags and javascript-initiated > > (reflected > > > > in > > > > > > browser tests). This particular TODO is one of the key questions, as > per > > > > > whether > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for > this > > > > > purpose. > > > > > > > > > > I think japhet@ is probably the best person to ask about that area, but > > he's > > > > OOO > > > > > at the moment. I'm loosely familiar. > > > > > > > > Friendly ping for japhet@, thanks. > > > > > > Sorry, I'm just back from vacation and digging through the queue... > > > > > > Just to warn you, I'm pretty clueless outside of content/. However, > > > DidReplaceEntry and IsSameDocument seem like distinct bits that shouldn't be > > > mixed if we can avoid it. You can have DidReplaceEntry without > IsSameDocument > > > (e.g., location.replace() to a different document) and you can have > > > IsSameDocument without DidReplaceEntry (e.g., history.pushState(), setting > > > location.href after onload completes, etc.). > > > > > > It seems like the bug here may be that DidReplaceEntry is incorrect for some > > > same-document navigations? > > > > For context related to your last question, please also take a look at > > https://codereview.chromium.org/2972793002/ > > > > In general, it looks like the old code attempts to cover the same-document > case, > > but it does so rather (IMO) poorly, by stripping out the ref in the former > > history_backend.cc:2103. That could be improved by adopting IsSameDocument() > > instead, but it wouldn't have user-visible impact IIUC (modulo leaked URLs). > > > > Taking one step further, the goal here was to handle DidReplaceEntry cases to > a > > different page, based on the assumption that the favicon is the same for both. > > Hrm. I think creis may in fact be better qualified here. My understanding of > DidReplaceEntry is based on the should_replace_current_entry bit that the > renderer process sends to the browser process, but in looking at > https://codereview.chromium.org/2972793002/, it's clear that the browser process > produces something subtly different to generate DidReplaceEntry, and I'm > unfamiliar with its subtleties. Sorry I didn't get to this before heading out, but it does sound like this is related to our other discussion on https://codereview.chromium.org/2972793002/ (and my attempt to fix it in https://chromium-review.googlesource.com/c/567683). Maybe you can help me understand the history code a bit more when I get back-- it would be great if this has a chance of resolving the HistoryBrowserTest.RedirectHistory failure we're seeing in the other CL(s). And yes, anything we can do to make DidReplaceEntry and IsSameDocument be more independent of each other is an improvement.
Description was changed from ========== RFC: Fix JS-initiated redirects leading to gray favicons/thumbnails crbug.com/498618 was fixed with a workaround to lookup URLs after stripping the fragment (aka ref) of the URL. This doesn't cover all cases though, such as the popular HTML5's history.replaceState(), as reflected in browser tests. As a more generalized approach, we can exploit the information provided contents about the navigation entry (did it replace another one? is it within the same document?). As a side effect, I belive (haven't verified) that a favicon DB leak is fixed, because storing entries for trimmed URLs (fragment/ref stripped out) would never be removed by the history expirer. BUG=741340 ========== to ========== Fix history.replaceState() not propagating back favicons When a page URL replaces another one in history, the transition can be treated similarly to redirect chains such that the favicons are associated to all page URLs. This makes sense because the actual page content doesn't really change. The special handling is needed because favicons might not have been reported for the first URL, prior to the replace, since WebContents calls WebContentsObserver::DidUpdateFaviconURL() late during the page load. BUG=741340,736254 ==========
mastiz@chromium.org changed reviewers: - creis@chromium.org, japhet@chromium.org, sky@chromium.org
On 2017/07/13 16:02:47, Charlie Reis (slow) wrote: > On 2017/07/11 18:02:54, Nate Chapin wrote: > > On 2017/07/11 17:54:38, mastiz wrote: > > > On 2017/07/11 17:40:07, Nate Chapin wrote: > > > > On 2017/07/05 06:32:11, mastiz wrote: > > > > > On 2017/06/27 19:10:06, Charlie Reis (OOO til July 5) wrote: > > > > > > On 2017/06/27 08:39:01, mastiz wrote: > > > > > > > On 2017/06/22 21:15:16, sky wrote: > > > > > > > > On 2017/06/22 20:31:10, mastiz wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > > > > > DONOTSUBMIT(mastiz): > > > > > > > > > if we really go with IsSameDocument, the > > > > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that > > > this > > > > CL > > > > > > is > > > > > > > > not > > > > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area > of > > > > > > > client-side > > > > > > > > > redirects, including meta-refresh tags and javascript-initiated > > > > > (reflected > > > > > > > in > > > > > > > > > browser tests). This particular TODO is one of the key > questions, > > as > > > > per > > > > > > > > whether > > > > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate > for > > > > this > > > > > > > > purpose. > > > > > > > > > > > > > > > > +creis > > > > > > > > > > > > > > > > Hopefully Charlie is a good source to help answer this question, > and > > > if > > > > > not > > > > > > he > > > > > > > > hopefully knows who to point you at. > > > > > > > > > > > > > > creis@: friendly ping. And please keep in mind that it's a request > for > > > > > > > clarification, not an actual patch that is ready to be reviewed. See > > > > > paragraph > > > > > > > above from my previous reply to scott. > > > > > > > > > > > > Sorry about that-- I'm pretty overloaded at the moment, trying to > catch > > up > > > > on > > > > > a > > > > > > large number of reviews after traveling last week. We could wait for > > Nate > > > > to > > > > > > get back? Or we could set up a VC and I can try to share whatever I > > can. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > File chrome/browser/history/history_tab_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2949023002/diff/1/chrome/browser/history/hist... > > > > > > chrome/browser/history/history_tab_helper.cc:85: // TODO / > > > > > DONOTSUBMIT(mastiz): > > > > > > if we really go with IsSameDocument, the > > > > > > On 2017/06/22 20:31:09, mastiz wrote: > > > > > > > On 2017/06/22 15:26:39, sky wrote: > > > > > > > > ? > > > > > > > > > > > > > > Sorry scott, the "RFC" in the title was trying to emphasize that > this > > CL > > > > is > > > > > > not > > > > > > > in a state to be landed, or even properly reviewed. > > > > > > > > > > > > > > I'm looking for guidance and people with expertise in the area of > > > > > client-side > > > > > > > redirects, including meta-refresh tags and javascript-initiated > > > (reflected > > > > > in > > > > > > > browser tests). This particular TODO is one of the key questions, as > > per > > > > > > whether > > > > > > > either of IsSameDocument() or DidReplaceEntry() is appropriate for > > this > > > > > > purpose. > > > > > > > > > > > > I think japhet@ is probably the best person to ask about that area, > but > > > he's > > > > > OOO > > > > > > at the moment. I'm loosely familiar. > > > > > > > > > > Friendly ping for japhet@, thanks. > > > > > > > > Sorry, I'm just back from vacation and digging through the queue... > > > > > > > > Just to warn you, I'm pretty clueless outside of content/. However, > > > > DidReplaceEntry and IsSameDocument seem like distinct bits that shouldn't > be > > > > mixed if we can avoid it. You can have DidReplaceEntry without > > IsSameDocument > > > > (e.g., location.replace() to a different document) and you can have > > > > IsSameDocument without DidReplaceEntry (e.g., history.pushState(), setting > > > > location.href after onload completes, etc.). > > > > > > > > It seems like the bug here may be that DidReplaceEntry is incorrect for > some > > > > same-document navigations? > > > > > > For context related to your last question, please also take a look at > > > https://codereview.chromium.org/2972793002/ > > > > > > In general, it looks like the old code attempts to cover the same-document > > case, > > > but it does so rather (IMO) poorly, by stripping out the ref in the former > > > history_backend.cc:2103. That could be improved by adopting IsSameDocument() > > > instead, but it wouldn't have user-visible impact IIUC (modulo leaked URLs). > > > > > > Taking one step further, the goal here was to handle DidReplaceEntry cases > to > > a > > > different page, based on the assumption that the favicon is the same for > both. > > > > Hrm. I think creis may in fact be better qualified here. My understanding of > > DidReplaceEntry is based on the should_replace_current_entry bit that the > > renderer process sends to the browser process, but in looking at > > https://codereview.chromium.org/2972793002/, it's clear that the browser > process > > produces something subtly different to generate DidReplaceEntry, and I'm > > unfamiliar with its subtleties. > > Sorry I didn't get to this before heading out, but it does sound like this is > related to our other discussion on https://codereview.chromium.org/2972793002/ > (and my attempt to fix it in https://chromium-review.googlesource.com/c/567683). > Maybe you can help me understand the history code a bit more when I get back-- > it would be great if this has a chance of resolving the > HistoryBrowserTest.RedirectHistory failure we're seeing in the other CL(s). And > yes, anything we can do to make DidReplaceEntry and IsSameDocument be more > independent of each other is an improvement. Closing this bug because, is it turns out, replaceState/pushState are considered navigations within the same page, which is a simpler thing to handle, as proposed in https://chromium-review.googlesource.com/c/585130/. I'm adding browser tests that would cover future changes related to https://chromium-review.googlesource.com/c/567683 |
