Browse Source

fix: properly pass openExternal activate option (#18657)

* fix: properly pass openExternal activate option

A reference to an OpenExternalOptions structure was being captured by an Objective-C block that
outlived the object that was being referenced.

* Fix test in CI

* Don't check for activate on linux

* Close BrowserWindow
Jeremy Spiegel 5 years ago
parent
commit
64f7974252
2 changed files with 34 additions and 10 deletions
  1. 8 7
      atom/common/platform_util_mac.mm
  2. 26 3
      spec/api-shell-spec.js

+ 8 - 7
atom/common/platform_util_mac.mm

@@ -104,14 +104,15 @@ void OpenExternal(const GURL& url,
     return;
   }
 
+  bool activate = options.activate;
   __block OpenExternalCallback c = std::move(callback);
-  dispatch_async(
-      dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-        __block std::string error = OpenURL(ns_url, options.activate);
-        dispatch_async(dispatch_get_main_queue(), ^{
-          std::move(c).Run(error);
-        });
-      });
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                 ^{
+                   __block std::string error = OpenURL(ns_url, activate);
+                   dispatch_async(dispatch_get_main_queue(), ^{
+                     std::move(c).Run(error);
+                   });
+                 });
 }
 
 bool MoveItemToTrash(const base::FilePath& full_path) {

+ 26 - 3
spec/api-shell-spec.js

@@ -4,7 +4,10 @@ const dirtyChai = require('dirty-chai')
 const fs = require('fs')
 const path = require('path')
 const os = require('os')
-const { shell } = require('electron')
+const { shell, remote } = require('electron')
+const { BrowserWindow } = remote
+
+const { closeWindow } = require('./window-helpers')
 
 const { expect } = chai
 chai.use(dirtyChai)
@@ -23,6 +26,7 @@ describe('shell module', () => {
 
   describe('shell.openExternal()', () => {
     let envVars = {}
+    let w
 
     beforeEach(function () {
       envVars = {
@@ -32,7 +36,9 @@ describe('shell module', () => {
       }
     })
 
-    afterEach(() => {
+    afterEach(async () => {
+      await closeWindow(w)
+      w = null
       // reset env vars to prevent side effects
       if (process.platform === 'linux') {
         process.env.DE = envVars.de
@@ -49,7 +55,24 @@ describe('shell module', () => {
         process.env.DISPLAY = ''
       }
 
-      shell.openExternal(url).then(() => done())
+      // Ensure an external window is activated via a new window's blur event
+      w = new BrowserWindow()
+      let promiseResolved = false
+      let blurEventEmitted = false
+
+      w.on('blur', () => {
+        blurEventEmitted = true
+        if (promiseResolved) {
+          done()
+        }
+      })
+
+      shell.openExternal(url).then(() => {
+        promiseResolved = true
+        if (blurEventEmitted || process.platform === 'linux') {
+          done()
+        }
+      })
     })
   })