fix(before-unload): When drafts are present, we do not resume quit properly after committing changes

Summary:
We designed the onBeforeUnload handler to resume a "close window" event, but it actually needs to resume a quit as well. This has become an issue recently and may be due to the DraftStore sessions not being removed from the DraftStore.

To test: Create a popout draft, close it, and then try to quit the app.

Test Plan: Run updated tests

Reviewers: dillon, evan

Reviewed By: dillon, evan

Differential Revision: https://phab.nylas.com/D1932

spec-nylas/stores/draft-store-spec.coffee

@@ -529,12 +529,12 @@ describe "DraftStore", ->
         }}
 
       it "should return false and call window.close itself", ->
-        spyOn(DraftStore, '_onBeforeUnloadComplete')
+        spyOn(atom, 'finishUnload')
         expect(DraftStore._onBeforeUnload()).toBe(false)
-        expect(DraftStore._onBeforeUnloadComplete).not.toHaveBeenCalled()
+        expect(atom.finishUnload).not.toHaveBeenCalled()
         @resolve()
         advanceClock(1000)
-        expect(DraftStore._onBeforeUnloadComplete).toHaveBeenCalled()
+        expect(atom.finishUnload).toHaveBeenCalled()
 
     describe "when drafts return immediately fulfilled commit promises", ->
       beforeEach ->
@@ -546,11 +546,11 @@ describe "DraftStore", ->
         }}
 
       it "should still wait one tick before firing atom.close again", ->
-        spyOn(DraftStore, '_onBeforeUnloadComplete')
+        spyOn(atom, 'finishUnload')
         expect(DraftStore._onBeforeUnload()).toBe(false)
-        expect(DraftStore._onBeforeUnloadComplete).not.toHaveBeenCalled()
+        expect(atom.finishUnload).not.toHaveBeenCalled()
         advanceClock()
-        expect(DraftStore._onBeforeUnloadComplete).toHaveBeenCalled()
+        expect(atom.finishUnload).toHaveBeenCalled()
 
     describe "when there are no drafts", ->
       beforeEach ->

src/atom.coffee

@@ -341,7 +341,7 @@ class Atom extends Model
   # content trace visualizer (chrome://tracing). It's like Chromium Developer
   # Tools Profiler, but for all processes and threads.
   trace: ->
-    tracing = require('remote').require('content-tracing')
+    tracing = remote.require('content-tracing')
     tracing.startRecording '*', 'record-until-full,enable-sampling,enable-systrace', ->
       console.log('Tracing started')
       setTimeout ->
@@ -588,7 +588,7 @@ class Atom extends Model
     if @isValidDimensions(dimensions)
       dimensions
     else
-      screen = remote.require 'screen'
+      screen = remote.require('screen')
       {width, height} = screen.getPrimaryDisplay().workAreaSize
       {x: 0, y: 0, width, height}
 
@@ -881,8 +881,13 @@ class Atom extends Model
     ipc.send('call-window-method', 'setAutoHideMenuBar', autoHide)
     ipc.send('call-window-method', 'setMenuBarVisibility', !autoHide)
 
-  # Lets multiple components register callbacks.
-  # The callbacks are expected to return either true or false
+  # Lets multiple components register beforeUnload callbacks.
+  # The callbacks are expected to return either true or false.
+  #
+  # Note: If you return false to cancel the window close, you /must/ perform
+  # work and then call finishUnload. We do not support cancelling quit!
+  # https://phab.nylas.com/D1932#inline-11722
+  #
   onBeforeUnload: (callback) -> @_unloadCallbacks.push(callback)
 
   _unloading: ->
@@ -896,3 +901,13 @@ class Atom extends Model
       else
         console.warn "You registered an `onBeforeUnload` callback that does not return either exactly `true` or `false`. It returned #{returnValue}", callback
     return continueUnload
+
+  # Call this method to resume the close / quit process if you returned
+  # false from a onBeforeUnload handler.
+  #
+  finishUnload: ->
+    _.defer =>
+      if remote.getGlobal('application').quitting
+        remote.quit()
+      else
+        @close()

src/browser/application.coffee

@@ -268,11 +268,11 @@ class Application
 
     # Called before the app tries to close any windows.
     app.on 'before-quit', =>
+      # Allow the main window to be closed.
+      @quitting = true
       # Destroy hot windows so that they can't block the app from quitting.
       # (Electron will wait for them to finish loading before quitting.)
       @windowManager.unregisterAllHotWindows()
-      # Allow the main window to be closed.
-      @quitting = true
 
     # Called after the app has closed all windows.
     app.on 'will-quit', =>
@@ -330,7 +330,7 @@ class Application
 
     ipc.on 'action-bridge-rebroadcast-to-main', (event, args...) =>
       mainWindow = @windowManager.mainWindow()
-      return if not mainWindow
+      return if not mainWindow or not mainWindow.browserWindow.webContents
       return if BrowserWindow.fromWebContents(event.sender) is mainWindow
       mainWindow.browserWindow.webContents.send('action-bridge-message', args...)
 

src/flux/stores/draft-store.coffee

@@ -166,7 +166,7 @@ class DraftStore
       # handler, so we need to always defer by one tick before re-firing close.
       Promise.settle(promises).then =>
         @_draftSessions = {}
-        @_onBeforeUnloadComplete()
+        atom.finishUnload()
 
       # Stop and wait before closing
       return false
@@ -174,10 +174,6 @@ class DraftStore
       # Continue closing
       return true
 
-  # For better specs
-  _onBeforeUnloadComplete: =>
-    _.defer -> atom.close()
-
   _onDataChanged: (change) =>
     return unless change.objectClass is Message.name
     containsDraft = _.some(change.objects, (msg) -> msg.draft)