Merge pull request #8511 from electron/start-drag-crash

Fix startDrag crash on macOS with empty image

atom/browser/api/atom_api_web_contents.cc

@@ -1327,17 +1327,25 @@ void WebContents::StartDrag(const mate::Dictionary& item,
 
   // Error checking.
   if (icon.IsEmpty()) {
-    args->ThrowError("icon must be set");
+    args->ThrowError("Must specify 'icon' option");
     return;
   }
 
+#if defined(OS_MACOSX)
+  // NSWindow.dragImage requires a non-empty NSImage
+  if (icon->image().IsEmpty()) {
+    args->ThrowError("Must specify non-empty 'icon' option");
+    return;
+  }
+#endif
+
   // Start dragging.
   if (!files.empty()) {
     base::MessageLoop::ScopedNestableTaskAllower allow(
         base::MessageLoop::current());
     DragFileItems(files, icon->image(), web_contents()->GetNativeView());
   } else {
-    args->ThrowError("There is nothing to drag");
+    args->ThrowError("Must specify either 'file' or 'files' option");
   }
 }
 

docs/api/web-contents.md

@@ -1132,8 +1132,9 @@ End subscribing for frame presentation events.
 #### `contents.startDrag(item)`
 
 * `item` Object
-  * `file` String
-  * `icon` [NativeImage](native-image.md)
+  * `file` String - The path to the file being dragged.
+  * `icon` [NativeImage](native-image.md) - The image must be non-empty on
+    macOS.
 
 Sets the `item` as dragging item for current drag-drop operation, `file` is the
 absolute path of the file to be dragged, and `icon` is the image showing under

spec/api-browser-window-spec.js

@@ -1663,35 +1663,33 @@ describe('BrowserWindow module', function () {
   })
 
   describe('dev tool extensions', function () {
-    let showPanelIntervalId
+    let showPanelTimeoutId
 
     const showLastDevToolsPanel = () => {
       w.webContents.once('devtools-opened', function () {
-        clearInterval(showPanelIntervalId)
-
-        showPanelIntervalId = setInterval(function () {
+        const show = function () {
           if (w == null || w.isDestroyed()) {
-            clearInterval(showLastDevToolsPanel)
             return
           }
-
           const {devToolsWebContents} = w
           if (devToolsWebContents == null || devToolsWebContents.isDestroyed()) {
-            clearInterval(showLastDevToolsPanel)
             return
           }
 
-          var showLastPanel = function () {
-            var lastPanelId = WebInspector.inspectorView._tabbedPane._tabs.peekLast().id
+          const showLastPanel = function () {
+            const lastPanelId = WebInspector.inspectorView._tabbedPane._tabs.peekLast().id
             WebInspector.inspectorView.showPanel(lastPanelId)
           }
-          devToolsWebContents.executeJavaScript(`(${showLastPanel})()`)
-        }, 100)
+          devToolsWebContents.executeJavaScript(`(${showLastPanel})()`, false, () => {
+            showPanelTimeoutId = setTimeout(show, 100)
+          })
+        }
+        showPanelTimeoutId = setTimeout(show, 100)
       })
     }
 
     afterEach(function () {
-      clearInterval(showPanelIntervalId)
+      clearTimeout(showPanelTimeoutId)
     })
 
     describe('BrowserWindow.addDevToolsExtension', function () {

spec/api-web-contents-spec.js

@@ -285,4 +285,22 @@ describe('webContents module', function () {
     })
     w.webContents.inspectElement(10, 10)
   })
+
+  describe('startDrag({file, icon})', () => {
+    it('throws errors for a missing file or a missing/empty icon', () => {
+      assert.throws(() => {
+        w.webContents.startDrag({icon: path.join(__dirname, 'fixtures', 'assets', 'logo.png')})
+      }, /Must specify either 'file' or 'files' option/)
+
+      assert.throws(() => {
+        w.webContents.startDrag({file: __filename})
+      }, /Must specify 'icon' option/)
+
+      if (process.platform === 'darwin') {
+        assert.throws(() => {
+          w.webContents.startDrag({file: __filename, icon: __filename})
+        }, /Must specify non-empty 'icon' option/)
+      }
+    })
+  })
 })