From cb2d31c7d236fc36e2220a68a85ae1993fe47c02 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Wed, 17 Oct 2018 11:20:27 +0200
Subject: [PATCH 01/19] 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.
---
 .../common/chromium/frame_host_manager.patch  | 105 ++++++++----------
 1 file changed, 47 insertions(+), 58 deletions(-)

diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch
index 128aaf67b7dd0..2969c211fe42c 100644
--- a/patches/common/chromium/frame_host_manager.patch
+++ b/patches/common/chromium/frame_host_manager.patch
@@ -1,4 +1,4 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From 4730f77f43c648f6168db58897b4569326a75627 Mon Sep 17 00:00:00 2001
 From: deepak1556 <hop2deep@gmail.com>
 Date: Wed, 14 Nov 2018 20:38:46 +0530
 Subject: frame_host_manager.patch
@@ -7,76 +7,65 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. 
 
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623b9b27b53 100644
+index 872e4609c..7c9e90b10 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
-@@ -1960,6 +1960,18 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
-   bool was_server_redirect = request.navigation_handle() &&
-                              request.navigation_handle()->WasServerRedirect();
- 
+@@ -1953,6 +1953,27 @@ bool RenderFrameHostManager::InitRenderView(
+ scoped_refptr<SiteInstance>
+ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+     const NavigationRequest& request) {
++  bool has_response_started =
++      (request.state() == NavigationRequest::RESPONSE_STARTED ||
++        request.state() == NavigationRequest::FAILED) &&
++      !speculative_render_frame_host_;
 +  BrowserContext* browser_context =
 +      delegate_->GetControllerForRenderManager().GetBrowserContext();
-+  // If the navigation can swap SiteInstances, compute the SiteInstance it
-+  // should use.
-+  // TODO(clamy): We should also consider as a candidate SiteInstance the
-+  // speculative SiteInstance that was computed on redirects.
 +  scoped_refptr<SiteInstance> candidate_site_instance =
-+      speculative_render_frame_host_
-+          ? speculative_render_frame_host_->GetSiteInstance()
-+          : content::SiteInstance::CreateForURL(browser_context,
-+                                                request.common_params().url);
++      GetCandidateSiteInstanceForNavigationRequest(request);
 +
-   if (frame_tree_node_->IsMainFrame()) {
-     // Renderer-initiated main frame navigations that may require a
-     // SiteInstance swap are sent to the browser via the OpenURL IPC and are
-@@ -1979,6 +1991,19 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
-                                                request.common_params().url));
-     no_renderer_swap_allowed |=
-         request.from_begin_navigation() && !can_renderer_initiate_transfer;
++  SiteInstance* client_custom_instance = nullptr;
++  GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
++      render_frame_host_.get(), browser_context, request.common_params().url,
++      has_response_started, candidate_site_instance.get(),
++      &client_custom_instance);
 +
-+    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* client_custom_instance = nullptr;
-+    GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
-+        render_frame_host_.get(), browser_context, request.common_params().url,
-+        has_response_started, candidate_site_instance.get(),
-+        &client_custom_instance);
-+    if (client_custom_instance)
-+      return scoped_refptr<SiteInstance>(client_custom_instance);
-   } else {
-     // Subframe navigations will use the current renderer, unless specifically
-     // allowed to swap processes.
-@@ -1990,18 +2015,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
-   if (no_renderer_swap_allowed)
-     return scoped_refptr<SiteInstance>(current_site_instance);
- 
--  // If the navigation can swap SiteInstances, compute the SiteInstance it
--  // should use.
--  // TODO(clamy): We should also consider as a candidate SiteInstance the
--  // speculative SiteInstance that was computed on redirects.
--  SiteInstance* candidate_site_instance =
--      speculative_render_frame_host_
--          ? 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,
-+      request.dest_site_instance(), candidate_site_instance.get(),
-       request.common_params().transition,
-       request.state() == NavigationRequest::FAILED,
-       request.restore_type() != RestoreType::NONE, request.is_view_source(),
++  return scoped_refptr<SiteInstance>(client_custom_instance ? client_custom_instance : candidate_site_instance);
++}
++
++scoped_refptr<SiteInstance>
++RenderFrameHostManager::GetCandidateSiteInstanceForNavigationRequest(
++    const NavigationRequest& request) {
+   // 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();
+diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
+index d8cf377f4..4391d1398 100644
+--- a/content/browser/frame_host/render_frame_host_manager.h
++++ b/content/browser/frame_host/render_frame_host_manager.h
+@@ -568,6 +568,15 @@ class CONTENT_EXPORT RenderFrameHostManager
+       bool new_is_view_source_mode,
+       bool is_failure) const;
+
++  // Returns the SiteInstance that should be used to host the navigation handled
++  // by |navigation_request|.
++  // Note: the SiteInstance returned by this function may not have an
++  // initialized RenderProcessHost. It will only be initialized when
++  // GetProcess() is called on the SiteInstance. In particular, calling this
++  // function will never lead to a process being created for the navigation.
++  scoped_refptr<SiteInstance> GetCandidateSiteInstanceForNavigationRequest(
++      const NavigationRequest& navigation_request);
++
+   // Returns the SiteInstance to use for the navigation.
+   scoped_refptr<SiteInstance> GetSiteInstanceForNavigation(
+       const GURL& dest_url,
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..2c22cb1cfe0dddc97c00e5f4ff89de6b18bc232f 100644
+index 3be31602689c..2c22cb1cfe0d 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
 @@ -196,6 +196,15 @@ class CONTENT_EXPORT ContentBrowserClient {
   public:
    virtual ~ContentBrowserClient() {}
- 
+
 +  // Electron: Allows overriding the SiteInstance when navigating.
 +  virtual void OverrideSiteInstanceForNavigation(
 +      RenderFrameHost* render_frame_host,

From 7530af6305fa16b2ff65455c3010214d06ca2b8a Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Thu, 18 Oct 2018 13:39:53 +0200
Subject: [PATCH 02/19] Revert "fix: use Chromium's determined new site
 instance as candidate when navigating."

This reverts commit eb95f935654a2c4d4457821297670836c10fdfd5.
---
 .../common/chromium/frame_host_manager.patch  | 101 ++++++++++--------
 1 file changed, 56 insertions(+), 45 deletions(-)

diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch
index 2969c211fe42c..772bb64805304 100644
--- a/patches/common/chromium/frame_host_manager.patch
+++ b/patches/common/chromium/frame_host_manager.patch
@@ -7,57 +7,68 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. 
 
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index 872e4609c..7c9e90b10 100644
+index 872e4609c94f..a59676004f24 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
-@@ -1953,6 +1953,27 @@ bool RenderFrameHostManager::InitRenderView(
- scoped_refptr<SiteInstance>
- RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
-     const NavigationRequest& request) {
-+  bool has_response_started =
-+      (request.state() == NavigationRequest::RESPONSE_STARTED ||
-+        request.state() == NavigationRequest::FAILED) &&
-+      !speculative_render_frame_host_;
+@@ -1960,6 +1960,18 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+   bool was_server_redirect = request.navigation_handle() &&
+                              request.navigation_handle()->WasServerRedirect();
+ 
 +  BrowserContext* browser_context =
 +      delegate_->GetControllerForRenderManager().GetBrowserContext();
++  // If the navigation can swap SiteInstances, compute the SiteInstance it
++  // should use.
++  // TODO(clamy): We should also consider as a candidate SiteInstance the
++  // speculative SiteInstance that was computed on redirects.
 +  scoped_refptr<SiteInstance> candidate_site_instance =
-+      GetCandidateSiteInstanceForNavigationRequest(request);
++      speculative_render_frame_host_
++          ? speculative_render_frame_host_->GetSiteInstance()
++          : content::SiteInstance::CreateForURL(browser_context,
++                                                request.common_params().url);
 +
-+  SiteInstance* client_custom_instance = nullptr;
-+  GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
-+      render_frame_host_.get(), browser_context, request.common_params().url,
-+      has_response_started, candidate_site_instance.get(),
-+      &client_custom_instance);
+   if (frame_tree_node_->IsMainFrame()) {
+     // Renderer-initiated main frame navigations that may require a
+     // SiteInstance swap are sent to the browser via the OpenURL IPC and are
+@@ -1979,6 +1991,19 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+                                                request.common_params().url));
+     no_renderer_swap_allowed |=
+         request.from_begin_navigation() && !can_renderer_initiate_transfer;
 +
-+  return scoped_refptr<SiteInstance>(client_custom_instance ? client_custom_instance : candidate_site_instance);
-+}
-+
-+scoped_refptr<SiteInstance>
-+RenderFrameHostManager::GetCandidateSiteInstanceForNavigationRequest(
-+    const NavigationRequest& request) {
-   // 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();
-diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
-index d8cf377f4..4391d1398 100644
---- a/content/browser/frame_host/render_frame_host_manager.h
-+++ b/content/browser/frame_host/render_frame_host_manager.h
-@@ -568,6 +568,15 @@ class CONTENT_EXPORT RenderFrameHostManager
-       bool new_is_view_source_mode,
-       bool is_failure) const;
-
-+  // Returns the SiteInstance that should be used to host the navigation handled
-+  // by |navigation_request|.
-+  // Note: the SiteInstance returned by this function may not have an
-+  // initialized RenderProcessHost. It will only be initialized when
-+  // GetProcess() is called on the SiteInstance. In particular, calling this
-+  // function will never lead to a process being created for the navigation.
-+  scoped_refptr<SiteInstance> GetCandidateSiteInstanceForNavigationRequest(
-+      const NavigationRequest& navigation_request);
-+
-   // Returns the SiteInstance to use for the navigation.
-   scoped_refptr<SiteInstance> GetSiteInstanceForNavigation(
-       const GURL& dest_url,
++    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* client_custom_instance = nullptr;
++    GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
++        render_frame_host_.get(), browser_context, request.common_params().url,
++        has_response_started, candidate_site_instance.get(),
++        &client_custom_instance);
++    if (client_custom_instance)
++      return scoped_refptr<SiteInstance>(client_custom_instance);
+   } else {
+     // Subframe navigations will use the current renderer, unless specifically
+     // allowed to swap processes.
+@@ -1990,18 +2015,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+   if (no_renderer_swap_allowed)
+     return scoped_refptr<SiteInstance>(current_site_instance);
+ 
+-  // If the navigation can swap SiteInstances, compute the SiteInstance it
+-  // should use.
+-  // TODO(clamy): We should also consider as a candidate SiteInstance the
+-  // speculative SiteInstance that was computed on redirects.
+-  SiteInstance* candidate_site_instance =
+-      speculative_render_frame_host_
+-          ? 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,
++      request.dest_site_instance(), candidate_site_instance.get(),
+       request.common_params().transition,
+       request.state() == NavigationRequest::FAILED,
+       request.restore_type() != RestoreType::NONE, request.is_view_source(),
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
 index 3be31602689c..2c22cb1cfe0d 100644
 --- a/content/public/browser/content_browser_client.h
@@ -65,7 +76,7 @@ index 3be31602689c..2c22cb1cfe0d 100644
 @@ -196,6 +196,15 @@ class CONTENT_EXPORT ContentBrowserClient {
   public:
    virtual ~ContentBrowserClient() {}
-
+ 
 +  // Electron: Allows overriding the SiteInstance when navigating.
 +  virtual void OverrideSiteInstanceForNavigation(
 +      RenderFrameHost* render_frame_host,

From 6e03731682124815667a1faaed0b352cfbaf9ed1 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Thu, 18 Oct 2018 13:40:05 +0200
Subject: [PATCH 03/19] fix: delegate site instance creation back to content
 when sandboxed.

---
 atom/browser/atom_browser_client.cc           |  6 +++++-
 .../common/chromium/frame_host_manager.patch  | 21 +++++++++++--------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index ac2d384206664..680ab637513b1 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -198,7 +198,11 @@ bool AtomBrowserClient::ShouldCreateNewSiteInstance(
     return false;
 
   int process_id = current_instance->GetProcess()->GetID();
-  if (!IsRendererSandboxed(process_id)) {
+  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
diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch
index 772bb64805304..01da58b74ea49 100644
--- a/patches/common/chromium/frame_host_manager.patch
+++ b/patches/common/chromium/frame_host_manager.patch
@@ -1,4 +1,4 @@
-From 4730f77f43c648f6168db58897b4569326a75627 Mon Sep 17 00:00:00 2001
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: deepak1556 <hop2deep@gmail.com>
 Date: Wed, 14 Nov 2018 20:38:46 +0530
 Subject: frame_host_manager.patch
@@ -7,10 +7,10 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. 
 
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index 872e4609c94f..a59676004f24 100644
+index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623b9b27b53 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
-@@ -1960,6 +1960,18 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+@@ -1960,6 +1960,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
    bool was_server_redirect = request.navigation_handle() &&
                               request.navigation_handle()->WasServerRedirect();
  
@@ -23,13 +23,12 @@ index 872e4609c94f..a59676004f24 100644
 +  scoped_refptr<SiteInstance> candidate_site_instance =
 +      speculative_render_frame_host_
 +          ? speculative_render_frame_host_->GetSiteInstance()
-+          : content::SiteInstance::CreateForURL(browser_context,
-+                                                request.common_params().url);
++          : nullptr;
 +
    if (frame_tree_node_->IsMainFrame()) {
      // Renderer-initiated main frame navigations that may require a
      // SiteInstance swap are sent to the browser via the OpenURL IPC and are
-@@ -1979,6 +1991,19 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+@@ -1979,6 +1990,23 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
                                                 request.common_params().url));
      no_renderer_swap_allowed |=
          request.from_begin_navigation() && !can_renderer_initiate_transfer;
@@ -39,17 +38,21 @@ index 872e4609c94f..a59676004f24 100644
 +         request.state() == NavigationRequest::FAILED) &&
 +        !speculative_render_frame_host_;
 +    // Gives user a chance to choose a custom site instance.
++    scoped_refptr<SiteInstance> override_candidate_instance = candidate_site_instance
++        ? candidate_site_instance
++        : content::SiteInstance::CreateForURL(browser_context,
++                                              request.common_params().url);
 +    SiteInstance* client_custom_instance = nullptr;
 +    GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
 +        render_frame_host_.get(), browser_context, request.common_params().url,
-+        has_response_started, candidate_site_instance.get(),
++        has_response_started, override_candidate_instance.get(),
 +        &client_custom_instance);
 +    if (client_custom_instance)
 +      return scoped_refptr<SiteInstance>(client_custom_instance);
    } else {
      // Subframe navigations will use the current renderer, unless specifically
      // allowed to swap processes.
-@@ -1990,18 +2015,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+@@ -1990,18 +2018,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
    if (no_renderer_swap_allowed)
      return scoped_refptr<SiteInstance>(current_site_instance);
  
@@ -70,7 +73,7 @@ index 872e4609c94f..a59676004f24 100644
        request.state() == NavigationRequest::FAILED,
        request.restore_type() != RestoreType::NONE, request.is_view_source(),
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 3be31602689c..2c22cb1cfe0d 100644
+index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..2c22cb1cfe0dddc97c00e5f4ff89de6b18bc232f 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
 @@ -196,6 +196,15 @@ class CONTENT_EXPORT ContentBrowserClient {

From 694e51b58e59d0f1b456c52e480b97283b8825a0 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Tue, 30 Oct 2018 07:21:00 +0100
Subject: [PATCH 04/19] fix: ensure site isolation is on

---
 atom/browser/atom_browser_client.cc | 5 +++++
 atom/browser/atom_browser_client.h  | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index 680ab637513b1..efa19a95ed6cc 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -803,4 +803,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
diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h
index e70ba5986fbf8..40684a59b6e72 100644
--- a/atom/browser/atom_browser_client.h
+++ b/atom/browser/atom_browser_client.h
@@ -65,6 +65,9 @@ class AtomBrowserClient : public content::ContentBrowserClient,
   // content::ContentBrowserClient:
   std::string GetApplicationLocale() override;
 
+  // content::ContentBrowserClient:
+  bool ShouldEnableStrictSiteIsolation() override;
+
  protected:
   void RenderProcessWillLaunch(
       content::RenderProcessHost* host,

From bd857e98471642f3d407f0c030484ed1914c2ae4 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Tue, 30 Oct 2018 07:26:27 +0100
Subject: [PATCH 05/19] test: adapt ut for cross-site navigation

---
 spec/api-browser-window-spec.js         | 61 +++++++++++--------------
 spec/fixtures/api/sandbox.html          |  5 +-
 spec/fixtures/module/preload-sandbox.js | 15 ++++--
 3 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
index 84d4abc85825c..bb50e121c3aac 100644
--- a/spec/api-browser-window-spec.js
+++ b/spec/api-browser-window-spec.js
@@ -90,6 +90,8 @@ describe('BrowserWindow module', () => {
           res.end()
         } else if (req.url === '/navigate-302') {
           res.end(`<html><body><script>window.location='${server.url}/302'</script></body></html>`)
+        } else if (req.url === '/cross-site') {
+          res.end(`<html><body><h1>${req.url}</h1></body></html>`)
         } else {
           res.end()
         }
@@ -1470,29 +1472,6 @@ describe('BrowserWindow module', () => {
 
       const preload = path.join(fixtures, 'module', 'preload-sandbox.js')
 
-      // http protocol to simulate accessing another domain. This is required
-      // because the code paths for cross domain popups is different.
-      function crossDomainHandler (request, callback) {
-        // Disabled due to false positive in StandardJS
-        // eslint-disable-next-line standard/no-callback-literal
-        callback({
-          mimeType: 'text/html',
-          data: `<html><body><h1>${request.url}</h1></body></html>`
-        })
-      }
-
-      before((done) => {
-        protocol.interceptStringProtocol('http', crossDomainHandler, () => {
-          done()
-        })
-      })
-
-      after((done) => {
-        protocol.uninterceptProtocol('http', () => {
-          done()
-        })
-      })
-
       it('exposes ipcRenderer to preload script', (done) => {
         ipcMain.once('answer', function (event, test) {
           assert.strictEqual(test, 'preload')
@@ -1587,10 +1566,18 @@ describe('BrowserWindow module', () => {
         })
 
         ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload)
-        const browserWindowCreated = emittedOnce(app, 'browser-window-created')
-        const childLoaded = emittedOnce(ipcMain, 'child-loaded')
-        w.loadFile(path.join(fixtures, 'api', 'sandbox.html'), { search: 'window-open-external' })
-        const expectedPopupUrl = 'http://www.google.com/#q=electron' // Set in the "sandbox.html".
+        w.loadFile(
+          path.join(fixtures, 'api', 'sandbox.html'),
+          { search: 'window-open-external' }
+        )
+
+        // Wait for a message from the main window saying that it's ready.
+        await emittedOnce(ipcMain, 'opener-loaded')
+
+        // Ask the opener to open a popup with window.opener.
+        const expectedPopupUrl =
+            `${server.url}/cross-site` // Set in "sandbox.html".
+        w.webContents.send('open-the-popup', expectedPopupUrl)
 
         // The page is going to open a popup that it won't be able to close.
         // We have to close it from here later.
@@ -1598,24 +1585,30 @@ describe('BrowserWindow module', () => {
         const [, popupWindow] = await browserWindowCreated
 
         // Wait for a message from the popup's preload script.
-        const [, openerIsNull, html, locationHref] = await childLoaded
-        expect(openerIsNull).to.be.true('window.opener is not null')
-        expect(html).to.equal(`<h1>${expectedPopupUrl}</h1>`,
-          'looks like a http: request has not been intercepted locally')
+        const [, openerIsNull, , locationHref] =
+            await emittedOnce(ipcMain, 'child-loaded')
+        expect(openerIsNull).to.be.false('window.opener is null')
         expect(locationHref).to.equal(expectedPopupUrl)
 
         // Ask the page to access the popup.
         const answer = emittedOnce(ipcMain, 'answer')
         w.webContents.send('touch-the-popup')
-        const [, exceptionMessage] = await answer
+        const [, popupAccessMessage] = await emittedOnce(ipcMain, 'answer')
+
+        // Ask the popup to access the opener.
+        w.webContents.send('touch-the-opener')
+        const [, openerAccessMessage] = await emittedOnce(ipcMain, 'answer')
 
         // We don't need the popup anymore, and its parent page can't close it,
         // so let's close it from here before we run any checks.
         await closeWindow(popupWindow, { assertSingleWindow: false })
 
-        expect(exceptionMessage).to.be.a('string',
+        expect(popupAccessMessage).to.be.a('string',
           `child's .document is accessible from its parent window`)
-        expect(exceptionMessage).to.match(/^Blocked a frame with origin/)
+        expect(popupAccessMessage).to.match(/^Blocked a frame with origin/)
+        expect(openerAccessMessage).to.be.a('string',
+          `opener .document is accessible from a popup window`)
+        expect(openerAccessMessage).to.match(/^Blocked a frame with origin/)
       })
 
       it('should inherit the sandbox setting in opened windows', (done) => {
diff --git a/spec/fixtures/api/sandbox.html b/spec/fixtures/api/sandbox.html
index 10263e0f57857..e997159d6ecb5 100644
--- a/spec/fixtures/api/sandbox.html
+++ b/spec/fixtures/api/sandbox.html
@@ -81,6 +81,9 @@
       },
       'window-open-external': () => {
         addEventListener('load', () => {
+          ipcRenderer.once('open-the-popup', (event, url) => {
+            popup = open(url, '', 'top=65,left=55,width=505,height=605')
+          })
           ipcRenderer.once('touch-the-popup', () => {
             let errorMessage = null
             try {
@@ -90,7 +93,7 @@
             }
             ipcRenderer.send('answer', errorMessage)
           })
-          popup = open('http://www.google.com/#q=electron', '', 'top=65,left=55,width=505,height=605')
+          ipcRenderer.send('opener-loaded')
         })
       },
       'verify-ipc-sender': () => {
diff --git a/spec/fixtures/module/preload-sandbox.js b/spec/fixtures/module/preload-sandbox.js
index 628f5c20270a7..97b3bcd63e8de 100644
--- a/spec/fixtures/module/preload-sandbox.js
+++ b/spec/fixtures/module/preload-sandbox.js
@@ -21,8 +21,17 @@
       }
     }
   } else if (location.href !== 'about:blank') {
-    addEventListener('DOMContentLoaded', () => {
-      ipcRenderer.send('child-loaded', window.opener == null, document.body.innerHTML, location.href)
-    }, false)
+    ipcRenderer.once('touch-the-opener', () => {
+      let errorMessage = null
+      try {
+        const openerDoc = opener.document // eslint-disable-line no-unused-vars
+      } catch (error) {
+        errorMessage = error.message
+      }
+      ipcRenderer.send('answer', errorMessage)
+    })
+    ipcRenderer.once('provide-details', () => {
+      ipcRenderer.send('answer', window.opener == null, document.body.innerHTML, location.href)
+    })
   }
 })()

From e799fe014195640084205a9557043ab338ca784e Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Wed, 7 Nov 2018 11:50:35 +0100
Subject: [PATCH 06/19] fix: register pending processes during a navigation.

---
 atom/browser/atom_browser_client.cc           | 129 +++++++++++-------
 atom/browser/atom_browser_client.h            |  33 +++--
 lib/browser/guest-window-manager.js           |   5 +-
 .../common/chromium/frame_host_manager.patch  | 116 +++++++++++++---
 spec/api-browser-window-spec.js               |   5 +-
 spec/fixtures/module/preload-sandbox.js       |  22 +--
 6 files changed, 218 insertions(+), 92 deletions(-)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index efa19a95ed6cc..7d1008dac1e79 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -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];
 
@@ -188,11 +188,11 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
   return WebContentsPreferences::GetWebContentsFromProcessID(process_id);
 }
 
-bool AtomBrowserClient::ShouldCreateNewSiteInstance(
+bool AtomBrowserClient::ShouldForceNewSiteInstance(
     content::RenderFrameHost* render_frame_host,
     content::BrowserContext* browser_context,
     content::SiteInstance* current_instance,
-    const GURL& url) {
+    const GURL& url) const {
   if (url.SchemeIs(url::kJavaScriptScheme))
     // "javacript:" scheme should always use same SiteInstance
     return false;
@@ -236,29 +236,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) {
@@ -326,62 +366,59 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host,
     web_preferences->OverrideWebkitPrefs(prefs);
 }
 
-void AtomBrowserClient::OverrideSiteInstanceForNavigation(
+content::ContentBrowserClient::SiteInstanceForNavigationType
+AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
     content::RenderFrameHost* rfh,
     content::BrowserContext* browser_context,
     const GURL& url,
     bool has_request_started,
-    content::SiteInstance* candidate_instance,
-    content::SiteInstance** new_instance) {
+    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;
+  if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance,
+                                  url)) {
+    return SiteInstanceForNavigationType::ASK_CHROMIUM;
+  }
 
   // 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;
-    }
+  content::SiteInstance* site_instance_from_affinity =
+      GetSiteInstanceFromAffinity(browser_context, url, rfh);
+  if (site_instance_from_affinity) {
+    *affinity_site_instance = site_instance_from_affinity;
+    return SiteInstanceForNavigationType::FORCE_AFFINITY;
   } 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
+    // 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_request_started) {
-      *new_instance = current_instance;
-      return;
+      return SiteInstanceForNavigationType::FORCE_CURRENT;
     }
-
-    *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;
   }
+
+  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(
@@ -559,10 +596,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;
     }
   }
diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h
index 40684a59b6e72..039bcf899a153 100644
--- a/atom/browser/atom_browser_client.h
+++ b/atom/browser/atom_browser_client.h
@@ -76,13 +76,15 @@ class AtomBrowserClient : public content::ContentBrowserClient,
   CreateSpeechRecognitionManagerDelegate() override;
   void OverrideWebkitPrefs(content::RenderViewHost* render_view_host,
                            content::WebPreferences* prefs) override;
-  void OverrideSiteInstanceForNavigation(
+  SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
       content::RenderFrameHost* render_frame_host,
       content::BrowserContext* browser_context,
       const GURL& dest_url,
       bool has_request_started,
-      content::SiteInstance* candidate_instance,
-      content::SiteInstance** new_instance) override;
+      content::SiteInstance** affinity_instance) const override;
+  void RegisterPendingSiteInstance(
+      content::RenderFrameHost* render_frame_host,
+      content::SiteInstance* pending_site_instance) override;
   void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
                                       int child_process_id) override;
   void DidCreatePpapiPlugin(content::BrowserPpapiHost* browser_host) override;
@@ -172,16 +174,23 @@ class AtomBrowserClient : public content::ContentBrowserClient,
     bool web_security = true;
   };
 
-  bool ShouldCreateNewSiteInstance(content::RenderFrameHost* render_frame_host,
-                                   content::BrowserContext* browser_context,
-                                   content::SiteInstance* current_instance,
-                                   const GURL& dest_url);
+  bool ShouldForceNewSiteInstance(content::RenderFrameHost* render_frame_host,
+                                  content::BrowserContext* browser_context,
+                                  content::SiteInstance* current_instance,
+                                  const GURL& dest_url) const;
   void AddProcessPreferences(int process_id, ProcessPreferences prefs);
   void RemoveProcessPreferences(int process_id);
-  bool IsProcessObserved(int process_id);
-  bool IsRendererSandboxed(int process_id);
-  bool RendererUsesNativeWindowOpen(int process_id);
-  bool RendererDisablesPopups(int process_id);
+  bool IsProcessObserved(int process_id) const;
+  bool IsRendererSandboxed(int process_id) const;
+  bool RendererUsesNativeWindowOpen(int process_id) const;
+  bool RendererDisablesPopups(int process_id) const;
+  std::string GetAffinityPreference(content::RenderFrameHost* rfh) const;
+  content::SiteInstance* GetSiteInstanceFromAffinity(
+      content::BrowserContext* browser_context,
+      const GURL& url,
+      content::RenderFrameHost* rfh) const;
+  void ConsiderSiteInstanceForAffinity(content::RenderFrameHost* rfh,
+                                       content::SiteInstance* site_instance);
 
   // pending_render_process => web contents.
   std::map<int, content::WebContents*> pending_processes_;
@@ -189,7 +198,7 @@ class AtomBrowserClient : public content::ContentBrowserClient,
   std::map<int, base::ProcessId> render_process_host_pids_;
 
   // list of site per affinity. weak_ptr to prevent instance locking
-  std::map<std::string, content::SiteInstance*> site_per_affinities;
+  std::map<std::string, content::SiteInstance*> site_per_affinities_;
 
   std::unique_ptr<AtomResourceDispatcherHostDelegate>
       resource_dispatcher_host_delegate_;
diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js
index 5bc5d26be6664..a892e0a3a1ac5 100644
--- a/lib/browser/guest-window-manager.js
+++ b/lib/browser/guest-window-manager.js
@@ -129,8 +129,9 @@ const createGuest = function (embedder, url, referrer, frameName, options, postD
     // the opener, even when the child window is from another origin.
     //
     // That's why the second condition(url !== "about:blank") is required: to
-    // force `OverrideSiteInstanceForNavigation` to be called and consequently
-    // spawn a new renderer if the new window is targeting a different origin.
+    // force `ShouldOverrideSiteInstanceForNavigation` to be called and
+    // consequently spawn a new renderer if the new window is targeting a
+    // different origin.
     //
     // If the URL is "about:blank", then it is very likely that the opener just
     // wants to synchronously script the popup, for example:
diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch
index 01da58b74ea49..f89cce80f750e 100644
--- a/patches/common/chromium/frame_host_manager.patch
+++ b/patches/common/chromium/frame_host_manager.patch
@@ -7,7 +7,7 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. 
 
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623b9b27b53 100644
+index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4afd834bb7 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
 @@ -1960,6 +1960,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -28,7 +28,7 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623
    if (frame_tree_node_->IsMainFrame()) {
      // Renderer-initiated main frame navigations that may require a
      // SiteInstance swap are sent to the browser via the OpenURL IPC and are
-@@ -1979,6 +1990,23 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+@@ -1979,6 +1990,51 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
                                                 request.common_params().url));
      no_renderer_swap_allowed |=
          request.from_begin_navigation() && !can_renderer_initiate_transfer;
@@ -38,21 +38,49 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623
 +         request.state() == NavigationRequest::FAILED) &&
 +        !speculative_render_frame_host_;
 +    // Gives user a chance to choose a custom site instance.
-+    scoped_refptr<SiteInstance> override_candidate_instance = candidate_site_instance
-+        ? candidate_site_instance
-+        : content::SiteInstance::CreateForURL(browser_context,
-+                                              request.common_params().url);
-+    SiteInstance* client_custom_instance = nullptr;
-+    GetContentClient()->browser()->OverrideSiteInstanceForNavigation(
-+        render_frame_host_.get(), browser_context, request.common_params().url,
-+        has_response_started, override_candidate_instance.get(),
-+        &client_custom_instance);
-+    if (client_custom_instance)
-+      return scoped_refptr<SiteInstance>(client_custom_instance);
++    SiteInstance* affinity_site_instance = nullptr;
++    scoped_refptr<SiteInstance> overriden_site_instance;
++    ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType =
++        GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation(
++            render_frame_host_.get(), browser_context,
++            request.common_params().url, has_response_started,
++            &affinity_site_instance);
++    switch (siteInstanceType) {
++      case ContentBrowserClient::SiteInstanceForNavigationType::
++          FORCE_CANDIDATE_OR_NEW:
++        overriden_site_instance =
++            candidate_site_instance
++                ? candidate_site_instance
++                : SiteInstance::CreateForURL(browser_context,
++                                             request.common_params().url);
++        break;
++      case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT:
++        overriden_site_instance = render_frame_host_->GetSiteInstance();
++        break;
++      case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_AFFINITY:
++        DCHECK(affinity_site_instance);
++        overriden_site_instance =
++            scoped_refptr<SiteInstance>(affinity_site_instance);
++        break;
++      case ContentBrowserClient::SiteInstanceForNavigationType::ASK_CHROMIUM:
++        DCHECK(!affinity_site_instance);
++        break;
++      default:
++        break;
++    }
++    if (overriden_site_instance) {
++      if (siteInstanceType ==
++          ContentBrowserClient::SiteInstanceForNavigationType::
++              FORCE_CANDIDATE_OR_NEW) {
++        GetContentClient()->browser()->RegisterPendingSiteInstance(
++            render_frame_host_.get(), overriden_site_instance.get());
++      }
++      return overriden_site_instance;
++    }
    } else {
      // Subframe navigations will use the current renderer, unless specifically
      // allowed to swap processes.
-@@ -1990,18 +2018,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+@@ -1990,23 +2046,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
    if (no_renderer_swap_allowed)
      return scoped_refptr<SiteInstance>(current_site_instance);
  
@@ -72,22 +100,72 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623
        request.common_params().transition,
        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/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
+index bb54b89bef5c6f32e7b4a056336c85494e2a04de..2d0633f38ddfc0fa1999674903b1d5e8952e22d5 100644
+--- a/content/public/browser/content_browser_client.cc
++++ b/content/public/browser/content_browser_client.cc
+@@ -47,6 +47,16 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
+                                                          handle);
+ }
+ 
++ContentBrowserClient::SiteInstanceForNavigationType
++ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
++    RenderFrameHost* render_frame_host,
++    BrowserContext* browser_context,
++    const GURL& dest_url,
++    bool has_response_started,
++    SiteInstance** affinity_instance) const {
++  return SiteInstanceForNavigationType::ASK_CHROMIUM;
++}
++
+ BrowserMainParts* ContentBrowserClient::CreateBrowserMainParts(
+     const MainFunctionParams& parameters) {
+   return nullptr;
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..2c22cb1cfe0dddc97c00e5f4ff89de6b18bc232f 100644
+index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..0554de92d81e54367e6f430bfe0c93e3a486ea66 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
-@@ -196,6 +196,15 @@ class CONTENT_EXPORT ContentBrowserClient {
+@@ -194,8 +194,36 @@ CONTENT_EXPORT void OverrideOnBindInterface(
+ // the observer interfaces.)
+ class CONTENT_EXPORT ContentBrowserClient {
   public:
++  // Identifies the type of site instance to use for a navigation.
++  enum SiteInstanceForNavigationType {
++    // Use either the candidate site instance or, if it doesn't exist
++    // a new, unrelated site instance for the navigation.
++    FORCE_CANDIDATE_OR_NEW = 0,
++
++    // Use the current site instance for the navigation.
++    FORCE_CURRENT,
++
++    // Use the provided affinity site instance for the navigation.
++    FORCE_AFFINITY,
++
++    // Delegate the site instance creation to Chromium.
++    ASK_CHROMIUM
++  };
    virtual ~ContentBrowserClient() {}
  
 +  // Electron: Allows overriding the SiteInstance when navigating.
-+  virtual void OverrideSiteInstanceForNavigation(
++  virtual SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
 +      RenderFrameHost* render_frame_host,
 +      BrowserContext* browser_context,
 +      const GURL& dest_url,
 +      bool has_response_started,
-+      SiteInstance* candidate_site_instance,
-+      SiteInstance** new_instance) {}
++      SiteInstance** affinity_instance) const;
++
++  // Electron: Registers a pending site instance during a navigation.
++  virtual void RegisterPendingSiteInstance(
++      content::RenderFrameHost* rfh,
++      content::SiteInstance* pending_site_instance){};
 +
    // Allows the embedder to set any number of custom BrowserMainParts
    // implementations for the browser startup code. See comments in
diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
index bb50e121c3aac..11e7dc5708d20 100644
--- a/spec/api-browser-window-spec.js
+++ b/spec/api-browser-window-spec.js
@@ -1584,7 +1584,8 @@ describe('BrowserWindow module', () => {
         // XXX(alexeykuzmin): It will leak if the test fails too soon.
         const [, popupWindow] = await browserWindowCreated
 
-        // Wait for a message from the popup's preload script.
+        // Ask the popup window for details.
+        popupWindow.webContents.send('provide-details')
         const [, openerIsNull, , locationHref] =
             await emittedOnce(ipcMain, 'child-loaded')
         expect(openerIsNull).to.be.false('window.opener is null')
@@ -1596,7 +1597,7 @@ describe('BrowserWindow module', () => {
         const [, popupAccessMessage] = await emittedOnce(ipcMain, 'answer')
 
         // Ask the popup to access the opener.
-        w.webContents.send('touch-the-opener')
+        popupWindow.webContents.send('touch-the-opener')
         const [, openerAccessMessage] = await emittedOnce(ipcMain, 'answer')
 
         // We don't need the popup anymore, and its parent page can't close it,
diff --git a/spec/fixtures/module/preload-sandbox.js b/spec/fixtures/module/preload-sandbox.js
index 97b3bcd63e8de..92cba0b3eb6b1 100644
--- a/spec/fixtures/module/preload-sandbox.js
+++ b/spec/fixtures/module/preload-sandbox.js
@@ -21,17 +21,17 @@
       }
     }
   } else if (location.href !== 'about:blank') {
-    ipcRenderer.once('touch-the-opener', () => {
-      let errorMessage = null
-      try {
-        const openerDoc = opener.document // eslint-disable-line no-unused-vars
-      } catch (error) {
-        errorMessage = error.message
-      }
-      ipcRenderer.send('answer', errorMessage)
-    })
-    ipcRenderer.once('provide-details', () => {
-      ipcRenderer.send('answer', window.opener == null, document.body.innerHTML, location.href)
+    addEventListener('DOMContentLoaded', () => {
+      ipcRenderer.on('touch-the-opener', () => {
+        let errorMessage = null
+        try {
+          const openerDoc = opener.document // eslint-disable-line no-unused-vars
+        } catch (error) {
+          errorMessage = error.message
+        }
+        ipcRenderer.send('answer', errorMessage)
+      })
+      ipcRenderer.send('child-loaded', window.opener == null, document.body.innerHTML, location.href)
     })
   }
 })()

From 9271a3968cb69bdf61c629c22d27c1dfa6b7fc0a Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Wed, 7 Nov 2018 14:21:11 +0100
Subject: [PATCH 07/19] refactor: dont call loadURL for a window constructed
 from an existing webContents.

---
 lib/browser/guest-window-manager.js | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js
index a892e0a3a1ac5..441d69ca888a3 100644
--- a/lib/browser/guest-window-manager.js
+++ b/lib/browser/guest-window-manager.js
@@ -118,29 +118,12 @@ const createGuest = function (embedder, url, referrer, frameName, options, postD
   }
 
   guest = new BrowserWindow(options)
-  if (!options.webContents || url !== 'about:blank') {
+  if (!options.webContents) {
     // We should not call `loadURL` if the window was constructed from an
-    // existing webContents(window.open in a sandboxed renderer) and if the url
-    // is not 'about:blank'.
+    // existing webContents (window.open in a sandboxed renderer).
     //
     // Navigating to the url when creating the window from an existing
-    // webContents would not be necessary(it will navigate there anyway), but
-    // apparently there's a bug that allows the child window to be scripted by
-    // the opener, even when the child window is from another origin.
-    //
-    // That's why the second condition(url !== "about:blank") is required: to
-    // force `ShouldOverrideSiteInstanceForNavigation` to be called and
-    // consequently spawn a new renderer if the new window is targeting a
-    // different origin.
-    //
-    // If the URL is "about:blank", then it is very likely that the opener just
-    // wants to synchronously script the popup, for example:
-    //
-    //     let popup = window.open()
-    //     popup.document.body.write('<h1>hello</h1>')
-    //
-    // The above code would not work if a navigation to "about:blank" is done
-    // here, since the window would be cleared of all changes in the next tick.
+    // webContents is not necessary (it will navigate there anyway).
     const loadOptions = {
       httpReferrer: referrer
     }

From 5aae6959dcf963df0963515ea3f33102c679ebc7 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 9 Nov 2018 18:08:11 +0100
Subject: [PATCH 08/19] test: add sandboxed affinity UT's.

---
 spec/api-browser-window-affinity-spec.js | 104 ++++++++++++-----------
 1 file changed, 55 insertions(+), 49 deletions(-)

diff --git a/spec/api-browser-window-affinity-spec.js b/spec/api-browser-window-affinity-spec.js
index 21d6d769bcfad..2108b5f0e5162 100644
--- a/spec/api-browser-window-affinity-spec.js
+++ b/spec/api-browser-window-affinity-spec.js
@@ -26,67 +26,73 @@ describe('BrowserWindow with affinity module', () => {
     })
   }
 
-  describe(`BrowserWindow with an affinity '${myAffinityName}'`, () => {
-    let mAffinityWindow
-    before(done => {
-      createWindowWithWebPrefs({ affinity: myAffinityName })
-        .then((w) => {
-          mAffinityWindow = w
+  function testAffinityProcessIds (name, webPreferences = {}) {
+    describe(name, () => {
+      let mAffinityWindow
+      before(done => {
+        createWindowWithWebPrefs({ affinity: myAffinityName, ...webPreferences })
+          .then((w) => {
+            mAffinityWindow = w
+            done()
+          })
+      })
+
+      after(done => {
+        closeWindow(mAffinityWindow, { assertSingleWindow: false }).then(() => {
+          mAffinityWindow = null
           done()
         })
-    })
-
-    after(done => {
-      closeWindow(mAffinityWindow, { assertSingleWindow: false }).then(() => {
-        mAffinityWindow = null
-        done()
       })
-    })
 
-    it('should have a different process id than a default window', done => {
-      createWindowWithWebPrefs({})
-        .then(w => {
-          const affinityID = mAffinityWindow.webContents.getOSProcessId()
-          const wcID = w.webContents.getOSProcessId()
+      it('should have a different process id than a default window', done => {
+        createWindowWithWebPrefs({ ...webPreferences })
+          .then(w => {
+            const affinityID = mAffinityWindow.webContents.getOSProcessId()
+            const wcID = w.webContents.getOSProcessId()
 
-          expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs')
-          closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
-        })
-    })
+            expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs')
+            closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
+          })
+      })
 
-    it(`should have a different process id than a window with a different affinity '${anotherAffinityName}'`, done => {
-      createWindowWithWebPrefs({ affinity: anotherAffinityName })
-        .then(w => {
-          const affinityID = mAffinityWindow.webContents.getOSProcessId()
-          const wcID = w.webContents.getOSProcessId()
+      it(`should have a different process id than a window with a different affinity '${anotherAffinityName}'`, done => {
+        createWindowWithWebPrefs({ affinity: anotherAffinityName, ...webPreferences })
+          .then(w => {
+            const affinityID = mAffinityWindow.webContents.getOSProcessId()
+            const wcID = w.webContents.getOSProcessId()
 
-          expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs')
-          closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
-        })
-    })
+            expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs')
+            closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
+          })
+      })
 
-    it(`should have the same OS process id than a window with the same affinity '${myAffinityName}'`, done => {
-      createWindowWithWebPrefs({ affinity: myAffinityName })
-        .then(w => {
-          const affinityID = mAffinityWindow.webContents.getOSProcessId()
-          const wcID = w.webContents.getOSProcessId()
+      it(`should have the same OS process id than a window with the same affinity '${myAffinityName}'`, done => {
+        createWindowWithWebPrefs({ affinity: myAffinityName, ...webPreferences })
+          .then(w => {
+            const affinityID = mAffinityWindow.webContents.getOSProcessId()
+            const wcID = w.webContents.getOSProcessId()
 
-          expect(affinityID).to.equal(wcID, 'Should have the same OS process ID')
-          closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
-        })
-    })
+            expect(affinityID).to.equal(wcID, 'Should have the same OS process ID')
+            closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
+          })
+      })
 
-    it(`should have the same OS process id than a window with an equivalent affinity '${myAffinityNameUpper}' (case insensitive)`, done => {
-      createWindowWithWebPrefs({ affinity: myAffinityNameUpper })
-        .then(w => {
-          const affinityID = mAffinityWindow.webContents.getOSProcessId()
-          const wcID = w.webContents.getOSProcessId()
+      it(`should have the same OS process id than a window with an equivalent affinity '${myAffinityNameUpper}' (case insensitive)`, done => {
+        createWindowWithWebPrefs({ affinity: myAffinityNameUpper, ...webPreferences })
+          .then(w => {
+            const affinityID = mAffinityWindow.webContents.getOSProcessId()
+            const wcID = w.webContents.getOSProcessId()
 
-          expect(affinityID).to.equal(wcID, 'Should have the same OS process ID')
-          closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
-        })
+            expect(affinityID).to.equal(wcID, 'Should have the same OS process ID')
+            closeWindow(w, { assertSingleWindow: false }).then(() => { done() })
+          })
+      })
     })
-  })
+  }
+
+  testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}'`)
+  testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}' and sandbox enabled`, { sandbox: true })
+  testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}' and nativeWindowOpen enabled`, { nativeWindowOpen: true })
 
   describe(`BrowserWindow with an affinity : nodeIntegration=false`, () => {
     const preload = path.join(fixtures, 'module', 'send-later.js')

From 2225c0b6ee550acaea6fc71081a3deeb7fb53dcc Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Sat, 10 Nov 2018 00:44:15 +0100
Subject: [PATCH 09/19] fix: check affinity before deciding if to force a new
 site instance.

---
 atom/browser/atom_browser_client.cc | 40 ++++++++++++++---------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index 7d1008dac1e79..3dbbf4eb0dad5 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -378,32 +378,32 @@ AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
     return SiteInstanceForNavigationType::ASK_CHROMIUM;
   }
 
-  content::SiteInstance* current_instance = rfh->GetSiteInstance();
-  if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance,
-                                  url)) {
-    return SiteInstanceForNavigationType::ASK_CHROMIUM;
-  }
-
   // Do we have an affinity site to manage ?
   content::SiteInstance* site_instance_from_affinity =
       GetSiteInstanceFromAffinity(browser_context, url, rfh);
   if (site_instance_from_affinity) {
     *affinity_site_instance = site_instance_from_affinity;
     return SiteInstanceForNavigationType::FORCE_AFFINITY;
-  } else {
-    // 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_request_started) {
-      return SiteInstanceForNavigationType::FORCE_CURRENT;
-    }
+  }
+
+  content::SiteInstance* current_instance = rfh->GetSiteInstance();
+  if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance,
+                                  url)) {
+    return SiteInstanceForNavigationType::ASK_CHROMIUM;
+  }
+
+  // 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_request_started) {
+    return SiteInstanceForNavigationType::FORCE_CURRENT;
   }
 
   return SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW;

From 6d023595a7cfea9abd8ec8991dacedf6aff36cf4 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 23 Nov 2018 12:51:41 +0100
Subject: [PATCH 10/19] chore: adapt subsequent patch.

---
 ...cross_site_document_resource_handler.patch | 34 ++++++++++++-------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/patches/common/chromium/cross_site_document_resource_handler.patch b/patches/common/chromium/cross_site_document_resource_handler.patch
index 565d7b4f4acfa..d119f47cef970 100644
--- a/patches/common/chromium/cross_site_document_resource_handler.patch
+++ b/patches/common/chromium/cross_site_document_resource_handler.patch
@@ -1,14 +1,21 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From b2048d8d28cc018d5b6b16794e6b2759a1bd6db4 Mon Sep 17 00:00:00 2001
 From: deepak1556 <hop2deep@gmail.com>
 Date: Thu, 15 Nov 2018 22:04:34 +0530
-Subject: cross_site_document_resource_handler.patch
+Subject: [PATCH] cross_site_document_resource_handler.patch
 
 Add a content layer hook to disable CORB for a renderer process,
 this patch can be removed once we switch to network service,
 where the embedders have a chance to design their URLLoaders.
 
+Patch-Filename: cross_site_document_resource_handler.patch
+---
+ .../browser/loader/cross_site_document_resource_handler.cc    | 3 +++
+ content/public/browser/content_browser_client.cc              | 4 ++++
+ content/public/browser/content_browser_client.h               | 3 +++
+ 3 files changed, 10 insertions(+)
+
 diff --git a/content/browser/loader/cross_site_document_resource_handler.cc b/content/browser/loader/cross_site_document_resource_handler.cc
-index 907922701280b589bf11691342de0ec95cdec6a1..eaf8bac18f8e3a2735ce7ded606199092a3746d3 100644
+index 907922701280..eaf8bac18f8e 100644
 --- a/content/browser/loader/cross_site_document_resource_handler.cc
 +++ b/content/browser/loader/cross_site_document_resource_handler.cc
 @@ -593,6 +593,9 @@ bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders(
@@ -22,14 +29,14 @@ index 907922701280b589bf11691342de0ec95cdec6a1..eaf8bac18f8e3a2735ce7ded60619909
  }
  
 diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
-index bb54b89bef5c6f32e7b4a056336c85494e2a04de..f069dfc4d2823b22eb0d90d28bc20236aeeddd04 100644
+index 2d0633f38ddf..f6d805520c2c 100644
 --- a/content/public/browser/content_browser_client.cc
 +++ b/content/public/browser/content_browser_client.cc
-@@ -47,6 +47,10 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
-                                                          handle);
+@@ -57,6 +57,10 @@ ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
+   return SiteInstanceForNavigationType::ASK_CHROMIUM;
  }
  
-+bool ContentBrowserClient::ShouldBypassCORB(int render_process_id) {
++bool ContentBrowserClient::ShouldBypassCORB(int render_process_id) const {
 +  return false;
 +}
 +
@@ -37,16 +44,19 @@ index bb54b89bef5c6f32e7b4a056336c85494e2a04de..f069dfc4d2823b22eb0d90d28bc20236
      const MainFunctionParams& parameters) {
    return nullptr;
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 2c22cb1cfe0dddc97c00e5f4ff89de6b18bc232f..a7b59095a887d566af9e74a646443fb49ea23bfc 100644
+index 0554de92d81e..64732e916ae5 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
-@@ -205,6 +205,9 @@ class CONTENT_EXPORT ContentBrowserClient {
-       SiteInstance* candidate_site_instance,
-       SiteInstance** new_instance) {}
+@@ -224,6 +224,9 @@ class CONTENT_EXPORT ContentBrowserClient {
+       content::RenderFrameHost* rfh,
+       content::SiteInstance* pending_site_instance){};
  
 +  // Electron: Allows bypassing CORB checks for a renderer process.
-+  virtual bool ShouldBypassCORB(int render_process_id);
++  virtual bool ShouldBypassCORB(int render_process_id) const;
 +
    // Allows the embedder to set any number of custom BrowserMainParts
    // implementations for the browser startup code. See comments in
    // browser_main_parts.h.
+-- 
+2.17.2 (Apple Git-113)
+

From f85ad5caa7101036787018f7d454bbd64aba5e98 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 23 Nov 2018 14:10:50 +0100
Subject: [PATCH 11/19] refactor: constify logically const methods.

---
 atom/browser/atom_browser_client.cc | 2 +-
 atom/browser/atom_browser_client.h  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index 3dbbf4eb0dad5..b5b60c4f49a5c 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -827,7 +827,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);
diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h
index 039bcf899a153..e722aff667264 100644
--- a/atom/browser/atom_browser_client.h
+++ b/atom/browser/atom_browser_client.h
@@ -149,7 +149,7 @@ class AtomBrowserClient : public content::ContentBrowserClient,
   GetSystemSharedURLLoaderFactory() override;
   void OnNetworkServiceCreated(
       network::mojom::NetworkService* network_service) override;
-  bool ShouldBypassCORB(int render_process_id) override;
+  bool ShouldBypassCORB(int render_process_id) const override;
 
   // content::RenderProcessHostObserver:
   void RenderProcessHostDestroyed(content::RenderProcessHost* host) override;
@@ -208,7 +208,7 @@ class AtomBrowserClient : public content::ContentBrowserClient,
 
   Delegate* delegate_ = nullptr;
 
-  base::Lock process_preferences_lock_;
+  mutable base::Lock process_preferences_lock_;
   std::map<int, ProcessPreferences> process_preferences_;
 
   DISALLOW_COPY_AND_ASSIGN(AtomBrowserClient);

From 220b722e17e7fc80375dd07cf4db9ce07cec1802 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 23 Nov 2018 18:39:24 +0100
Subject: [PATCH 12/19] fix: do not reuse site instances when navigation
 redirects cross-site.

---
 atom/browser/atom_browser_client.cc           | 64 +++++++++++++------
 atom/browser/atom_browser_client.h            | 20 ++++--
 ...cross_site_document_resource_handler.patch | 24 ++-----
 .../common/chromium/frame_host_manager.patch  | 35 +++++-----
 4 files changed, 85 insertions(+), 58 deletions(-)

diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index b5b60c4f49a5c..2c8c9d2d70b86 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -189,27 +189,35 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID(
 }
 
 bool AtomBrowserClient::ShouldForceNewSiteInstance(
-    content::RenderFrameHost* render_frame_host,
+    content::RenderFrameHost* current_rfh,
+    content::RenderFrameHost* speculative_rfh,
     content::BrowserContext* browser_context,
-    content::SiteInstance* current_instance,
-    const GURL& url) const {
+    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 (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 {
-    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);
+    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.
@@ -224,6 +232,26 @@ bool AtomBrowserClient::ShouldForceNewSiteInstance(
   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 navigationWasRedirected = false;
+  if (has_response_started) {
+    navigationWasRedirected = !IsSameWebSite(
+        browser_context, current_instance->GetSiteURL(), dest_url);
+  } else {
+    navigationWasRedirected =
+        speculative_instance &&
+        !IsSameWebSite(browser_context, speculative_instance->GetSiteURL(),
+                       dest_url);
+  }
+
+  return navigationWasRedirected;
+}
+
 void AtomBrowserClient::AddProcessPreferences(
     int process_id,
     AtomBrowserClient::ProcessPreferences prefs) {
@@ -368,10 +396,11 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host,
 
 content::ContentBrowserClient::SiteInstanceForNavigationType
 AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
-    content::RenderFrameHost* rfh,
+    content::RenderFrameHost* current_rfh,
+    content::RenderFrameHost* speculative_rfh,
     content::BrowserContext* browser_context,
     const GURL& url,
-    bool has_request_started,
+    bool has_response_started,
     content::SiteInstance** affinity_site_instance) const {
   if (g_suppress_renderer_process_restart) {
     g_suppress_renderer_process_restart = false;
@@ -380,15 +409,14 @@ AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
 
   // Do we have an affinity site to manage ?
   content::SiteInstance* site_instance_from_affinity =
-      GetSiteInstanceFromAffinity(browser_context, url, rfh);
+      GetSiteInstanceFromAffinity(browser_context, url, current_rfh);
   if (site_instance_from_affinity) {
     *affinity_site_instance = site_instance_from_affinity;
     return SiteInstanceForNavigationType::FORCE_AFFINITY;
   }
 
-  content::SiteInstance* current_instance = rfh->GetSiteInstance();
-  if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance,
-                                  url)) {
+  if (!ShouldForceNewSiteInstance(current_rfh, speculative_rfh, browser_context,
+                                  url, has_response_started)) {
     return SiteInstanceForNavigationType::ASK_CHROMIUM;
   }
 
@@ -402,7 +430,7 @@ AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation(
   // 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) {
+  if (has_response_started) {
     return SiteInstanceForNavigationType::FORCE_CURRENT;
   }
 
diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h
index e722aff667264..20f84a51f7936 100644
--- a/atom/browser/atom_browser_client.h
+++ b/atom/browser/atom_browser_client.h
@@ -77,11 +77,12 @@ class AtomBrowserClient : public content::ContentBrowserClient,
   void OverrideWebkitPrefs(content::RenderViewHost* render_view_host,
                            content::WebPreferences* prefs) override;
   SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
-      content::RenderFrameHost* render_frame_host,
+      content::RenderFrameHost* current_rfh,
+      content::RenderFrameHost* speculative_rfh,
       content::BrowserContext* browser_context,
-      const GURL& dest_url,
+      const GURL& url,
       bool has_request_started,
-      content::SiteInstance** affinity_instance) const override;
+      content::SiteInstance** affinity_site_instance) const override;
   void RegisterPendingSiteInstance(
       content::RenderFrameHost* render_frame_host,
       content::SiteInstance* pending_site_instance) override;
@@ -174,10 +175,17 @@ class AtomBrowserClient : public content::ContentBrowserClient,
     bool web_security = true;
   };
 
-  bool ShouldForceNewSiteInstance(content::RenderFrameHost* render_frame_host,
+  bool ShouldForceNewSiteInstance(content::RenderFrameHost* current_rfh,
+                                  content::RenderFrameHost* speculative_rfh,
                                   content::BrowserContext* browser_context,
-                                  content::SiteInstance* current_instance,
-                                  const GURL& dest_url) const;
+                                  const GURL& dest_url,
+                                  bool has_request_started) const;
+  bool NavigationWasRedirectedCrossSite(
+      content::BrowserContext* browser_context,
+      content::SiteInstance* current_instance,
+      content::SiteInstance* speculative_instance,
+      const GURL& dest_url,
+      bool has_request_started) const;
   void AddProcessPreferences(int process_id, ProcessPreferences prefs);
   void RemoveProcessPreferences(int process_id);
   bool IsProcessObserved(int process_id) const;
diff --git a/patches/common/chromium/cross_site_document_resource_handler.patch b/patches/common/chromium/cross_site_document_resource_handler.patch
index d119f47cef970..de284a575a06a 100644
--- a/patches/common/chromium/cross_site_document_resource_handler.patch
+++ b/patches/common/chromium/cross_site_document_resource_handler.patch
@@ -1,21 +1,14 @@
-From b2048d8d28cc018d5b6b16794e6b2759a1bd6db4 Mon Sep 17 00:00:00 2001
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: deepak1556 <hop2deep@gmail.com>
 Date: Thu, 15 Nov 2018 22:04:34 +0530
-Subject: [PATCH] cross_site_document_resource_handler.patch
+Subject: cross_site_document_resource_handler.patch
 
 Add a content layer hook to disable CORB for a renderer process,
 this patch can be removed once we switch to network service,
 where the embedders have a chance to design their URLLoaders.
 
-Patch-Filename: cross_site_document_resource_handler.patch
----
- .../browser/loader/cross_site_document_resource_handler.cc    | 3 +++
- content/public/browser/content_browser_client.cc              | 4 ++++
- content/public/browser/content_browser_client.h               | 3 +++
- 3 files changed, 10 insertions(+)
-
 diff --git a/content/browser/loader/cross_site_document_resource_handler.cc b/content/browser/loader/cross_site_document_resource_handler.cc
-index 907922701280..eaf8bac18f8e 100644
+index 907922701280b589bf11691342de0ec95cdec6a1..eaf8bac18f8e3a2735ce7ded606199092a3746d3 100644
 --- a/content/browser/loader/cross_site_document_resource_handler.cc
 +++ b/content/browser/loader/cross_site_document_resource_handler.cc
 @@ -593,6 +593,9 @@ bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders(
@@ -29,10 +22,10 @@ index 907922701280..eaf8bac18f8e 100644
  }
  
 diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
-index 2d0633f38ddf..f6d805520c2c 100644
+index f713d0cfbf90665d921f56f4d828887ad1f7842c..4fe21468aee93a7cb3783220ebfe8dd100a3d1d5 100644
 --- a/content/public/browser/content_browser_client.cc
 +++ b/content/public/browser/content_browser_client.cc
-@@ -57,6 +57,10 @@ ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
+@@ -57,6 +57,10 @@ ContentBrowserClient::SiteInstanceForNavigationType ContentBrowserClient::Should
    return SiteInstanceForNavigationType::ASK_CHROMIUM;
  }
  
@@ -44,10 +37,10 @@ index 2d0633f38ddf..f6d805520c2c 100644
      const MainFunctionParams& parameters) {
    return nullptr;
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 0554de92d81e..64732e916ae5 100644
+index 4bf6b2b5f8110f539adc61858cfdc8f77f7ed08b..94454812e27d4d357eeee27cfc1e185386ea2003 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
-@@ -224,6 +224,9 @@ class CONTENT_EXPORT ContentBrowserClient {
+@@ -225,6 +225,9 @@ class CONTENT_EXPORT ContentBrowserClient {
        content::RenderFrameHost* rfh,
        content::SiteInstance* pending_site_instance){};
  
@@ -57,6 +50,3 @@ index 0554de92d81e..64732e916ae5 100644
    // Allows the embedder to set any number of custom BrowserMainParts
    // implementations for the browser startup code. See comments in
    // browser_main_parts.h.
--- 
-2.17.2 (Apple Git-113)
-
diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch
index f89cce80f750e..d2ad29ae76281 100644
--- a/patches/common/chromium/frame_host_manager.patch
+++ b/patches/common/chromium/frame_host_manager.patch
@@ -7,7 +7,7 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. 
 
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4afd834bb7 100644
+index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..39d26adb60c50f88d19e824846519338083dc166 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
 @@ -1960,6 +1960,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
@@ -42,7 +42,7 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4a
 +    scoped_refptr<SiteInstance> overriden_site_instance;
 +    ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType =
 +        GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation(
-+            render_frame_host_.get(), browser_context,
++            current_frame_host(), speculative_frame_host(), browser_context,
 +            request.common_params().url, has_response_started,
 +            &affinity_site_instance);
 +    switch (siteInstanceType) {
@@ -109,20 +109,20 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4a
  }
  
 diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
-index bb54b89bef5c6f32e7b4a056336c85494e2a04de..2d0633f38ddfc0fa1999674903b1d5e8952e22d5 100644
+index bb54b89bef5c6f32e7b4a056336c85494e2a04de..f713d0cfbf90665d921f56f4d828887ad1f7842c 100644
 --- a/content/public/browser/content_browser_client.cc
 +++ b/content/public/browser/content_browser_client.cc
 @@ -47,6 +47,16 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info,
                                                           handle);
  }
  
-+ContentBrowserClient::SiteInstanceForNavigationType
-+ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
-+    RenderFrameHost* render_frame_host,
-+    BrowserContext* browser_context,
-+    const GURL& dest_url,
-+    bool has_response_started,
-+    SiteInstance** affinity_instance) const {
++ContentBrowserClient::SiteInstanceForNavigationType ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation(
++    content::RenderFrameHost* current_rfh,
++    content::RenderFrameHost* speculative_rfh,
++    content::BrowserContext* browser_context,
++    const GURL& url,
++    bool has_request_started,
++    content::SiteInstance** affinity_site_instance) const {
 +  return SiteInstanceForNavigationType::ASK_CHROMIUM;
 +}
 +
@@ -130,10 +130,10 @@ index bb54b89bef5c6f32e7b4a056336c85494e2a04de..2d0633f38ddfc0fa1999674903b1d5e8
      const MainFunctionParams& parameters) {
    return nullptr;
 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
-index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..0554de92d81e54367e6f430bfe0c93e3a486ea66 100644
+index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..4bf6b2b5f8110f539adc61858cfdc8f77f7ed08b 100644
 --- a/content/public/browser/content_browser_client.h
 +++ b/content/public/browser/content_browser_client.h
-@@ -194,8 +194,36 @@ CONTENT_EXPORT void OverrideOnBindInterface(
+@@ -194,8 +194,37 @@ CONTENT_EXPORT void OverrideOnBindInterface(
  // the observer interfaces.)
  class CONTENT_EXPORT ContentBrowserClient {
   public:
@@ -156,11 +156,12 @@ index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..0554de92d81e54367e6f430bfe0c93e3
  
 +  // Electron: Allows overriding the SiteInstance when navigating.
 +  virtual SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation(
-+      RenderFrameHost* render_frame_host,
-+      BrowserContext* browser_context,
-+      const GURL& dest_url,
-+      bool has_response_started,
-+      SiteInstance** affinity_instance) const;
++    content::RenderFrameHost* current_rfh,
++    content::RenderFrameHost* speculative_rfh,
++    content::BrowserContext* browser_context,
++    const GURL& url,
++    bool has_request_started,
++    content::SiteInstance** affinity_site_instance) const;
 +
 +  // Electron: Registers a pending site instance during a navigation.
 +  virtual void RegisterPendingSiteInstance(

From f4e3036a5b5d5da0108ea494d7066f560c4a4772 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Mon, 26 Nov 2018 14:38:59 +0100
Subject: [PATCH 13/19] test: ensure localStorage accessible after x-site
 redirect.

---
 spec/chromium-spec.js | 53 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js
index 439475506a3a8..e7eb28d363046 100644
--- a/spec/chromium-spec.js
+++ b/spec/chromium-spec.js
@@ -1066,6 +1066,59 @@ describe('chromium feature', () => {
         contents.loadURL(`${protocolName}://host/cookie`)
       })
     })
+
+    describe('can be accessed', () => {
+      let server = null
+      before((done) => {
+        server = http.createServer((req, res) => {
+          const respond = () => {
+            if (req.url === '/redirect-cross-site') {
+              res.setHeader('Location', `${server.cross_site_url}/redirected`)
+              res.statusCode = 302
+              res.end()
+            } else if (req.url === '/redirected') {
+              res.end('<html><script>window.localStorage</script></html>')
+            } else {
+              res.end()
+            }
+          }
+          setTimeout(respond, 0)
+        })
+        server.listen(0, '127.0.0.1', () => {
+          server.url = `http://127.0.0.1:${server.address().port}`
+          server.cross_site_url = `http://localhost:${server.address().port}`
+          done()
+        })
+      })
+
+      after(() => {
+        server.close()
+        server = null
+      })
+
+      const testLocalStorageAfterXSiteRedirect = (testTitle, extraPreferences = {}) => {
+        it(testTitle, (done) => {
+          const webPreferences = { show: false, ...extraPreferences }
+          w = new BrowserWindow(webPreferences)
+          let redirected = false
+          w.webContents.on('crashed', () => {
+            assert.fail('renderer crashed / was killed')
+          })
+          w.webContents.on('did-redirect-navigation', (event, url) => {
+            assert.strictEqual(url, `${server.cross_site_url}/redirected`)
+            redirected = true
+          })
+          w.webContents.on('did-finish-load', () => {
+            assert.strictEqual(redirected, true, 'didnt redirect')
+            done()
+          })
+          w.loadURL(`${server.url}/redirect-cross-site`)
+        })
+      }
+
+      testLocalStorageAfterXSiteRedirect('after a cross-site redirect')
+      testLocalStorageAfterXSiteRedirect('after a cross-site redirect in sandbox mode', { sandbox: true })
+    })
   })
 
   describe('websockets', () => {

From c30f0033f5fdd63306a0a3831dec632f293ad54d Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Mon, 26 Nov 2018 15:27:20 +0100
Subject: [PATCH 14/19] test: adapt localStorage acess denied UT for site
 isolation.

---
 spec/chromium-spec.js                         | 20 +++++++++++--------
 .../fixtures/pages/storage/local_storage.html |  7 +++++--
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js
index e7eb28d363046..b2679624be170 100644
--- a/spec/chromium-spec.js
+++ b/spec/chromium-spec.js
@@ -1016,16 +1016,20 @@ describe('chromium feature', () => {
         contents = null
       })
 
-      // FIXME(deepak1556): Disabled with site isolation ON
-      // Localstorage area is accessed on the browser process
-      // before checking accessibility on the renderer side,
-      // causing illegal origin access renderer termination.
-      xit('cannot access localStorage', (done) => {
-        ipcMain.once('local-storage-response', (event, error) => {
+      it('cannot access localStorage', (done) => {
+        contents.on('crashed', (event, killed) => {
+          // Site isolation ON: process is killed for trying to access resources without permission.
+          if (process.platform !== 'win32') {
+            // Chromium on Windows does not set this flag correctly.
+            assert.strictEqual(killed, true, 'Process should\'ve been killed')
+          }
+          done()
+        })
+        ipcMain.once('local-storage-response', (event, message) => {
+          // Site isolation OFF: access is refused.
           assert.strictEqual(
-            error,
+            message,
             'Failed to read the \'localStorage\' property from \'Window\': Access is denied for this document.')
-          done()
         })
         contents.loadURL(protocolName + '://host/localStorage')
       })
diff --git a/spec/fixtures/pages/storage/local_storage.html b/spec/fixtures/pages/storage/local_storage.html
index fc9bab0090849..523ef20851830 100644
--- a/spec/fixtures/pages/storage/local_storage.html
+++ b/spec/fixtures/pages/storage/local_storage.html
@@ -1,8 +1,11 @@
 <script>
+  const {ipcRenderer} = require('electron')
+
+  let message = 'Ok'
   try {
     window.localStorage
   } catch (e) {
-    const {ipcRenderer} = require('electron')
-    ipcRenderer.send('local-storage-response', e.message)
+    message = e.message
   }
+  ipcRenderer.send('local-storage-response', message)
 </script>

From d8d16a5988fade50cad01518cee1653ac579fc68 Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Thu, 29 Nov 2018 18:09:21 +0100
Subject: [PATCH 15/19] fix: do not send render-view-deleted for speculative
 frames.

---
 atom/browser/api/atom_api_web_contents.cc | 15 ++++++++++++++-
 atom/browser/api/atom_api_web_contents.h  |  6 ++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
index 3f928a566659f..8ff08e260c7c5 100644
--- a/atom/browser/api/atom_api_web_contents.cc
+++ b/atom/browser/api/atom_api_web_contents.cc
@@ -781,8 +781,21 @@ 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) {
-  Emit("render-view-deleted", render_view_host->GetProcess()->GetID());
+  // Only emit render-view-deleted if the RVH that has been deleted is the
+  // current RVH. Do not emit it for other (speculative) RVH's as that does not
+  // mean the embedder is closing the window.
+  if (-1 == currently_committed_process_id ||
+      render_view_host->GetProcess()->GetID() ==
+          currently_committed_process_id) {
+    currently_committed_process_id = -1;
+    Emit("render-view-deleted", render_view_host->GetProcess()->GetID());
+  }
 }
 
 void WebContents::RenderProcessGone(base::TerminationStatus status) {
diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h
index 607ff3bfb2846..db507ce3aee15 100644
--- a/atom/browser/api/atom_api_web_contents.h
+++ b/atom/browser/api/atom_api_web_contents.h
@@ -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(
@@ -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);
 };
 

From 9829b19db22f08579de9d6f4ff6d77ffa0409c9a Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Thu, 29 Nov 2018 21:14:33 +0100
Subject: [PATCH 16/19] chore: amend tests after rebase.

---
 spec/api-browser-window-spec.js | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
index 11e7dc5708d20..5949a336307c5 100644
--- a/spec/api-browser-window-spec.js
+++ b/spec/api-browser-window-spec.js
@@ -1566,17 +1566,19 @@ describe('BrowserWindow module', () => {
         })
 
         ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload)
+        const openerWindowOpen = emittedOnce(ipcMain, 'opener-loaded')
         w.loadFile(
           path.join(fixtures, 'api', 'sandbox.html'),
           { search: 'window-open-external' }
         )
 
         // Wait for a message from the main window saying that it's ready.
-        await emittedOnce(ipcMain, 'opener-loaded')
+        await openerWindowOpen
 
         // Ask the opener to open a popup with window.opener.
-        const expectedPopupUrl =
-            `${server.url}/cross-site` // Set in "sandbox.html".
+        const expectedPopupUrl = `${server.url}/cross-site` // Set in "sandbox.html".
+
+        const browserWindowCreated = emittedOnce(app, 'browser-window-created')
         w.webContents.send('open-the-popup', expectedPopupUrl)
 
         // The page is going to open a popup that it won't be able to close.
@@ -1585,20 +1587,21 @@ describe('BrowserWindow module', () => {
         const [, popupWindow] = await browserWindowCreated
 
         // Ask the popup window for details.
+        const detailsAnswer = emittedOnce(ipcMain, 'child-loaded')
         popupWindow.webContents.send('provide-details')
-        const [, openerIsNull, , locationHref] =
-            await emittedOnce(ipcMain, 'child-loaded')
+        const [, openerIsNull, , locationHref] = await detailsAnswer
         expect(openerIsNull).to.be.false('window.opener is null')
         expect(locationHref).to.equal(expectedPopupUrl)
 
         // Ask the page to access the popup.
-        const answer = emittedOnce(ipcMain, 'answer')
+        const touchPopupResult = emittedOnce(ipcMain, 'answer')
         w.webContents.send('touch-the-popup')
-        const [, popupAccessMessage] = await emittedOnce(ipcMain, 'answer')
+        const [, popupAccessMessage] = await touchPopupResult
 
         // Ask the popup to access the opener.
+        const touchOpenerResult = emittedOnce(ipcMain, 'answer')
         popupWindow.webContents.send('touch-the-opener')
-        const [, openerAccessMessage] = await emittedOnce(ipcMain, 'answer')
+        const [, openerAccessMessage] = await touchOpenerResult
 
         // We don't need the popup anymore, and its parent page can't close it,
         // so let's close it from here before we run any checks.

From c56311f3c17a9ef5e86cd5cb1e2b1c8d5a90393a Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 30 Nov 2018 11:29:42 +0100
Subject: [PATCH 17/19] test: add ut for webContents' render-view-deleted
 emission

---
 spec/api-web-contents-spec.js | 63 +++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js
index a3583bf67bcc3..634fbb735d669 100644
--- a/spec/api-web-contents-spec.js
+++ b/spec/api-web-contents-spec.js
@@ -712,6 +712,69 @@ describe('webContents module', () => {
     })
   })
 
+  describe('render-view-deleted', () => {
+    let server = null
+
+    before((done) => {
+      server = http.createServer((req, res) => {
+        const respond = () => {
+          if (req.url === '/redirect-cross-site') {
+            res.setHeader('Location', `${server.cross_site_url}/redirected`)
+            res.statusCode = 302
+            res.end()
+          } else if (req.url === '/redirected') {
+            res.end('<html><script>window.localStorage</script></html>')
+          } else {
+            res.end()
+          }
+        }
+        setTimeout(respond, 0)
+      })
+      server.listen(0, '127.0.0.1', () => {
+        server.url = `http://127.0.0.1:${server.address().port}`
+        server.cross_site_url = `http://localhost:${server.address().port}`
+        done()
+      })
+    })
+
+    after(() => {
+      server.close()
+      server = null
+    })
+
+    it('does not emit when speculative RVHs are deleted', (done) => {
+      let renderViewDeletedEmitted = false
+      w.webContents.once('destroyed', () => {
+        assert.strictEqual(renderViewDeletedEmitted, false, 'render-view-deleted was emitted')
+        done()
+      })
+      const renderViewDeletedHandler = () => {
+        renderViewDeletedEmitted = true
+      }
+      w.webContents.on('render-view-deleted', renderViewDeletedHandler)
+      w.webContents.on('did-finish-load', (e) => {
+        w.webContents.removeListener('render-view-deleted', renderViewDeletedHandler)
+        w.close()
+      })
+      w.loadURL(`${server.url}/redirect-cross-site`)
+    })
+
+    it('emits if the current RVHs are deleted', (done) => {
+      let renderViewDeletedEmitted = false
+      w.webContents.once('destroyed', () => {
+        assert.strictEqual(renderViewDeletedEmitted, true, 'render-view-deleted wasn\'t emitted')
+        done()
+      })
+      w.webContents.on('render-view-deleted', () => {
+        renderViewDeletedEmitted = true
+      })
+      w.webContents.on('did-finish-load', (e) => {
+        w.close()
+      })
+      w.loadURL(`${server.url}/redirect-cross-site`)
+    })
+  })
+
   describe('setIgnoreMenuShortcuts(ignore)', () => {
     it('does not throw', () => {
       assert.strictEqual(w.webContents.setIgnoreMenuShortcuts(true), undefined)

From 14daf67b1cb4290beaffd2920aac68a081addd3b Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Fri, 30 Nov 2018 17:37:46 +0100
Subject: [PATCH 18/19] 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.
---
 atom/browser/api/atom_api_web_contents.cc | 10 +++---
 lib/browser/guest-window-manager.js       |  4 +--
 spec/api-web-contents-spec.js             | 38 ++++++++++++++++-------
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
index 8ff08e260c7c5..73d612e760df4 100644
--- a/atom/browser/api/atom_api_web_contents.cc
+++ b/atom/browser/api/atom_api_web_contents.cc
@@ -787,14 +787,16 @@ void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host,
 }
 
 void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
-  // Only emit render-view-deleted if the RVH that has been deleted is the
-  // current RVH. Do not emit it for other (speculative) RVH's as that does not
-  // mean the embedder is closing the window.
+  Emit("render-view-deleted", render_view_host->GetProcess()->GetID());
+
+  // When the RVH that has been deleted is the current RVH it means that the
+  // the embedder is closing the window.
   if (-1 == currently_committed_process_id ||
       render_view_host->GetProcess()->GetID() ==
           currently_committed_process_id) {
     currently_committed_process_id = -1;
-    Emit("render-view-deleted", render_view_host->GetProcess()->GetID());
+    Emit("current-render-view-deleted",
+         render_view_host->GetProcess()->GetID());
   }
 }
 
diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js
index 441d69ca888a3..a878a571d7efe 100644
--- a/lib/browser/guest-window-manager.js
+++ b/lib/browser/guest-window-manager.js
@@ -90,9 +90,9 @@ const setupGuest = function (embedder, frameName, guest, options) {
   }
   const closedByUser = function () {
     embedder._sendInternal('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSED_' + guestId)
-    embedder.removeListener('render-view-deleted', closedByEmbedder)
+    embedder.removeListener('current-render-view-deleted', closedByEmbedder)
   }
-  embedder.once('render-view-deleted', closedByEmbedder)
+  embedder.once('current-render-view-deleted', closedByEmbedder)
   guest.once('closed', closedByUser)
   if (frameName) {
     frameToGuest.set(frameName, guest)
diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js
index 634fbb735d669..83f5002e93ed6 100644
--- a/spec/api-web-contents-spec.js
+++ b/spec/api-web-contents-spec.js
@@ -712,7 +712,7 @@ describe('webContents module', () => {
     })
   })
 
-  describe('render-view-deleted', () => {
+  describe('render view deleted events', () => {
     let server = null
 
     before((done) => {
@@ -742,31 +742,47 @@ describe('webContents module', () => {
       server = null
     })
 
-    it('does not emit when speculative RVHs are deleted', (done) => {
-      let renderViewDeletedEmitted = false
+    it('does not emit current-render-view-deleted when speculative RVHs are deleted', (done) => {
+      let currentRenderViewDeletedEmitted = false
       w.webContents.once('destroyed', () => {
-        assert.strictEqual(renderViewDeletedEmitted, false, 'render-view-deleted was emitted')
+        assert.strictEqual(currentRenderViewDeletedEmitted, false, 'current-render-view-deleted was emitted')
         done()
       })
       const renderViewDeletedHandler = () => {
-        renderViewDeletedEmitted = true
+        currentRenderViewDeletedEmitted = true
       }
-      w.webContents.on('render-view-deleted', renderViewDeletedHandler)
+      w.webContents.on('current-render-view-deleted', renderViewDeletedHandler)
       w.webContents.on('did-finish-load', (e) => {
-        w.webContents.removeListener('render-view-deleted', renderViewDeletedHandler)
+        w.webContents.removeListener('current-render-view-deleted', renderViewDeletedHandler)
         w.close()
       })
       w.loadURL(`${server.url}/redirect-cross-site`)
     })
 
-    it('emits if the current RVHs are deleted', (done) => {
-      let renderViewDeletedEmitted = false
+    it('emits current-render-view-deleted if the current RVHs are deleted', (done) => {
+      let currentRenderViewDeletedEmitted = false
       w.webContents.once('destroyed', () => {
-        assert.strictEqual(renderViewDeletedEmitted, true, 'render-view-deleted wasn\'t emitted')
+        assert.strictEqual(currentRenderViewDeletedEmitted, true, 'current-render-view-deleted wasn\'t emitted')
+        done()
+      })
+      w.webContents.on('current-render-view-deleted', () => {
+        currentRenderViewDeletedEmitted = true
+      })
+      w.webContents.on('did-finish-load', (e) => {
+        w.close()
+      })
+      w.loadURL(`${server.url}/redirect-cross-site`)
+    })
+
+    it('emits render-view-deleted if any RVHs are deleted', (done) => {
+      let rvhDeletedCount = 0
+      w.webContents.once('destroyed', () => {
+        const expectedRenderViewDeletedEventCount = 3 // 1 speculative upon redirection + 2 upon window close.
+        assert.strictEqual(rvhDeletedCount, expectedRenderViewDeletedEventCount, 'render-view-deleted wasn\'t emitted the expected nr. of times')
         done()
       })
       w.webContents.on('render-view-deleted', () => {
-        renderViewDeletedEmitted = true
+        rvhDeletedCount++
       })
       w.webContents.on('did-finish-load', (e) => {
         w.close()

From 8b4e030d1ba2afa2ae153d3a161468119dd172aa Mon Sep 17 00:00:00 2001
From: Pedro Pontes <pepontes@microsoft.com>
Date: Sat, 1 Dec 2018 08:49:00 +0100
Subject: [PATCH 19/19] refactor: style and comments fixed.

---
 atom/browser/api/atom_api_web_contents.cc | 18 ++++++++++++------
 atom/browser/api/atom_api_web_contents.h  |  2 +-
 atom/browser/atom_browser_client.cc       |  8 ++++----
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
index 73d612e760df4..1ea6d243db4de 100644
--- a/atom/browser/api/atom_api_web_contents.cc
+++ b/atom/browser/api/atom_api_web_contents.cc
@@ -783,18 +783,24 @@ void WebContents::RenderViewCreated(content::RenderViewHost* render_view_host) {
 
 void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host,
                                         content::RenderViewHost* new_host) {
-  currently_committed_process_id = new_host->GetProcess()->GetID();
+  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());
 
-  // When the RVH that has been deleted is the current RVH it means that the
-  // the embedder is closing the window.
-  if (-1 == currently_committed_process_id ||
+  if (-1 == currently_committed_process_id_ ||
       render_view_host->GetProcess()->GetID() ==
-          currently_committed_process_id) {
-    currently_committed_process_id = -1;
+          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());
   }
diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h
index db507ce3aee15..4f37e621bd4e5 100644
--- a/atom/browser/api/atom_api_web_contents.h
+++ b/atom/browser/api/atom_api_web_contents.h
@@ -534,7 +534,7 @@ class WebContents : public mate::TrackableObject<WebContents>,
 
   // 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;
+  int currently_committed_process_id_ = -1;
 
   DISALLOW_COPY_AND_ASSIGN(WebContents);
 };
diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
index 2c8c9d2d70b86..900b37d8bb22a 100644
--- a/atom/browser/atom_browser_client.cc
+++ b/atom/browser/atom_browser_client.cc
@@ -238,18 +238,18 @@ bool AtomBrowserClient::NavigationWasRedirectedCrossSite(
     content::SiteInstance* speculative_instance,
     const GURL& dest_url,
     bool has_response_started) const {
-  bool navigationWasRedirected = false;
+  bool navigation_was_redirected = false;
   if (has_response_started) {
-    navigationWasRedirected = !IsSameWebSite(
+    navigation_was_redirected = !IsSameWebSite(
         browser_context, current_instance->GetSiteURL(), dest_url);
   } else {
-    navigationWasRedirected =
+    navigation_was_redirected =
         speculative_instance &&
         !IsSameWebSite(browser_context, speculative_instance->GetSiteURL(),
                        dest_url);
   }
 
-  return navigationWasRedirected;
+  return navigation_was_redirected;
 }
 
 void AtomBrowserClient::AddProcessPreferences(