Browse Source

fix: destroy tray on current tick (#18196) (#19619)

This code was originally added in #6448 to handle an edge case crash in 10.9, and we no longer support 10.9 and therefore no longer need to account for this case.

It addressed the crash, but also created a race condition whereby when a new tray is created the old tray's destroy wouldn't have been fully completed and therefore a new one would be spawned. This fixes that by destroying the tray on the current tick once more.
Shelley Vohr 5 years ago
parent
commit
fc94fb4480
2 changed files with 34 additions and 13 deletions
  1. 1 5
      atom/browser/api/atom_api_tray.cc
  2. 33 8
      spec/api-tray-spec.js

+ 1 - 5
atom/browser/api/atom_api_tray.cc

@@ -59,11 +59,7 @@ Tray::Tray(v8::Isolate* isolate,
   InitWith(isolate, wrapper);
 }
 
-Tray::~Tray() {
-  // Destroy the native tray in next tick.
-  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,
-                                                  tray_icon_.release());
-}
+Tray::~Tray() = default;
 
 // static
 mate::WrappableBase* Tray::New(mate::Handle<NativeImage> image,

+ 33 - 8
spec/api-tray-spec.js

@@ -1,4 +1,5 @@
 const { remote } = require('electron')
+const assert = require('assert')
 const { Menu, Tray, nativeImage } = remote
 
 describe('tray module', () => {
@@ -8,12 +9,12 @@ describe('tray module', () => {
     tray = new Tray(nativeImage.createEmpty())
   })
 
-  afterEach(() => {
-    tray.destroy()
-    tray = null
-  })
-
   describe('tray.setContextMenu', () => {
+    afterEach(() => {
+      tray.destroy()
+      tray = null
+    })
+
     it('accepts menu instance', () => {
       tray.setContextMenu(new Menu())
     })
@@ -23,7 +24,22 @@ describe('tray module', () => {
     })
   })
 
+  describe('tray.destroy()', () => {
+    it('destroys a tray', () => {
+      assert.strictEqual(tray.isDestroyed(), false)
+      tray.destroy()
+
+      assert.strictEqual(tray.isDestroyed(), true)
+      tray = null
+    })
+  })
+
   describe('tray.popUpContextMenu', () => {
+    afterEach(() => {
+      tray.destroy()
+      tray = null
+    })
+
     before(function () {
       if (process.platform !== 'win32') {
         this.skip()
@@ -43,22 +59,31 @@ describe('tray module', () => {
   describe('tray.setImage', () => {
     it('accepts empty image', () => {
       tray.setImage(nativeImage.createEmpty())
+
+      tray.destroy()
+      tray = null
     })
   })
 
   describe('tray.setPressedImage', () => {
     it('accepts empty image', () => {
       tray.setPressedImage(nativeImage.createEmpty())
+
+      tray.destroy()
+      tray = null
     })
   })
 
   describe('tray.setTitle', () => {
+    before(function () {
+      if (process.platform !== 'darwin') this.skip()
+    })
+
     it('accepts non-empty string', () => {
       tray.setTitle('Hello World!')
-    })
 
-    it('accepts empty string', () => {
-      tray.setTitle('')
+      tray.destroy()
+      tray = null
     })
   })
 })