refactor: use IPC helpers in window-setup (#17948)

docs/api/browser-window-proxy.md

@@ -35,7 +35,7 @@ Invokes the print dialog on the child window.
 
 #### `win.postMessage(message, targetOrigin)`
 
-* `message` String
+* `message` any
 * `targetOrigin` String
 
 Sends a message to the child window with the specified origin or `*` for no

filenames.auto.gni

@@ -166,7 +166,9 @@ auto_filenames = {
 
   isolated_bundle_deps = [
     "lib/common/electron-binding-setup.ts",
+    "lib/common/error-utils.js",
     "lib/isolated_renderer/init.js",
+    "lib/renderer/ipc-renderer-internal-utils.ts",
     "lib/renderer/ipc-renderer-internal.ts",
     "lib/renderer/web-view/web-view-constants.ts",
     "lib/renderer/web-view/web-view-element.ts",

lib/browser/guest-window-manager.js

@@ -3,6 +3,7 @@
 const { BrowserWindow, webContents } = require('electron')
 const { isSameOrigin } = process.electronBinding('v8_util')
 const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal')
+const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils')
 const parseFeaturesString = require('@electron/internal/common/parse-features-string')
 
 const hasProp = {}.hasOwnProperty
@@ -153,6 +154,9 @@ const getGuestWindow = function (guestContents) {
       guestWindow = BrowserWindow.fromWebContents(hostContents)
     }
   }
+  if (!guestWindow) {
+    throw new Error('getGuestWindow failed')
+  }
   return guestWindow
 }
 
@@ -267,53 +271,42 @@ ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN', functio
   }
 })
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestId) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
+const handleMessage = function (channel, handler) {
+  ipcMainUtils.handle(channel, (event, guestId, ...args) => {
+    const guestContents = webContents.fromId(guestId)
+    if (!guestContents) {
+      throw new Error(`Invalid guestId: ${guestId}`)
+    }
 
-  if (!canAccessWindow(event.sender, guestContents)) {
-    console.error(`Blocked ${event.sender.getURL()} from closing its opener.`)
-    return
-  }
-
-  const guestWindow = getGuestWindow(guestContents)
-  if (guestWindow != null) guestWindow.destroy()
-})
+    return handler(event, guestContents, ...args)
+  })
+}
 
 const windowMethods = new Set([
+  'destroy',
   'focus',
   'blur'
 ])
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) {
-    event.returnValue = null
-    return
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', (event, guestContents, method, ...args) => {
+  if (!canAccessWindow(event.sender, guestContents)) {
+    console.error(`Blocked ${event.sender.getURL()} from accessing guestId: ${guestContents.id}`)
+    throw new Error(`Access denied to guestId: ${guestContents.id}`)
   }
 
-  if (!canAccessWindow(event.sender, guestContents) || !windowMethods.has(method)) {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
-    event.returnValue = null
-    return
+  if (!windowMethods.has(method)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling method: ${method}`)
+    throw new Error(`Invalid method: ${method}`)
   }
 
-  const guestWindow = getGuestWindow(guestContents)
-  if (guestWindow != null) {
-    event.returnValue = guestWindow[method](...args)
-  } else {
-    event.returnValue = null
-  }
+  return getGuestWindow(guestContents)[method](...args)
 })
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, guestId, message, targetOrigin, sourceOrigin) {
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', (event, guestContents, message, targetOrigin, sourceOrigin) => {
   if (targetOrigin == null) {
     targetOrigin = '*'
   }
 
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
-
   // The W3C does not seem to have word on how postMessage should work when the
   // origins do not match, so we do not do |canAccessWindow| check here since
   // postMessage across origins is useful and not harmful.
@@ -324,37 +317,22 @@ ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function
 })
 
 const webContentsMethods = new Set([
-  'print',
-  'executeJavaScript'
-])
-
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
-
-  if (canAccessWindow(event.sender, guestContents) && webContentsMethods.has(method)) {
-    guestContents[method](...args)
-  } else {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
-  }
-})
-
-const webContentsSyncMethods = new Set([
   'getURL',
-  'loadURL'
+  'loadURL',
+  'executeJavaScript',
+  'print'
 ])
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) {
-    event.returnValue = null
-    return
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', (event, guestContents, method, ...args) => {
+  if (!canAccessWindow(event.sender, guestContents)) {
+    console.error(`Blocked ${event.sender.getURL()} from accessing guestId: ${guestContents.id}`)
+    throw new Error(`Access denied to guestId: ${guestContents.id}`)
   }
 
-  if (canAccessWindow(event.sender, guestContents) && webContentsSyncMethods.has(method)) {
-    event.returnValue = guestContents[method](...args)
-  } else {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
-    event.returnValue = null
+  if (!webContentsMethods.has(method)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling method: ${method}`)
+    throw new Error(`Invalid method: ${method}`)
   }
+
+  return guestContents[method](...args)
 })

lib/browser/navigation-controller.js

@@ -1,22 +1,22 @@
 'use strict'
 
-const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal')
+const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils')
 
 // The history operation in renderer is redirected to browser.
-ipcMainInternal.on('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK', function (event) {
-  event.sender.goBack()
+ipcMainUtils.handle('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK', function (event) {
+  return event.sender.goBack()
 })
 
-ipcMainInternal.on('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD', function (event) {
-  event.sender.goForward()
+ipcMainUtils.handle('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD', function (event) {
+  return event.sender.goForward()
 })
 
-ipcMainInternal.on('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', function (event, offset) {
-  event.sender.goToOffset(offset)
+ipcMainUtils.handle('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', function (event, offset) {
+  return event.sender.goToOffset(offset)
 })
 
-ipcMainInternal.on('ELECTRON_NAVIGATION_CONTROLLER_LENGTH', function (event) {
-  event.returnValue = event.sender.length()
+ipcMainUtils.handle('ELECTRON_NAVIGATION_CONTROLLER_LENGTH', function (event) {
+  return event.sender.length()
 })
 
 // JavaScript implementation of Chromium's NavigationController.

lib/renderer/window-setup.ts

@@ -1,4 +1,5 @@
 import { ipcRendererInternal } from '@electron/internal/renderer/ipc-renderer-internal'
+import * as ipcRendererUtils from '@electron/internal/renderer/ipc-renderer-internal-utils'
 
 // This file implements the following APIs:
 // - window.history.back()
@@ -17,8 +18,6 @@ import { ipcRendererInternal } from '@electron/internal/renderer/ipc-renderer-in
 // - document.hidden
 // - document.visibilityState
 
-const { defineProperty } = Object
-
 // Helper function to resolve relative url.
 const resolveURL = (url: string, base: string) => new URL(url, base).href
 
@@ -77,7 +76,7 @@ class LocationProxy {
           // It's right, that's bad, but we're doing it anway.
           (guestURL as any)[propertyKey] = newVal
 
-          return this._invokeWebContentsMethodSync('loadURL', guestURL.toString())
+          return this._invokeWebContentsMethod('loadURL', guestURL.toString())
         }
       }
     })
@@ -107,7 +106,7 @@ class LocationProxy {
   }
 
   private _invokeWebContentsMethodSync (method: string, ...args: any[]) {
-    return ipcRendererInternal.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', this.guestId, method, ...args)
+    return ipcRendererUtils.invokeSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
   }
 }
 
@@ -125,7 +124,7 @@ class BrowserWindowProxy {
   }
   public set location (url: string | any) {
     url = resolveURL(url, this.location.href)
-    this._invokeWebContentsMethodSync('loadURL', url)
+    this._invokeWebContentsMethod('loadURL', url)
   }
 
   constructor (guestId: number) {
@@ -139,7 +138,7 @@ class BrowserWindowProxy {
   }
 
   public close () {
-    ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', this.guestId)
+    this._invokeWindowMethod('destroy')
   }
 
   public focus () {
@@ -154,8 +153,8 @@ class BrowserWindowProxy {
     this._invokeWebContentsMethod('print')
   }
 
-  public postMessage (message: any, targetOrigin: any) {
-    ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', this.guestId, message, toString(targetOrigin), window.location.origin)
+  public postMessage (message: any, targetOrigin: string) {
+    ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', this.guestId, message, toString(targetOrigin), window.location.origin)
   }
 
   public eval (code: string) {
@@ -163,15 +162,11 @@ class BrowserWindowProxy {
   }
 
   private _invokeWindowMethod (method: string, ...args: any[]) {
-    return ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', this.guestId, method, ...args)
+    return ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', this.guestId, method, ...args)
   }
 
   private _invokeWebContentsMethod (method: string, ...args: any[]) {
-    return ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
-  }
-
-  private _invokeWebContentsMethodSync (method: string, ...args: any[]) {
-    return ipcRendererInternal.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', this.guestId, method, ...args)
+    return ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
   }
 }
 
@@ -229,20 +224,20 @@ export const windowSetup = (
   })
 
   window.history.back = function () {
-    ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK')
+    ipcRendererUtils.invoke('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK')
   }
 
   window.history.forward = function () {
-    ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD')
+    ipcRendererUtils.invoke('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD')
   }
 
   window.history.go = function (offset: number) {
-    ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset)
+    ipcRendererUtils.invoke('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset)
   }
 
-  defineProperty(window.history, 'length', {
+  Object.defineProperty(window.history, 'length', {
     get: function () {
-      return ipcRendererInternal.sendSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH')
+      return ipcRendererUtils.invokeSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH')
     }
   })
 
@@ -265,13 +260,13 @@ export const windowSetup = (
     })
 
     // Make document.hidden and document.visibilityState return the correct value.
-    defineProperty(document, 'hidden', {
+    Object.defineProperty(document, 'hidden', {
       get: function () {
         return cachedVisibilityState !== 'visible'
       }
     })
 
-    defineProperty(document, 'visibilityState', {
+    Object.defineProperty(document, 'visibilityState', {
       get: function () {
         return cachedVisibilityState
       }

spec/chromium-spec.js

@@ -618,9 +618,9 @@ describe('chromium feature', () => {
       w.close()
     })
 
-    it('does nothing when origin of current window does not match opener', (done) => {
+    it('fails when origin of current window does not match opener', (done) => {
       listener = (event) => {
-        expect(event.data).to.equal('')
+        expect(event.data).to.equal(null)
         done()
       }
       window.addEventListener('message', listener)
@@ -666,10 +666,10 @@ describe('chromium feature', () => {
       if (webview != null) webview.remove()
     })
 
-    it('does nothing when origin of webview src URL does not match opener', (done) => {
+    it('fails when origin of webview src URL does not match opener', (done) => {
       webview = new WebView()
       webview.addEventListener('console-message', (e) => {
-        expect(e.message).to.equal('')
+        expect(e.message).to.equal('null')
         done()
       })
       webview.setAttribute('allowpopups', 'on')

spec/fixtures/pages/window-opener-location.html

@@ -1,7 +1,11 @@
 <html>
 <body>
 <script type="text/javascript" charset="utf-8">
-  window.opener.postMessage(window.opener.location.href, '*')
+  try {
+    window.opener.postMessage(window.opener.location.href, '*')
+  } catch {
+    window.opener.postMessage(null, '*')
+  }
 </script>
 </body>
 </html>