Referrers are cleared for all link transitions from remote NTP |
||||||
Issue descriptionChrome Version: 69.0.3474.0 (Официальная сборка) canary (64 бит) (cohort: Clang-64) OS: Win7 What steps will reproduce the problem? 1. Choose Yandex as default search engine. 2. Open NTP 3. Start typing "onlime" in NTP search editbox 4. Select and click with mouse first navigational suggest in suggest popup - "onlime.ru" 5. Another tab opens with onlime.ru site open. 6. Open devtools in onlime.ru tab, select console and enter "document.referrer" Expected value - not empty referrer string containing something like "https://yandex.ru/clck/jsredir?from=yandex.ru%3Bsuggest..." Actual value - empty string. Problem is not reproduced with same steps if I create tab and navigate to yandex.ru and repeat steps 3-6 from above. I have created video with steps to reproduce https://drive.google.com/open?id=1ZNvTwOwfePv_YlR0gVYds0mauEDFdqLX This is regression after 2 CLs: https://codereview.chromium.org/2084953003 - implemented Referrer reset for all links navigations from NTP https://chromium-review.googlesource.com/c/chromium/src/+/998178 - started to apply Referrer reset to navigations from Yandex suggest in remote NTP
,
Jun 29 2018
Ok, I have found how CL(https://chromium-review.googlesource.com/c/chromium/src/+/998178) broken our(Yandex NTP) navigation stats that used referrers: I omit some details, to keep description readable. Old behaviour: 0. User set Yandex as default search 1. User opens NTP - Yandex remote NTP page is opened, SiteInstance for it created, for example SiteInstance id=1 2. User types in Yandex NTP, in search editbox - "onlime.ru" and click with mouse on first navigation suggest. 3. Navigation to "https://yandex.ru/clck/jsredir?long_query_string" is initiated. To check if current NTP SiteInstance(id=1) can be reused for destination url, function RenderFrameHostManager::GetSiteInstanceForNavigationRequest calls indirectly RenderFrameHostManager::IsCurrentlySameSite function with "https://yandex.ru/clck/jsredir?long_query_string" as dest_url param. Old version of IsCurrentlySameSite function returned false for this request. New SiteInstance(id=2) was created for destination url, it navigated to "https://yandex.ru/clck/jsredir?long_query_string" which immediately redirected itself to "http://www.onlime.ru" - also in separate SiteInstance. Referrer from SiteInstance(id=2) was correctly passed to "http://www.onlime.ru" site. New behaviour: Steps 0,1,2 - same as above. 3. Navigation to "https://yandex.ru/clck/jsredir?long_query_string" is initiated. To check if current NTP SiteInstance(id=1) can be used for destination url, function RenderFrameHostManager::GetSiteInstanceForNavigationRequest calls indirectly RenderFrameHostManager::IsCurrentlySameSite function with "https://yandex.ru/clck/jsredir?long_query_string" as dest_url param. New version of IsCurrentlySameSite returnes true. Old NTP SiteInstance(id=1) is reused to navigate to "https://yandex.ru/clck/jsredir?long_query_string". It navigates to this new link and when immediately redirected itself to "http://www.onlime.ru". Referrer from SiteInstance(id=1) is cleared in function ChromeContentBrowserClient::OverrideNavigationParams while navigating to "http://www.onlime.ru" site. Function OverrideNavigationParams is implemented so it clears all referrers for transitions from NTP site instance for transition type PAGE_TRANSITION_LINK. This is done to fix https://codereview.chromium.org/2084953003. What problems I see here: 1. Logic that clears all referrers and overrides transition type for all link navigation from NTP implemented in ChromeContentBrowserClient::OverrideNavigationParams seems like a very crude solution for https://codereview.chromium.org/2084953003. Yandex remote NTP contains link elements other than TopSites and clearing referrers for them is clearly incorrect. 2. New version of IsCurrentlySameSite function implemented in https://chromium-review.googlesource.com/c/chromium/src/+/998178 allows to reuse NTP SiteInstance for all transition to same-host URLs (that's how I understand it). I am not sure that this is intendend result of source CL. What can solve problem: 1. Revert CL https://chromium-review.googlesource.com/c/chromium/src/+/998178. Or change IsCurrentlySameSite so it will not reuse NTP SiteInstance for navigations. 2. Rework OverrideNavigationParams so it will more intelligently detect clicks on TopSites. WDYT? For me, the simplest and fastest solution is revert of CL mentioned in item 1, and further work on both items.
,
Jun 29 2018
Thanks for the report and investigation so far! I don't think we can revert r549321, because that fixed a launch-blocking site isolation bug with Hangouts (details in Google-internal issue 828720). Site isolation has launched for everyone in M67. I think that fix only needed to apply to hosted apps, so maybe we can find a way to exclude NTP from the IsCurrentlySameSite checks and have them apply only to hosted apps? That's tricky because content/ only knows about effective URLs but not whether they're for NTP vs. hosted apps. Maybe SiteInstanceImpl::HasEffectiveURL should also call out to the embedder and only return true for hosted apps but not NTP? I'm also curious why https://yandex.ru/clck/jsredir?long_query_string is considered part of the remote NTP. I'm guessing this plumbs to search::IsNTPOrServiceWorkerURL() from ChromeContentBrowserClient::GetEffectiveURL() and search::ShouldAssignURLToInstantRenderer(). What does GetNewTabPageURL() return for Yandex's NTP there, and is there a way to make that not match the clck URL? If possible, I'd prefer to fix things there instead of special-casing IsCurrentlySameSite to exclude NTP.
,
Jul 5
Hi alex. I am thinking about minimum solution for promblem. About your last question - "why https://yandex.ru/clck/jsredir?long_query_string is considered part of the remote NTP." HasEffectiveURL() function returns false for "https://yandex.ru/clck/jsredir?long_query_string" GetNewTabPageURL() returns "https://www.yandex.ru/chrome/newtab" for Yandex remote NTP Logic in RenderFrameHostManager::IsCurrentlySameSite goes like this for input params: RenderFrameHostImpl* candidate(original_url="https://www.yandex.ru/chrome/newtab") dest_url = "https://yandex.ru/clck/jsredir?long_query_string" bool src_has_effective_url = true // because candidate is NTP RenderFrame bool dest_has_effective_url = false // destination url is not considered an NTP url bool should_compare_effective_urls = false // after assignment inside if statement https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_manager.cc?type=cs&q=IsCurrentlySa&g=0&l=1544 bool should_check_for_wrong_process = false // as a result of all bool values calculated above Next check for HasWrongProcessForURL is skipped and bool value "true" is returned from comparison of SiteInstanceImpl::IsSameWebSite("https://www.yandex.ru/chrome/newtab", "https://yandex.ru/clck/jsredir?long_query_string", should_compare_effective_urls = false) https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_manager.cc?type=cs&q=IsCurrentlySa&g=0&l=1568 because both NTP and destination url have same host "https://www.yandex.ru"
,
Jul 17
,
Aug 3
This bug has an owner, thus, it's been triaged. Changing status to "assigned".
,
Aug 7
,
Aug 13
,
Aug 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909 commit 6aedc6ecdaf896a00e7e50c97ab072feb8ea0909 Author: Alexander Yashkin <a-v-y@yandex-team.ru> Date: Wed Aug 15 08:58:45 2018 Fixed NTP site instance sharing bug This fixes the bug when every window opened from remote NTP tab with same host as remote NTP but different path would be considered to belong to same SiteInstance as NTP. That could lead to nasty effects, such as referrer reset for transfers from Yandex remote NTP. Basically its a rewrite of https://chromium-review.googlesource.com/c/chromium/src/+/998178 patch so it would apply to hosted apps only, as intended, IMHO. Bug: 859062 ,718516 Change-Id: I76e90c2ff6311ca99a234e75424f8cba9932c05a Reviewed-on: https://chromium-review.googlesource.com/1141574 Commit-Queue: Alexander Yashkin <a-v-y@yandex-team.ru> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#583199} [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/chrome_content_browser_client_browsertest.cc [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/public/browser/content_browser_client.cc [modify] https://crrev.com/6aedc6ecdaf896a00e7e50c97ab072feb8ea0909/content/public/browser/content_browser_client.h
,
Aug 15
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by a-...@yandex-team.ru
, Jun 29 2018