Merge pull request #12686 from Anrock/browserview-handle-window-open

fix: handle window.open events in BrowserView

atom/browser/native_window_mac.mm

@@ -480,6 +480,7 @@ NativeWindowMac::NativeWindowMac(const mate::Dictionary& options,
 }
 
 NativeWindowMac::~NativeWindowMac() {
+  if (wheel_event_monitor_)
     [NSEvent removeMonitor:wheel_event_monitor_];
 }
 
@@ -517,6 +518,13 @@ void NativeWindowMac::Close() {
 }
 
 void NativeWindowMac::CloseImmediately() {
+  // Remove event monitor before destroying window, otherwise the monitor may
+  // call its callback after window has been destroyed.
+  if (wheel_event_monitor_) {
+    [NSEvent removeMonitor:wheel_event_monitor_];
+    wheel_event_monitor_ = nil;
+  }
+
   // Retain the child window before closing it. If the last reference to the
   // NSWindow goes away inside -[NSWindow close], then bad stuff can happen.
   // See e.g. http://crbug.com/616701.

lib/browser/api/browser-window.js

@@ -3,7 +3,6 @@
 const electron = require('electron')
 const { WebContentsView, TopLevelWindow } = electron
 const { BrowserWindow } = process.atomBinding('window')
-const ipcMain = require('@electron/internal/browser/ipc-main-internal')
 
 Object.setPrototypeOf(BrowserWindow.prototype, TopLevelWindow.prototype)
 
@@ -26,43 +25,6 @@ BrowserWindow.prototype._init = function () {
     nativeSetBounds.call(this, bounds, ...opts)
   }
 
-  // Make new windows requested by links behave like "window.open"
-  this.webContents.on('-new-window', (event, url, frameName, disposition,
-    additionalFeatures, postData,
-    referrer) => {
-    const options = {
-      show: true,
-      width: 800,
-      height: 600
-    }
-    ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN',
-      event, url, referrer, frameName, disposition,
-      options, additionalFeatures, postData)
-  })
-
-  // Create a new browser window for the native implementation of
-  // "window.open", used in sandbox and nativeWindowOpen mode
-  this.webContents.on('-add-new-contents', (event, webContents, disposition,
-    userGesture, left, top, width, height, url, frameName) => {
-    if ((disposition !== 'foreground-tab' && disposition !== 'new-window' &&
-         disposition !== 'background-tab')) {
-      event.preventDefault()
-      return
-    }
-
-    const options = {
-      show: true,
-      x: left,
-      y: top,
-      width: width || 800,
-      height: height || 600,
-      webContents: webContents
-    }
-    const referrer = { url: '', policy: 'default' }
-    ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN',
-      event, url, referrer, frameName, disposition, options)
-  })
-
   // window.resizeTo(...)
   // window.moveTo(...)
   this.webContents.on('move', (event, size) => {

lib/browser/api/web-contents.js

@@ -376,6 +376,46 @@ WebContents.prototype._init = function () {
     this.reload()
   })
 
+  // Handle window.open for BrowserWindow and BrowserView.
+  if (['browserview', 'window'].includes(this.getType())) {
+    // Make new windows requested by links behave like "window.open".
+    this.on('-new-window', (event, url, frameName, disposition,
+      additionalFeatures, postData,
+      referrer) => {
+      const options = {
+        show: true,
+        width: 800,
+        height: 600
+      }
+      ipcMainInternal.emit('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN',
+        event, url, referrer, frameName, disposition,
+        options, additionalFeatures, postData)
+    })
+
+    // Create a new browser window for the native implementation of
+    // "window.open", used in sandbox and nativeWindowOpen mode.
+    this.on('-add-new-contents', (event, webContents, disposition,
+      userGesture, left, top, width, height, url, frameName) => {
+      if ((disposition !== 'foreground-tab' && disposition !== 'new-window' &&
+           disposition !== 'background-tab')) {
+        event.preventDefault()
+        return
+      }
+
+      const options = {
+        show: true,
+        x: left,
+        y: top,
+        width: width || 800,
+        height: height || 600,
+        webContents
+      }
+      const referrer = { url: '', policy: 'default' }
+      ipcMainInternal.emit('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN',
+        event, url, referrer, frameName, disposition, options)
+    })
+  }
+
   app.emit('web-contents-created', {}, this)
 }
 

spec/api-browser-view-spec.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const assert = require('assert')
 const chai = require('chai')
 const ChildProcess = require('child_process')
 const dirtyChai = require('dirty-chai')
@@ -14,6 +15,8 @@ const { expect } = chai
 chai.use(dirtyChai)
 
 describe('BrowserView module', () => {
+  const fixtures = path.resolve(__dirname, 'fixtures')
+
   let w = null
   let view = null
 
@@ -178,11 +181,26 @@ describe('BrowserView module', () => {
 
   describe('new BrowserView()', () => {
     it('does not crash on exit', async () => {
-      const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-browserview.js')
+      const appPath = path.join(fixtures, 'api', 'leak-exit-browserview.js')
       const electronPath = remote.getGlobal('process').execPath
       const appProcess = ChildProcess.spawn(electronPath, [appPath])
       const [code] = await emittedOnce(appProcess, 'close')
       expect(code).to.equal(0)
     })
   })
+
+  describe('window.open()', () => {
+    it('works in BrowserView', (done) => {
+      view = new BrowserView()
+      w.setBrowserView(view)
+      view.webContents.once('new-window', (e, url, frameName, disposition, options, additionalFeatures) => {
+        e.preventDefault()
+        assert.strictEqual(url, 'http://host/')
+        assert.strictEqual(frameName, 'host')
+        assert.strictEqual(additionalFeatures[0], 'this-is-not-a-standard-feature')
+        done()
+      })
+      view.webContents.loadFile(path.join(fixtures, 'pages', 'window-open.html'))
+    })
+  })
 })