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

Issue 848446 link

Starred by 2 users
Status: Fixed
Owner:
█ nasko@chromium.org
Out until 24 Jan
Closed: Jun 2018
Cc:
clamy@chromium.org
creis@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Clean up HasWrongProcessForURL logic in Reload

Project Member Reported by creis@chromium.org, May 31 2018 
In  issue 847718 , we noted some strange behavior where reloads of renderer-initiated cross-site navigations (which haven't committed yet) are hitting the HasWrongProcessForURL block of NavigationControllerImpl::Reload, which was designed for hosted app reloads that need to swap processes.

This is not intentional and can lead to some bad side effects.  It's due to proactively setting the SiteInstance on a pending NavigationEntry to the SiteInstance of the initiator, even when the navigation will end up in a different process due to Site Isolation.

The HasWrongProcessForURL code itself seems like it could be improved or possibly handled elsewhere.

We should look into:
1) Not setting the SiteInstance on the pending NavigationEntry if possible.  (This is related to issue 803859.)
2) Removing or cleaning up the HasWrongProcessForURL code in Reload to handle the hosted app reload case in a better way.  (Initial strawman CL at https://chromium-review.googlesource.com/c/chromium/src/+/1080326 shows that the code can't just be removed without other changes, though.)

Sounds like Nasko can take a look at these in more detail.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 15 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aee2f86ac16f51525a7058c5339a8dcad3a38bc7

commit aee2f86ac16f51525a7058c5339a8dcad3a38bc7
Author: Nasko Oskov <nasko@chromium.org>
Date: Fri Jun 15 00:05:52 2018

Fix session history navigations and reloads which change SiteInstance.

Reloading an URL usually does not change its SiteInstance, however there
are a couple of exceptions - hosted apps and recently isolation of error
pages. In both of these cases, a commit for a specific URL can happen
in SiteInstance A and a subsequent reload results in a change to
SiteInstance B. With error page isolation, this can happen easily due
to timeout or some other transient network error on the initial navigation
to the URL, which ends up in an error page process. A later successful
reload then results in a different SiteInstance for the same URL.

Session history navigations are similar in behavior. When navigating
back/forward, the navigation can be redirected cross-site, resulting
in a SiteInstance change.

This CL changes ClassifyNavigation to account for these and classifies
them as NEW_PAGE with replacement. It ensures that when an entry's
SiteInstance changes, which means the security context has changed,
we don't reuse and update the existing entry, but rather we replace it
with a newly constructed one.

The CL adds a specific test for session history navigations that are
redirected and adds session history length checks to the existing test
of reloading a hosted app, which changes SiteInstances.

Bug:  840485 ,  848446 
Change-Id: Iabacc60ce5726d5d919877b6e65e5d2f51dff3e2
Reviewed-on: https://chromium-review.googlesource.com/1086876
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567492}
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/chrome/browser/extensions/app_process_apitest.cc
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/aee2f86ac16f51525a7058c5339a8dcad3a38bc7/content/browser/frame_host/render_frame_host_manager_browsertest.cc

Comment 2 by nasko@chromium.org, Jun 15 2018 

The usage of HasWrongProcessForURL in Reload is now removed. The initial comment has another part:

"1) Not setting the SiteInstance on the pending NavigationEntry if possible.  (This is related to issue 803859.)"

Given that it is already tracked by issue 803859, I think we can consider closing this one as fixed and continue tracking 803859.

Comment 3 by creis@chromium.org, Jun 15 2018

Status: Fixed (was: Assigned)
That part was referring to the set_site_instance line in NavigatorImpl::DidStartMainFrameNavigation, which you already removed in r564315 for issue 849430.  :)  I think this is done-- thanks!
►

Sign in to add a comment