Merge pull request #14034 from desktop/you-can-crash-if-you-want-to-you-can-leave-your-friends-behind

Avoid spawning from main process on Catalina

app/src/lib/get-os.ts

@@ -21,6 +21,11 @@ function systemVersionGreaterThanOrEqualTo(version: string) {
   return sysver === undefined ? false : compare(sysver, version, '>=')
 }
 
+function systemVersionLessThan(version: string) {
+  const sysver = getSystemVersionSafe()
+  return sysver === undefined ? false : compare(sysver, version, '<')
+}
+
 /** Get the OS we're currently running on. */
 export function getOS() {
   const version = getSystemVersionSafe()
@@ -33,6 +38,11 @@ export function getOS() {
   }
 }
 
+/** We're currently running macOS and it is macOS Catalina or earlier. */
+export const isMacOSCatalinaOrEarlier = memoizeOne(
+  () => __DARWIN__ && systemVersionLessThan('10.16')
+)
+
 /** We're currently running macOS and it is at least Mojave. */
 export const isMacOSMojaveOrLater = memoizeOne(
   () => __DARWIN__ && systemVersionGreaterThanOrEqualTo('10.13.0')

app/src/lib/ipc-shared.ts

@@ -39,8 +39,7 @@ export type RequestChannels = {
     extra: Record<string, string>,
     nonFatal: boolean
   ) => void
-  'show-item-in-folder': (path: string) => void
-  'show-folder-contents': (path: string) => void
+  'unsafe-open-directory': (path: string) => void
   'menu-event': (name: MenuEvent) => void
   log: (level: LogLevel, message: string) => void
   'will-quit': () => void
@@ -89,6 +88,7 @@ export type RequestResponseChannels = {
   'get-app-path': () => Promise<string>
   'is-running-under-arm64-translation': () => Promise<boolean>
   'move-to-trash': (path: string) => Promise<void>
+  'show-item-in-folder': (path: string) => Promise<void>
   'show-contextual-menu': (
     items: ReadonlyArray<ISerializableMenuItem>,
     addSpellCheckMenu: boolean

app/src/lib/shell.ts

@@ -2,6 +2,7 @@
 
 import * as ChildProcess from 'child_process'
 import * as os from 'os'
+import { isMacOSCatalinaOrEarlier } from './get-os'
 
 type IndexLookup = {
   [propName: string]: string
@@ -23,6 +24,15 @@ const ExcludedEnvironmentVars = new Set(['LOCAL_GIT_DIRECTORY'])
  * @param process The process to inspect.
  */
 export function shellNeedsPatching(process: NodeJS.Process): boolean {
+  // We don't want to run this in the main process until the following issues
+  // are closed (and possibly not after they're closed either)
+  //
+  // See https://github.com/desktop/desktop/issues/13974
+  // See https://github.com/electron/electron/issues/32718
+  if (process.type === 'browser' && isMacOSCatalinaOrEarlier()) {
+    return false
+  }
+
   return __DARWIN__ && !process.env.PWD
 }
 

app/src/main-process/main.ts

@@ -29,8 +29,6 @@ import {
 import { now } from './now'
 import { showUncaughtException } from './show-uncaught-exception'
 import { buildContextMenu } from './menu/build-context-menu'
-import { stat } from 'fs-extra'
-import { isApplicationBundle } from '../lib/is-application-bundle'
 import { OrderedWebRequest } from './ordered-webrequest'
 import { installAuthenticatedAvatarFilter } from './authenticated-avatar-filter'
 import { installAliveOriginFilter } from './alive-origin-filter'
@@ -581,64 +579,13 @@ app.on('ready', () => {
   })
 
   ipcMain.handle('move-to-trash', (_, path) => shell.trashItem(path))
+  ipcMain.handle('show-item-in-folder', async (_, path) =>
+    shell.showItemInFolder(path)
+  )
 
-  ipcMain.on('show-item-in-folder', (_, path) => {
-    Fs.stat(path, err => {
-      if (err) {
-        log.error(`Unable to find file at '${path}'`, err)
-        return
-      }
-      shell.showItemInFolder(path)
-    })
-  })
-
-  ipcMain.on('show-folder-contents', async (_, path) => {
-    const stats = await stat(path).catch(err => {
-      log.error(`Unable to retrieve file information for ${path}`, err)
-      return null
-    })
-
-    if (!stats) {
-      return
-    }
-
-    if (!stats.isDirectory()) {
-      log.error(
-        `Trying to get the folder contents of a non-folder at '${path}'`
-      )
-      shell.showItemInFolder(path)
-      return
-    }
-
-    // On Windows and Linux we can count on a directory being just a
-    // directory.
-    if (!__DARWIN__) {
-      UNSAFE_openDirectory(path)
-      return
-    }
-
-    // On macOS a directory might also be an app bundle and if it is
-    // and we attempt to open it we're gonna execute that app which
-    // it far from ideal so we'll look up the metadata for the path
-    // and attempt to determine whether it's an app bundle or not.
-    //
-    // If we fail loading the metadata we'll assume it's an app bundle
-    // out of an abundance of caution.
-    const isBundle = await isApplicationBundle(path).catch(err => {
-      log.error(`Failed to load metadata for path '${path}'`, err)
-      return true
-    })
-
-    if (isBundle) {
-      log.info(
-        `Preventing direct open of path '${path}' as it appears to be an application bundle`
-      )
-
-      shell.showItemInFolder(path)
-    } else {
-      UNSAFE_openDirectory(path)
-    }
-  })
+  ipcMain.on('unsafe-open-directory', async (_, path) =>
+    UNSAFE_openDirectory(path)
+  )
 
   /** An event sent by the renderer asking to select all of the window's contents */
   ipcMain.on('select-all-window-contents', () =>

app/src/ui/main-process-proxy.ts

@@ -1,6 +1,8 @@
 import { ExecutableMenuItem } from '../models/app-menu'
 import { RequestResponseChannels, RequestChannels } from '../lib/ipc-shared'
 import * as ipcRenderer from '../lib/ipc-renderer'
+import { stat } from 'fs-extra'
+import { isApplicationBundle } from '../lib/is-application-bundle'
 
 /**
  * Creates a strongly typed proxy method for sending a duplex IPC message to the
@@ -85,8 +87,61 @@ export const isWindowFocused = invokeProxy('is-window-focused', 0)
 /** Tell the main process to focus on the main window. */
 export const focusWindow = sendProxy('focus-window', 0)
 
-export const showItemInFolder = sendProxy('show-item-in-folder', 1)
-export const showFolderContents = sendProxy('show-folder-contents', 1)
+const _showItemInFolder = invokeProxy('show-item-in-folder', 1)
+
+export const showItemInFolder = (path: string) =>
+  stat(path)
+    .then(() => _showItemInFolder(path))
+    .catch(err => log.error(`Unable show item in folder '${path}'`, err))
+
+const UNSAFE_openDirectory = sendProxy('unsafe-open-directory', 1)
+
+export async function showFolderContents(path: string) {
+  const stats = await stat(path).catch(err => {
+    log.error(`Unable to retrieve file information for ${path}`, err)
+    return null
+  })
+
+  if (!stats) {
+    return
+  }
+
+  if (!stats.isDirectory()) {
+    log.error(`Trying to get the folder contents of a non-folder at '${path}'`)
+    await _showItemInFolder(path)
+    return
+  }
+
+  // On Windows and Linux we can count on a directory being just a
+  // directory.
+  if (!__DARWIN__) {
+    UNSAFE_openDirectory(path)
+    return
+  }
+
+  // On macOS a directory might also be an app bundle and if it is
+  // and we attempt to open it we're gonna execute that app which
+  // it far from ideal so we'll look up the metadata for the path
+  // and attempt to determine whether it's an app bundle or not.
+  //
+  // If we fail loading the metadata we'll assume it's an app bundle
+  // out of an abundance of caution.
+  const isBundle = await isApplicationBundle(path).catch(err => {
+    log.error(`Failed to load metadata for path '${path}'`, err)
+    return true
+  })
+
+  if (isBundle) {
+    log.info(
+      `Preventing direct open of path '${path}' as it appears to be an application bundle`
+    )
+
+    await _showItemInFolder(path)
+  } else {
+    UNSAFE_openDirectory(path)
+  }
+}
+
 export const openExternal = invokeProxy('open-external', 1)
 export const moveItemToTrash = invokeProxy('move-to-trash', 1)