fix: lost window.opener after cross-origin navigation (#18173)

* Get a site instance related to current one instead of creation a new one

Using `GetRelatedSiteInstance` will keep the relation (same browsing instance) between the current and the new site instance.

* Some relies on preloads in opened window

The fact that, now, we always have an opener for opened windows diables note integration in opened windows, except if `nodeIntegrationInSubFrames` is enabled.

* Add a test on window.opener after cross-orgin navigation

* Make sure to unregisterProtocol in tests

* Introduc and use a NetworkSandbox for tests

* Modify tests about zoom persistence to properly simulate cross-origin navigation

* Revert "Modify tests about zoom persistence to properly simulate cross-origin navigation"

This reverts commit 0a7537f2eb7f183ddec16637e8a2e92a0d600321.

patches/common/chromium/frame_host_manager.patch

@@ -7,8 +7,42 @@ Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance. Also allows for us to at-runtime
 enable or disable this patch.
 
+diff --git a/content/browser/browsing_instance.cc b/content/browser/browsing_instance.cc
+index 12e1c5cff95aa6d0a907a249208e23371cf29785..3bc26b7870ff3bf6a69cb1e123fb372f763442ee 100644
+--- a/content/browser/browsing_instance.cc
++++ b/content/browser/browsing_instance.cc
+@@ -79,6 +79,13 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURL(
+   return instance;
+ }
+ 
++scoped_refptr<SiteInstanceImpl> BrowsingInstance::CreateSiteInstanceForURL(
++    const GURL& url) {
++  scoped_refptr<SiteInstanceImpl> instance = new SiteInstanceImpl(this);
++  instance->SetSite(url);
++  return instance;
++}
++
+ void BrowsingInstance::GetSiteAndLockForURL(const GURL& url,
+                                             bool allow_default_instance,
+                                             GURL* site_url,
+diff --git a/content/browser/browsing_instance.h b/content/browser/browsing_instance.h
+index 775b64a8d20f89845812852a2904a1e6875c2b4a..5235b57bbf44fc7b30ca6943c43a290f07f003bf 100644
+--- a/content/browser/browsing_instance.h
++++ b/content/browser/browsing_instance.h
+@@ -136,6 +136,11 @@ class CONTENT_EXPORT BrowsingInstance final
+       const GURL& url,
+       bool allow_default_instance);
+ 
++  // Create a new SiteInstance for the given URL bound the current
++  // BrowsingInstance.
++  scoped_refptr<SiteInstanceImpl> CreateSiteInstanceForURL(
++      const GURL& url);
++
+   // Adds the given SiteInstance to our map, to ensure that we do not create
+   // another SiteInstance for the same site.
+   void RegisterSiteInstance(SiteInstanceImpl* site_instance);
 diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
-index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb5a567cba 100644
+index b5301d164138f21ca8ae01abfb153efde51ec324..55f87cc788c7eed13494ebe6ea6eb18027a04996 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
 @@ -2127,6 +2127,20 @@ bool RenderFrameHostManager::InitRenderView(
@@ -56,7 +90,7 @@ index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb
 +          overriden_site_instance =
 +              candidate_site_instance
 +                  ? candidate_site_instance
-+                  : SiteInstance::CreateForURL(browser_context,
++                  : current_site_instance->CreateRelatedSiteInstance(
 +                                              request.common_params().url);
 +          break;
 +        case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT:
@@ -117,6 +151,33 @@ index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb
    return dest_site_instance;
  }
  
+diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
+index fd184108a7993094c29be3f7ebde658e259ede2c..75aa4a6b7d58a1bebe34efc923953c69348428a9 100644
+--- a/content/browser/site_instance_impl.cc
++++ b/content/browser/site_instance_impl.cc
+@@ -342,6 +342,10 @@ bool SiteInstanceImpl::HasRelatedSiteInstance(const GURL& url) {
+   return browsing_instance_->HasSiteInstance(url);
+ }
+ 
++scoped_refptr<SiteInstance> SiteInstanceImpl::CreateRelatedSiteInstance(const GURL& url) {
++  return browsing_instance_->CreateSiteInstanceForURL(url);
++}
++
+ scoped_refptr<SiteInstance> SiteInstanceImpl::GetRelatedSiteInstance(
+     const GURL& url) {
+   return browsing_instance_->GetSiteInstanceForURL(
+diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h
+index a46901055bdf17b6b0dab14edf753b234dc04a12..29c201b0c95eb0c7a35f47d6f3ab5b48c73dbf15 100644
+--- a/content/browser/site_instance_impl.h
++++ b/content/browser/site_instance_impl.h
+@@ -83,6 +83,7 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
+   BrowserContext* GetBrowserContext() override;
+   const GURL& GetSiteURL() override;
+   scoped_refptr<SiteInstance> GetRelatedSiteInstance(const GURL& url) override;
++	scoped_refptr<SiteInstance> CreateRelatedSiteInstance(const GURL& url) override;
+   bool IsRelatedSiteInstance(const SiteInstance* instance) override;
+   size_t GetRelatedActiveContentsCount() override;
+   bool RequiresDedicatedProcess() override;
 diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc
 index 787cd81b26508d3e04956721f0e7cec2f457aa60..8e62a5dd26595411757e03078ed0e44646c47a52 100644
 --- a/content/public/browser/content_browser_client.cc
@@ -187,3 +248,19 @@ index bf9b3a601fc16d5070e4467a258a047f47b059f3..3c35eddc2498157c2b98bab55991d8aa
    // Allows the embedder to set any number of custom BrowserMainParts
    // implementations for the browser startup code. See comments in
    // browser_main_parts.h.
+diff --git a/content/public/browser/site_instance.h b/content/public/browser/site_instance.h
+index a3e880e20e51d988175f0e8e2c42e7f5c1740104..61bbf88265e717934533117efbc2330661e32b11 100644
+--- a/content/public/browser/site_instance.h
++++ b/content/public/browser/site_instance.h
+@@ -121,6 +121,11 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
+   //   corresponds to a site URL with the host "example.com".
+   virtual const GURL& GetSiteURL() = 0;
+ 
++	// Create a SiteInstance for the given URL that shares the current
++	// BrowsingInstance.
++	virtual scoped_refptr<SiteInstance> CreateRelatedSiteInstance(
++	    const GURL& url) = 0;
++
+   // Gets a SiteInstance for the given URL that shares the current
+   // BrowsingInstance, creating a new SiteInstance if necessary.  This ensures
+   // that a BrowsingInstance only has one SiteInstance per site, so that pages

spec/api-browser-window-spec.js

@@ -9,6 +9,7 @@ const qs = require('querystring')
 const http = require('http')
 const { closeWindow } = require('./window-helpers')
 const { emittedOnce } = require('./events-helpers')
+const { createNetworkSandbox } = require('./network-helper')
 const { ipcRenderer, remote } = require('electron')
 const { app, ipcMain, BrowserWindow, BrowserView, protocol, session, screen, webContents } = remote
 
@@ -1997,17 +1998,29 @@ describe('BrowserWindow module', () => {
     })
 
     describe('nativeWindowOpen option', () => {
-      beforeEach(() => {
+      const networkSandbox = createNetworkSandbox(protocol)
+
+      beforeEach(async () => {
+        // used to create cross-origin navigation situations
+        await networkSandbox.serveFileFromProtocol('foo', path.join(fixtures, 'api', 'window-open-location-change.html'))
+        await networkSandbox.serveFileFromProtocol('bar', path.join(fixtures, 'api', 'window-open-location-final.html'))
+
         w.destroy()
         w = new BrowserWindow({
           show: false,
           webPreferences: {
             nodeIntegration: true,
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // tests relies on preloads in opened windows
+            nodeIntegrationInSubFrames: true
           }
         })
       })
 
+      afterEach(async () => {
+        await networkSandbox.reset()
+      })
+
       it('opens window of about:blank with cross-scripting enabled', (done) => {
         ipcMain.once('answer', (event, content) => {
           expect(content).to.equal('Hello')
@@ -2052,7 +2065,9 @@ describe('BrowserWindow module', () => {
         w = new BrowserWindow({
           show: false,
           webPreferences: {
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
           }
         })
 
@@ -2069,7 +2084,9 @@ describe('BrowserWindow module', () => {
         w = new BrowserWindow({
           show: false,
           webPreferences: {
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
           }
         })
 
@@ -2083,14 +2100,13 @@ describe('BrowserWindow module', () => {
         w.loadFile(path.join(fixtures, 'api', 'new-window.html'))
       })
       it('retains the original web preferences when window.location is changed to a new origin', async () => {
-        await serveFileFromProtocol('foo', path.join(fixtures, 'api', 'window-open-location-change.html'))
-        await serveFileFromProtocol('bar', path.join(fixtures, 'api', 'window-open-location-final.html'))
-
         w.destroy()
         w = new BrowserWindow({
           show: true,
           webPreferences: {
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
           }
         })
 
@@ -2103,7 +2119,33 @@ describe('BrowserWindow module', () => {
         expect(typeofProcess).to.eql('undefined')
       })
 
+      it('window.opener is not null when window.location is changed to a new origin', async () => {
+        w.destroy()
+        w = new BrowserWindow({
+          show: true,
+          webPreferences: {
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
+          }
+        })
+
+        ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', path.join(fixtures, 'api', 'window-open-preload.js'))
+        const p = emittedOnce(ipcMain, 'answer')
+        w.loadFile(path.join(fixtures, 'api', 'window-open-location-open.html'))
+        const [, , , windowOpenerIsNull] = await p
+        expect(windowOpenerIsNull).to.be.false('window.opener is null')
+      })
+
       it('should have nodeIntegration disabled in child windows', async () => {
+        w.destroy()
+        w = new BrowserWindow({
+          show: false,
+          webPreferences: {
+            nodeIntegration: true,
+            nativeWindowOpen: true
+          }
+        })
         const p = emittedOnce(ipcMain, 'answer')
         w.loadFile(path.join(fixtures, 'api', 'native-window-open-argv.html'))
         const [, typeofProcess] = await p
@@ -3777,22 +3819,3 @@ const isScaleFactorRounding = () => {
   // Return true if scale factor is odd number above 2
   return scaleFactor > 2 && scaleFactor % 2 === 1
 }
-
-function serveFileFromProtocol (protocolName, filePath) {
-  return new Promise((resolve, reject) => {
-    protocol.registerBufferProtocol(protocolName, (request, callback) => {
-      // Disabled due to false positive in StandardJS
-      // eslint-disable-next-line standard/no-callback-literal
-      callback({
-        mimeType: 'text/html',
-        data: fs.readFileSync(filePath)
-      })
-    }, (error) => {
-      if (error != null) {
-        reject(error)
-      } else {
-        resolve()
-      }
-    })
-  })
-}

spec/fixtures/api/window-open-preload.js

@@ -2,7 +2,8 @@ const { ipcRenderer } = require('electron')
 
 setImmediate(function () {
   if (window.location.toString() === 'bar://page') {
-    ipcRenderer.send('answer', process.argv, typeof global.process)
+    const windowOpenerIsNull = window.opener == null
+    ipcRenderer.send('answer', process.argv, typeof global.process, windowOpenerIsNull)
     window.close()
   }
 })

spec/network-helper.js

@@ -0,0 +1,75 @@
+const fs = require('fs')
+
+/**
+ * Test sandbox environment used to fake network responses.
+ */
+class NetworkSandbox {
+  constructor (protocol) {
+    this.protocol = protocol
+    this._resetFns = []
+  }
+
+  /**
+   * Reset the sandbox.
+   */
+  async reset () {
+    for (const resetFn of this._resetFns) {
+      await resetFn()
+    }
+    this._resetFns = []
+  }
+
+  /**
+   * Will serve the content of file at `filePath` to network requests towards
+   * `protocolName` scheme.
+   *
+   * Example: `sandbox.serveFileFromProtocol('foo', 'index.html')` will serve the content
+   * of 'index.html' to `foo://page` requests.
+   *
+   * @param {string} protocolName
+   * @param {string} filePath
+   */
+  serveFileFromProtocol (protocolName, filePath) {
+    return new Promise((resolve, reject) => {
+      this.protocol.registerBufferProtocol(protocolName, (request, callback) => {
+        // Disabled due to false positive in StandardJS
+        // eslint-disable-next-line standard/no-callback-literal
+        callback({
+          mimeType: 'text/html',
+          data: fs.readFileSync(filePath)
+        })
+      }, (error) => {
+        if (error != null) {
+          reject(error)
+        } else {
+          this._resetFns.push(() => this.unregisterProtocol(protocolName))
+          resolve()
+        }
+      })
+    })
+  }
+
+  unregisterProtocol (protocolName) {
+    return new Promise((resolve, reject) => {
+      this.protocol.unregisterProtocol(protocolName, (error) => {
+        if (error != null) {
+          reject(error)
+        } else {
+          resolve()
+        }
+      })
+    })
+  }
+}
+
+/**
+ * Will create a NetworkSandbox that uses
+ * `protocol` as `Electron.Protocol`.
+ *
+ * @param {Electron.Protocol} protocol
+ */
+function createNetworkSandbox (protocol) {
+  return new NetworkSandbox(protocol)
+}
+
+exports.createNetworkSandbox = createNetworkSandbox