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

Issue 859062 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Referrers are cleared for all link transitions from remote NTP

Project Member Reported by a-...@yandex-team.ru, Jun 29 2018

Issue description

Chrome 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



 
I have created this bug from analyze performed in https://bugs.chromium.org/p/chromium/issues/detail?id=718516
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. 

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.
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"

Components: -UI>Browser>NewTabPage Internals>Sandbox>SiteIsolation
Status: Assigned (was: Untriaged)
This bug has an owner, thus, it's been triaged. Changing status to "assigned".
Cc: ramyan@chromium.org
Status: Started (was: Assigned)
Project Member

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

Status: Fixed (was: Started)

Sign in to add a comment