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

Issue 849430 link

Starred by 1 user
Status: Assigned
Owner:
█ nasko@chromium.org
Out until 24 Jan
Cc:
clamy@chromium.org
creis@chromium.org
alex...@chromium.org
█ nasko@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 840485



Sign in to add a comment

SiteInstance of FrameNavigationEntry should not change after it is set

Project Member Reported by nasko@chromium.org, Jun 4 2018 
Session history in the browser is implemented through NavigationEntry and FrameNavigationEntry. Once a navigation has committed, an FrameNavigationEntry is created for it and it stores the SiteInstance associated with the committed document. It is used to pick the right process to use when performing session history navigations and/or reloading the page. However, it is possible for the SiteInstance for an URL to change (e.g. hosted app is installed, server redirect) between the first time it was committed and any subsequent navigation to the same FrameNavigationEntry.

Currently, the SiteInstance of FrameNavigationEntry can be updated through either FrameNavigationEntry::set_site_instance() or through FrameNavigationEntry::UpdateEntry. This can lead to partially updating a FrameNavigationEntry and resulting an entry that mixes data that came from two different security contexts, which is potentially bad.

Going forward, we should change how we treat navigations to existing FrameNavigationEntry which change its SiteInstance. Currently, they are classified as NAVIGATION_TYPE_EXISTING_PAGE and the entry is updated. Instead, they should be classified NAVIGATION_TYPE_NEW_PAGE with |repalce_entry| set to true. This will result in not altering session history, except for the entry that committed and it will be replaced with a completely newly constructed one and avoid carrying over any stale data over.

Comment 1 by creis@chromium.org, Jun 4 2018 

Right, we should do this.  For context, I was careful to ensure that FNE::UpdateEntry updated *all* values of the FNE whenever the SiteInstance changed, so that at least there weren't stale values on the FNE itself.  However, that doesn't help with everything else on the NavigationEntry it's part of, so I'd much prefer to treat any SiteInstance-altering navigation as new with replacement.

(Note that in the subframe case, I think we'll probably want it to be classified as NAVIGATION_TYPE_NEW_SUBFRAME with replacement.)

Happy to see that part 1 is in review at https://chromium-review.googlesource.com/c/chromium/src/+/1083596, by restricting set_site_instance.  Part 2 will involve changing the classification to avoid UpdateEntry, which will require more work.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 5 2018 

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

commit c5284710d1a71f3b21958bcfdea530bec4c70cd0
Author: Nasko Oskov <nasko@chromium.org>
Date: Tue Jun 05 00:30:55 2018

Part 1: Do not allow directly changing SiteInstance on FrameNavigationEntry.

Session history keeps a SiteInstance associated with each entry to ensure
that back/forward navigations and reloads can correctly load in the right
process. However, it is possible that the SiteInstance changes between
the time the navigation that caused the session history entry to be
created and the subsequent history navigation/reload. In such case, a new
session history entry should be created with replacement instead of
reusing the existing entry.

This implies that SiteInstance should never be updated on its own for a
FrameNavigationEntry, so this CL is the first step along the way. It is
removing its setter for pending entries and enforces that it is never
changed once set. The follow up will do the same for updating entries,
however it does need to change how NavigationController works, so
it will come separately.

Bug: 849430
Change-Id: Ia0a21d55d9a81bd94b06fac97de1b04ecdd95fdc
Reviewed-on: https://chromium-review.googlesource.com/1083596
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564315}
[modify] https://crrev.com/c5284710d1a71f3b21958bcfdea530bec4c70cd0/content/browser/frame_host/frame_navigation_entry.h
[modify] https://crrev.com/c5284710d1a71f3b21958bcfdea530bec4c70cd0/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/c5284710d1a71f3b21958bcfdea530bec4c70cd0/content/browser/site_instance_impl_unittest.cc

Comment 3 by benhenry@chromium.org, Aug 3

Status: Assigned (was: Untriaged)
This bug has an owner, thus, it's been triaged. Changing status to "assigned".
►

Sign in to add a comment