Skip to content

Commit

Permalink
fix: always use new site instance for a new navigation.
Browse files Browse the repository at this point in the history
  • Loading branch information
@ppontes
ppontes committed Jun 18, 2019
1 parent ccd15fc commit 07cad8d
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 22 deletions.
8 changes: 8 additions & 0 deletions atom/browser/atom_browser_client.cc
View file
Expand Up @@ -416,6 +416,7 @@ AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
content::RenderFrameHost* speculative_rfh,
content::BrowserContext* browser_context,
const GURL& url,
bool has_navigation_started,
bool has_response_started,
content::SiteInstance** affinity_site_instance) const {
if (g_suppress_renderer_process_restart) {
Expand Down Expand Up @@ -450,6 +451,13 @@ AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
return SiteInstanceForNavigationType::FORCE_CURRENT;
}

if (!has_navigation_started) {
// If the navigation didn't start yet, ignore any candidate site instance.
// If such instance exists, it belongs to a previous navigation still
// taking place. Fixes https://github.com/electron/electron/issues/17576.
return SiteInstanceForNavigationType::FORCE_NEW;
}

return SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW;
}

Expand Down
1 change: 1 addition & 0 deletions atom/browser/atom_browser_client.h
View file
Expand Up @@ -83,6 +83,7 @@ class AtomBrowserClient : public content::ContentBrowserClient,
content::RenderFrameHost* speculative_rfh,
content::BrowserContext* browser_context,
const GURL& url,
bool has_navigation_started,
bool has_request_started,
content::SiteInstance** affinity_site_instance) const override;
void RegisterPendingSiteInstance(
Expand Down
57 changes: 35 additions & 22 deletions patches/chromium/frame_host_manager.patch
View file
Expand Up @@ -14,7 +14,7 @@ index 12e1c5cff95aa6d0a907a249208e23371cf29785..3bc26b7870ff3bf6a69cb1e123fb372f
@@ -79,6 +79,13 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURL(
return instance;
}

+scoped_refptr<SiteInstanceImpl> BrowsingInstance::CreateSiteInstanceForURL(
+ const GURL& url) {
+ scoped_refptr<SiteInstanceImpl> instance = new SiteInstanceImpl(this);
Expand All @@ -32,7 +32,7 @@ index 775b64a8d20f89845812852a2904a1e6875c2b4a..5235b57bbf44fc7b30ca6943c43a290f
@@ -136,6 +136,11 @@ class CONTENT_EXPORT BrowsingInstance final
const GURL& url,
bool allow_default_instance);

+ // Create a new SiteInstance for the given URL bound the current
+ // BrowsingInstance.
+ scoped_refptr<SiteInstanceImpl> CreateSiteInstanceForURL(
Expand All @@ -42,7 +42,7 @@ index 775b64a8d20f89845812852a2904a1e6875c2b4a..5235b57bbf44fc7b30ca6943c43a290f
// another SiteInstance for the same site.
void RegisterSiteInstance(SiteInstanceImpl* site_instance);
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f25971b603ed2 100644
index 297b61198dd46114b3d8c89488a71ed01aa299c4..a49866aeb424a98b22b42ca427050631347a5c49 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -2127,6 +2127,20 @@ bool RenderFrameHostManager::InitRenderView(
Expand All @@ -66,32 +66,42 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f2597
// First, check if the navigation can switch SiteInstances. If not, the
// navigation should use the current SiteInstance.
SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
@@ -2158,6 +2172,53 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -2158,6 +2172,61 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
request.common_params().url);
no_renderer_swap_allowed |=
request.from_begin_navigation() && !can_renderer_initiate_transfer;
+
+ if (!GetContentClient()->browser()->CanUseCustomSiteInstance()) {
+ bool has_navigation_started = request.state() != NavigationRequest::NOT_STARTED;
+ bool has_response_started =
+ (request.state() == NavigationRequest::RESPONSE_STARTED ||
+ request.state() == NavigationRequest::FAILED) &&
+ !speculative_render_frame_host_;
+ // Gives user a chance to choose a custom site instance.
+ SiteInstance* affinity_site_instance = nullptr;
+ scoped_refptr<SiteInstance> overriden_site_instance;
+ bool should_register_site_instance = false;
+ ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType =
+ GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation(
+ current_frame_host(), speculative_frame_host(), browser_context,
+ request.common_params().url, has_response_started,
+ request.common_params().url, has_navigation_started,
+ has_response_started,
+ &affinity_site_instance);
+
+ switch (siteInstanceType) {
+ case ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW:
+ overriden_site_instance =
+ candidate_site_instance
+ ? candidate_site_instance
+ : current_site_instance->CreateRelatedSiteInstance(
+ request.common_params().url);
+ request.common_params().url);
+ should_register_site_instance = true;
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_NEW:
+ overriden_site_instance = current_site_instance->CreateRelatedSiteInstance(
+ request.common_params().url);
+ should_register_site_instance = true;
+ break;
+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT:
+ overriden_site_instance = render_frame_host_->GetSiteInstance();
Expand All @@ -108,9 +118,7 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f2597
+ break;
+ }
+ if (overriden_site_instance) {
+ if (siteInstanceType ==
+ ContentBrowserClient::SiteInstanceForNavigationType::
+ FORCE_CANDIDATE_OR_NEW) {
+ if (should_register_site_instance) {
+ GetContentClient()->browser()->RegisterPendingSiteInstance(
+ render_frame_host_.get(), overriden_site_instance.get());
+ }
Expand All @@ -120,10 +128,10 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f2597
} else {
// Subframe navigations will use the current renderer, unless specifically
// allowed to swap processes.
@@ -2169,23 +2230,28 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -2169,23 +2238,28 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
if (no_renderer_swap_allowed && !should_swap_for_error_isolation)
return scoped_refptr<SiteInstance>(current_site_instance);

+ if (GetContentClient()->browser()->CanUseCustomSiteInstance()) {
// If the navigation can swap SiteInstances, compute the SiteInstance it
// should use.
Expand All @@ -135,7 +143,7 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f2597
? speculative_render_frame_host_->GetSiteInstance()
: nullptr;
+ }

scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
request.common_params().url, request.source_site_instance(),
- request.dest_site_instance(), candidate_site_instance,
Expand All @@ -144,21 +152,21 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..40b848a8b448bed2d167bf5f6c0f2597
request.state() == NavigationRequest::FAILED,
request.restore_type() != RestoreType::NONE, request.is_view_source(),
was_server_redirect);

+ GetContentClient()->browser()->RegisterPendingSiteInstance(
+ render_frame_host_.get(), dest_site_instance.get());
+
return dest_site_instance;
}

diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index fd184108a7993094c29be3f7ebde658e259ede2c..75aa4a6b7d58a1bebe34efc923953c69348428a9 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -342,6 +342,10 @@ bool SiteInstanceImpl::HasRelatedSiteInstance(const GURL& url) {
return browsing_instance_->HasSiteInstance(url);
}

+scoped_refptr<SiteInstance> SiteInstanceImpl::CreateRelatedSiteInstance(const GURL& url) {
+ return browsing_instance_->CreateSiteInstanceForURL(url);
+}
Expand All @@ -179,13 +187,13 @@ index a46901055bdf17b6b0dab14edf753b234dc04a12..113660b6eeff81d56a0415b0fa16211e
size_t GetRelatedActiveContentsCount() override;
bool RequiresDedicatedProcess() override;
diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
index 2dd31166cc52ccb528b338b63fde7d2fb4bbf63d..ce64276225d5b0acf684e9e70c600a64a56fe96e 100644
index 2dd31166cc52ccb528b338b63fde7d2fb4bbf63d..34d88fc1a8d6dec0ae4506a3a97c2fa1a32bda01 100644
--- a/content/public/browser/content_browser_client.cc
+++ b/content/public/browser/content_browser_client.cc
@@ -52,6 +52,20 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
@@ -52,6 +52,21 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
handle);
}

+bool ContentBrowserClient::CanUseCustomSiteInstance() {
+ return false;
+}
Expand All @@ -195,6 +203,7 @@ index 2dd31166cc52ccb528b338b63fde7d2fb4bbf63d..ce64276225d5b0acf684e9e70c600a64
+ content::RenderFrameHost* speculative_rfh,
+ content::BrowserContext* browser_context,
+ const GURL& url,
+ bool has_navigation_started,
+ bool has_request_started,
+ content::SiteInstance** affinity_site_instance) const {
+ return SiteInstanceForNavigationType::ASK_CHROMIUM;
Expand All @@ -204,10 +213,10 @@ index 2dd31166cc52ccb528b338b63fde7d2fb4bbf63d..ce64276225d5b0acf684e9e70c600a64
const MainFunctionParams& parameters) {
return nullptr;
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 04bfc1a4a804d1f5aa28f894e2feb816bbe80ffc..5bf7340b106bd3ce826ff3322106ef94cbe19d29 100644
index 04bfc1a4a804d1f5aa28f894e2feb816bbe80ffc..caa35f17b78c5f3257ce14ae2d31de6b9872647f 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -211,8 +211,41 @@ CONTENT_EXPORT void OverrideOnBindInterface(
@@ -211,8 +211,45 @@ CONTENT_EXPORT void OverrideOnBindInterface(
// the observer interfaces.)
class CONTENT_EXPORT ContentBrowserClient {
public:
Expand All @@ -220,6 +229,9 @@ index 04bfc1a4a804d1f5aa28f894e2feb816bbe80ffc..5bf7340b106bd3ce826ff3322106ef94
+ // Use the current site instance for the navigation.
+ FORCE_CURRENT,
+
+ // Use a new, unrelated site instance.
+ FORCE_NEW,
+
+ // Use the provided affinity site instance for the navigation.
+ FORCE_AFFINITY,
+
Expand All @@ -228,7 +240,7 @@ index 04bfc1a4a804d1f5aa28f894e2feb816bbe80ffc..5bf7340b106bd3ce826ff3322106ef94
+ };
+
virtual ~ContentBrowserClient() {}

+ // Electron: Allows disabling the below ShouldOverride patch
+ virtual bool CanUseCustomSiteInstance();
+
Expand All @@ -238,6 +250,7 @@ index 04bfc1a4a804d1f5aa28f894e2feb816bbe80ffc..5bf7340b106bd3ce826ff3322106ef94
+ content::RenderFrameHost* speculative_rfh,
+ content::BrowserContext* browser_context,
+ const GURL& url,
+ bool has_navigation_started,
+ bool has_request_started,
+ content::SiteInstance** affinity_site_instance) const;
+
Expand All @@ -256,7 +269,7 @@ index a3e880e20e51d988175f0e8e2c42e7f5c1740104..faadd39d01530092f4f31a896ecb60f2
@@ -121,6 +121,11 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
// corresponds to a site URL with the host "example.com".
virtual const GURL& GetSiteURL() = 0;

+ // Create a SiteInstance for the given URL that shares the current
+ // BrowsingInstance.
+ virtual scoped_refptr<SiteInstance> CreateRelatedSiteInstance(
Expand Down

0 comments on commit 07cad8d

Please sign in to comment.