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

* SiteInstance::CreateRelatedSiteInstance and use it

* Introduce and use a NetworkSandbox and introduce a test for window.opener on x-site navigation

patches/common/chromium/frame_host_manager.patch

@@ -6,8 +6,42 @@ Subject: frame_host_manager.patch
 Allows embedder to intercept site instances chosen by chromium
 and respond with custom instance.
 
+diff --git a/content/browser/browsing_instance.cc b/content/browser/browsing_instance.cc
+index 12e1c5cff95aa6d0a907a249208e23371cf29785..e64703c0edac7f7e9af211b0130a776e6ce3f953 100644
+--- a/content/browser/browsing_instance.cc
++++ b/content/browser/browsing_instance.cc
+@@ -138,6 +138,13 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
+   return nullptr;
+ }
+ 
++scoped_refptr<SiteInstanceImpl> BrowsingInstance::CreateSiteInstanceForURL(
++    const GURL& url) {
++  scoped_refptr<SiteInstanceImpl> instance = new SiteInstanceImpl(this);
++  instance->SetSite(url);
++  return instance;
++}
++
+ void BrowsingInstance::RegisterSiteInstance(SiteInstanceImpl* site_instance) {
+   DCHECK(site_instance->browsing_instance_.get() == this);
+   DCHECK(site_instance->HasSite());
+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 297b61198dd46114b3d8c89488a71ed01aa299c4..0f64b4926f9077fbf5c4729fed5888088b6912e8 100644
+index 297b61198dd46114b3d8c89488a71ed01aa299c4..1f0ec6c02e8b189c9170441e2c8709cc1b561c84 100644
 --- a/content/browser/frame_host/render_frame_host_manager.cc
 +++ b/content/browser/frame_host/render_frame_host_manager.cc
 @@ -2127,6 +2127,16 @@ bool RenderFrameHostManager::InitRenderView(
@@ -50,7 +84,7 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..0f64b4926f9077fbf5c4729fed588808
 +        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:
@@ -107,6 +141,33 @@ index 297b61198dd46114b3d8c89488a71ed01aa299c4..0f64b4926f9077fbf5c4729fed588808
    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 d15061de5254fd4f248fed92f47a1b1fcf34cd68..671d2259d50bfbb5b56a79907c1c86218ee04469 100644
 --- a/content/public/browser/content_browser_client.cc
@@ -170,3 +231,19 @@ index e5cddbc3a28e0f90bd1d1ae6ebe28d5f2d0299c7..fcc5a0e5cd830afe459c1defe968b01e
    // 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

@@ -10,6 +10,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
 
@@ -305,7 +306,6 @@ describe('BrowserWindow module', () => {
     })
 
     it('should return a promise that resolves even if pushState occurs during navigation', async () => {
-      const data = Buffer.alloc(2 * 1024 * 1024).toString('base64')
       const p = w.loadURL('data:text/html,<script>window.history.pushState({}, "/foo")</script>')
       await expect(p).to.eventually.be.fulfilled()
     })
@@ -2033,17 +2033,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) => {
           assert.strictEqual(content, 'Hello')
@@ -2088,7 +2100,9 @@ describe('BrowserWindow module', () => {
         w = new BrowserWindow({
           show: false,
           webPreferences: {
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
           }
         })
 
@@ -2105,7 +2119,9 @@ describe('BrowserWindow module', () => {
         w = new BrowserWindow({
           show: false,
           webPreferences: {
-            nativeWindowOpen: true
+            nativeWindowOpen: true,
+            // test relies on preloads in opened window
+            nodeIntegrationInSubFrames: true
           }
         })
 
@@ -2119,14 +2135,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
           }
         })
 
@@ -2139,7 +2154,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
@@ -3836,22 +3877,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