Skip to content

Commit

Permalink
fix: use appropriate site instance for cross-site nav's (electron#15821)
Browse files Browse the repository at this point in the history 
* fix: use Chromium's determined new site instance as candidate when navigating.

When navigating to a new address, consider using Chromium's determined site instance
for the new page as it should belong to an existing browsing instance when the
navigation was triggered by window.open().

fixes 8100.

* Revert "fix: use Chromium's determined new site instance as candidate when navigating."

This reverts commit eb95f93.

* fix: delegate site instance creation back to content when sandboxed.

* fix: ensure site isolation is on

* test: adapt ut for cross-site navigation

* fix: register pending processes during a navigation.

* refactor: dont call loadURL for a window constructed from an existing webContents.

* test: add sandboxed affinity UT's.

* fix: check affinity before deciding if to force a new site instance.

* chore: adapt subsequent patch.

* refactor: constify logically const methods.

* fix: do not reuse site instances when navigation redirects cross-site.

* test: ensure localStorage accessible after x-site redirect.

* test: adapt localStorage acess denied UT for site isolation.

* fix: do not send render-view-deleted for speculative frames.

* chore: amend tests after rebase.

* test: add ut for webContents' render-view-deleted emission

* fix: introduce current-render-view-deleted for current RVH's deletions.

Revert render-view-deleted to being emitted with any RVH's deletion.
current-render-view-deleted is emitted only when the RVH being deleted
is the current one.

* refactor: style and comments fixed.
  • Loading branch information
@ppontes
ppontes authored and Cheng Zhao committed Dec 5, 2018
1 parent 46e7214 commit d5d1fa8
Show file tree
Hide file tree
Showing 14 changed files with 573 additions and 232 deletions.
21 changes: 21 additions & 0 deletions atom/browser/api/atom_api_web_contents.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,29 @@ void WebContents::RenderViewCreated(content::RenderViewHost* render_view_host) {
impl->disable_hidden_ = !background_throttling_;
}

void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) {
currently_committed_process_id_ = new_host->GetProcess()->GetID();
}

void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
// This event is necessary for tracking any states with respect to
// intermediate render view hosts aka speculative render view hosts. Currently
// used by object-registry.js to ref count remote objects.
Emit("render-view-deleted", render_view_host->GetProcess()->GetID());

if (-1 == currently_committed_process_id_ ||
render_view_host->GetProcess()->GetID() ==
currently_committed_process_id_) {
currently_committed_process_id_ = -1;

// When the RVH that has been deleted is the current RVH it means that the
// the web contents are being closed. This is communicated by this event.
// Currently tracked by guest-window-manager.js to destroy the
// BrowserWindow.
Emit("current-render-view-deleted",
render_view_host->GetProcess()->GetID());
}
}

void WebContents::RenderProcessGone(base::TerminationStatus status) {
Expand Down
6 changes: 6 additions & 0 deletions atom/browser/api/atom_api_web_contents.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,8 @@ class WebContents : public mate::TrackableObject<WebContents>,
// content::WebContentsObserver:
void BeforeUnloadFired(const base::TimeTicks& proceed_time) override;
void RenderViewCreated(content::RenderViewHost*) override;
void RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) override;
void RenderViewDeleted(content::RenderViewHost*) override;
void RenderProcessGone(base::TerminationStatus status) override;
void DocumentLoadedInFrame(
Expand Down Expand Up @@ -530,6 +532,10 @@ class WebContents : public mate::TrackableObject<WebContents>,
// Observers of this WebContents.
base::ObserverList<ExtendedWebContentsObserver> observers_;

// The ID of the process of the currently committed RenderViewHost.
// -1 means no speculative RVH has been committed yet.
int currently_committed_process_id_ = -1;

DISALLOW_COPY_AND_ASSIGN(WebContents);
};

Expand Down
210 changes: 142 additions & 68 deletions atom/browser/atom_browser_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ AtomBrowserClient::~AtomBrowserClient() {
content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
int process_id) {
// If the process is a pending process, we should use the web contents
// for the frame host passed into OverrideSiteInstanceForNavigation.
// for the frame host passed into RegisterPendingProcess.
if (base::ContainsKey(pending_processes_, process_id))
return pending_processes_[process_id];

Expand All @@ -188,24 +188,36 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
return WebContentsPreferences::GetWebContentsFromProcessID(process_id);
}

bool AtomBrowserClient::ShouldCreateNewSiteInstance(
content::RenderFrameHost* render_frame_host,
bool AtomBrowserClient::ShouldForceNewSiteInstance(
content::RenderFrameHost* current_rfh,
content::RenderFrameHost* speculative_rfh,
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
const GURL& url) {
const GURL& url,
bool has_response_started) const {
if (url.SchemeIs(url::kJavaScriptScheme))
// "javacript:" scheme should always use same SiteInstance
return false;

content::SiteInstance* current_instance = current_rfh->GetSiteInstance();
content::SiteInstance* speculative_instance =
speculative_rfh ? speculative_rfh->GetSiteInstance() : nullptr;
int process_id = current_instance->GetProcess()->GetID();
if (!IsRendererSandboxed(process_id)) {
if (!RendererUsesNativeWindowOpen(process_id)) {
// non-sandboxed renderers without native window.open should always create
// a new SiteInstance
return true;
}
auto* web_contents =
content::WebContents::FromRenderFrameHost(render_frame_host);
if (NavigationWasRedirectedCrossSite(browser_context, current_instance,
speculative_instance, url,
has_response_started)) {
// Navigation was redirected. We can't force the current, speculative or a
// new unrelated site instance to be used. Delegate to the content layer.
return false;
} else if (IsRendererSandboxed(process_id)) {
// Renderer is sandboxed, delegate the decision to the content layer for all
// origins.
return false;
} else if (!RendererUsesNativeWindowOpen(process_id)) {
// non-sandboxed renderers without native window.open should always create
// a new SiteInstance
return true;
} else {
auto* web_contents = content::WebContents::FromRenderFrameHost(current_rfh);
if (!ChildWebContentsTracker::FromWebContents(web_contents)) {
// Root WebContents should always create new process to make sure
// native addons are loaded correctly after reload / navigation.
Expand All @@ -220,6 +232,26 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance(
return !IsSameWebSite(browser_context, src_url, url);
}

bool AtomBrowserClient::NavigationWasRedirectedCrossSite(
content::BrowserContext* browser_context,
content::SiteInstance* current_instance,
content::SiteInstance* speculative_instance,
const GURL& dest_url,
bool has_response_started) const {
bool navigation_was_redirected = false;
if (has_response_started) {
navigation_was_redirected = !IsSameWebSite(
browser_context, current_instance->GetSiteURL(), dest_url);
} else {
navigation_was_redirected =
speculative_instance &&
!IsSameWebSite(browser_context, speculative_instance->GetSiteURL(),
dest_url);
}

return navigation_was_redirected;
}

void AtomBrowserClient::AddProcessPreferences(
int process_id,
AtomBrowserClient::ProcessPreferences prefs) {
Expand All @@ -232,29 +264,69 @@ void AtomBrowserClient::RemoveProcessPreferences(int process_id) {
process_preferences_.erase(process_id);
}

bool AtomBrowserClient::IsProcessObserved(int process_id) {
bool AtomBrowserClient::IsProcessObserved(int process_id) const {
base::AutoLock auto_lock(process_preferences_lock_);
return process_preferences_.find(process_id) != process_preferences_.end();
}

bool AtomBrowserClient::IsRendererSandboxed(int process_id) {
bool AtomBrowserClient::IsRendererSandboxed(int process_id) const {
base::AutoLock auto_lock(process_preferences_lock_);
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.sandbox;
}

bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) {
bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) const {
base::AutoLock auto_lock(process_preferences_lock_);
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.native_window_open;
}

bool AtomBrowserClient::RendererDisablesPopups(int process_id) {
bool AtomBrowserClient::RendererDisablesPopups(int process_id) const {
base::AutoLock auto_lock(process_preferences_lock_);
auto it = process_preferences_.find(process_id);
return it != process_preferences_.end() && it->second.disable_popups;
}

std::string AtomBrowserClient::GetAffinityPreference(
content::RenderFrameHost* rfh) const {
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
auto* web_preferences = WebContentsPreferences::From(web_contents);
std::string affinity;
if (web_preferences &&
web_preferences->GetPreference("affinity", &affinity) &&
!affinity.empty()) {
affinity = base::ToLowerASCII(affinity);
}

return affinity;
}

content::SiteInstance* AtomBrowserClient::GetSiteInstanceFromAffinity(
content::BrowserContext* browser_context,
const GURL& url,
content::RenderFrameHost* rfh) const {
std::string affinity = GetAffinityPreference(rfh);
if (!affinity.empty()) {
auto iter = site_per_affinities_.find(affinity);
GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url);
if (iter != site_per_affinities_.end() &&
IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) {
return iter->second;
}
}

return nullptr;
}

void AtomBrowserClient::ConsiderSiteInstanceForAffinity(
content::RenderFrameHost* rfh,
content::SiteInstance* site_instance) {
std::string affinity = GetAffinityPreference(rfh);
if (!affinity.empty()) {
site_per_affinities_[affinity] = site_instance;
}
}

void AtomBrowserClient::RenderProcessWillLaunch(
content::RenderProcessHost* host,
service_manager::mojom::ServiceRequest* service_request) {
Expand Down Expand Up @@ -322,62 +394,59 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host,
web_preferences->OverrideWebkitPrefs(prefs);
}

void AtomBrowserClient::OverrideSiteInstanceForNavigation(
content::RenderFrameHost* rfh,
content::ContentBrowserClient::SiteInstanceForNavigationType
AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
content::RenderFrameHost* current_rfh,
content::RenderFrameHost* speculative_rfh,
content::BrowserContext* browser_context,
const GURL& url,
bool has_request_started,
content::SiteInstance* candidate_instance,
content::SiteInstance** new_instance) {
bool has_response_started,
content::SiteInstance** affinity_site_instance) const {
if (g_suppress_renderer_process_restart) {
g_suppress_renderer_process_restart = false;
return;
return SiteInstanceForNavigationType::ASK_CHROMIUM;
}

content::SiteInstance* current_instance = rfh->GetSiteInstance();
if (!ShouldCreateNewSiteInstance(rfh, browser_context, current_instance, url))
return;

// Do we have an affinity site to manage ?
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
auto* web_preferences = WebContentsPreferences::From(web_contents);
std::string affinity;
if (web_preferences &&
web_preferences->GetPreference("affinity", &affinity) &&
!affinity.empty()) {
affinity = base::ToLowerASCII(affinity);
auto iter = site_per_affinities.find(affinity);
GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url);
if (iter != site_per_affinities.end() &&
IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) {
*new_instance = iter->second;
} else {
site_per_affinities[affinity] = candidate_instance;
*new_instance = candidate_instance;
// Remember the original web contents for the pending renderer process.
auto* pending_process = candidate_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
}
} else {
// OverrideSiteInstanceForNavigation will be called more than once during a
// navigation (currently twice, on request and when it's about to commit in
// the renderer), look at RenderFrameHostManager::GetFrameHostForNavigation.
// In the default mode we should resuse the same site instance until the
// request commits otherwise it will get destroyed. Currently there is no
// unique lifetime tracker for a navigation request during site instance
// creation. We check for the state of the request, which should be one of
// (WAITING_FOR_RENDERER_RESPONSE, STARTED, RESPONSE_STARTED, FAILED) along
// with the availability of a speculative render frame host.
if (has_request_started) {
*new_instance = current_instance;
return;
}
content::SiteInstance* site_instance_from_affinity =
GetSiteInstanceFromAffinity(browser_context, url, current_rfh);
if (site_instance_from_affinity) {
*affinity_site_instance = site_instance_from_affinity;
return SiteInstanceForNavigationType::FORCE_AFFINITY;
}

if (!ShouldForceNewSiteInstance(current_rfh, speculative_rfh, browser_context,
url, has_response_started)) {
return SiteInstanceForNavigationType::ASK_CHROMIUM;
}

*new_instance = candidate_instance;
// Remember the original web contents for the pending renderer process.
auto* pending_process = candidate_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
// ShouldOverrideSiteInstanceForNavigation will be called more than once
// during a navigation (currently twice, on request and when it's about
// to commit in the renderer), look at
// RenderFrameHostManager::GetFrameHostForNavigation.
// In the default mode we should reuse the same site instance until the
// request commits otherwise it will get destroyed. Currently there is no
// unique lifetime tracker for a navigation request during site instance
// creation. We check for the state of the request, which should be one of
// (WAITING_FOR_RENDERER_RESPONSE, STARTED, RESPONSE_STARTED, FAILED) along
// with the availability of a speculative render frame host.
if (has_response_started) {
return SiteInstanceForNavigationType::FORCE_CURRENT;
}

return SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW;
}

void AtomBrowserClient::RegisterPendingSiteInstance(
content::RenderFrameHost* rfh,
content::SiteInstance* pending_site_instance) {
// Do we have an affinity site to manage?
ConsiderSiteInstanceForAffinity(rfh, pending_site_instance);

// Remember the original web contents for the pending renderer process.
auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
auto* pending_process = pending_site_instance->GetProcess();
pending_processes_[pending_process->GetID()] = web_contents;
}

void AtomBrowserClient::AppendExtraCommandLineSwitches(
Expand Down Expand Up @@ -555,10 +624,10 @@ void AtomBrowserClient::SiteInstanceDeleting(
content::SiteInstance* site_instance) {
// We are storing weak_ptr, is it fundamental to maintain the map up-to-date
// when an instance is destroyed.
for (auto iter = site_per_affinities.begin();
iter != site_per_affinities.end(); ++iter) {
for (auto iter = site_per_affinities_.begin();
iter != site_per_affinities_.end(); ++iter) {
if (iter->second == site_instance) {
site_per_affinities.erase(iter);
site_per_affinities_.erase(iter);
break;
}
}
Expand Down Expand Up @@ -786,7 +855,7 @@ void AtomBrowserClient::OnNetworkServiceCreated(
network_service);
}

bool AtomBrowserClient::ShouldBypassCORB(int render_process_id) {
bool AtomBrowserClient::ShouldBypassCORB(int render_process_id) const {
// This is called on the network thread.
base::AutoLock auto_lock(process_preferences_lock_);
auto it = process_preferences_.find(render_process_id);
Expand All @@ -799,4 +868,9 @@ std::string AtomBrowserClient::GetApplicationLocale() {
return *g_application_locale;
}

bool AtomBrowserClient::ShouldEnableStrictSiteIsolation() {
// Enable site isolation. It is off by default in Chromium <= 69.
return true;
}

} // namespace atom
Loading

0 comments on commit d5d1fa8

Please sign in to comment.