If the app.quit() call is removed from removeWindow(), then there are background process left not-cleaned-up after the last window is closed on Windows. I have to put that back in. (Unless there is some sneaky more-elegant solution hiding somewhere or that I haven't thought of, suggestions welcome. Have debugger, will test.)

An observation: Most of these event handlers seem to expect to be called more than once. The 'close' and 'closed' handlers can run many times with no issue. The 'before-quit' almost doesn't seem to work fully if not called multiple times. And I've had it called multiple times in a lot (all?) of my non-buggy quits that I've inspected and stepped through.

Not to get into too broad a discussion, but: I wish these handlers were re-written to be simpler, less interdependent on one-another, and most especially I would like to see them reliably work when run only once each.

Here's what a successful quit looks like on Windows, according to my debugging:

'close' event handler:
  event.preventDefault()
  this.unloading = true
  saveCurrentWindowOptions()

 // [async/unrelated]
 blurHandler() // 'blur' event emitted because Atom is unfocused while I'm looking at the debugger
   saveCurrentWindowOptions() // This function runs a lot!
 // [end async/unrelated]

// still in the 'close' event handler:
  browserWindow.close() // Why do this?? Is this how we communicate with Chromium?
    'close' event handler // Nested/self-call.. is this necessary?? No-op in our code because this.unloading === true

'window-all-closed' event handler:
  app.quit()
    'before-quit' event handler:
      event.preventDefault() // I think Atom forgets to actually explicitly quit the first time around, which the default behavior is meant to do?

'closed' event handler:
  removeWindow()
    app.quit()
      'before-quit' event handler // event.preventDefault() is skipped because this.quitting === true, so I guess we actually quit now... But that's just a guess? Perhaps that's why the 'will-quit' event is emitted this time?
      'will-quit' event handler:
        this.killAllProcesses() // Or is this when we actually quit? Perhaps this is only triggered when event.preventdefault() is skipped in the 'before-quit' handler?

This is what happens when pressing the red "x" button on the last open window on Windows.

The original close is cancelled and cleanup is done. The second call to close is done to resume the close operation, which shouldn't be prevented the second time, hence the close handler being a no-op. This seems reasonable to me; the idea is to reschedule the close to after some potentially asynchronous work is finished.

Atom doesn't exactly forget to quit the first time, it's just waiting for a response from a closed window (if I recall my debugging correctly). But while waiting for the window promises to resolve Atom is left in a state that really shouldn't be allowed (with quitting true, so subsequent quits will actually do a quit, but with quitting work still being performed).

The above problem is exactly what happens during the removeWindow quit. As the before-quit handler does not cancel the event this time, the quit progresses and the will-quit handler is called after all windows have been closed.

I've been looking at the Electron source code that handles all this because I suspect the original issue is caused by Electron itself getting into an invalid quitting state. That's my only hypothesis for why app.quit() can run, but the before-quit handler doesn't.

Thank you, this is definitely helpful to have laid out in clearer terms. And the more I've looked at this I've tended closer to seeing it how you described.

The pattern I'm seeing could be summarized shortly like this: "disable default handling" --> "custom work" --> "Re-enable default handling, or finish action in custom Atom code" (That last bit is different depending on which handler or function).

I've been looking at the Electron source code that handles all this because I suspect the original issue is caused by Electron itself getting into an invalid quitting state.

Yes, past a certain point you can attempt to quit from the app any number of times and any number of ways and nothing happens. It's hard for me to fathom how Atom's code alone could cause this. And the JS debugger becomes nearly useless to track what's going on, if anything, so I suspect native code is involved at or near the point of failure... (Though I can't say for sure with the experience and information I have with this).

If it's an electron bug, I wonder whether or not it's been fixed in a newer version.

[On the subject of this pull request, this pull request is still needed, so that Atom uses the Electron API correctly, so as to match Atom's intended behavior. But if there is a tangentially related bug in upstream Electron, I do support any effort to eventually find it and fix it. It's just that this discussion is not 100% on-topic. I want to clarify that none of the unresolved parts of this discussion are meant to be addressed in this PR, and if they ever do get resolved, they probably won't be addressed directly in Atom, but rather in Electron.]

@Aerijo I would like it if Atom's 'before-quit' handler ran only once, and simply emitted the 'will-quit' event when it thought it was done.

That's the kind of cleanup and simplification I would love to see. No need to re-enable the default handling for 'before-quit'. No need to run through any 'before-quit' handler more than once.

Or, as an alternative, try not to manually handle an event at all if the built-in default behavior is adequate.

It should all be flattened out so most event handlers don't need to run more than once in Atom's code, if at all. (My opinion).

I don't want to take up any more of your time and energy. It was a deliberately provocative proposal, but I thought it was clear I didn't know how to implement it. I'm okay with that. Especially while I slowly learn more. It's too big a problem for me to tackle yet, I just want to have a goal in mind to use to help me get better with Electron and Atom. If I don't achieve what I set out to do, hopefully something adjacent to it will be improved by the time I'm done.

I won't properly understand why this is impossible at this time, because I'm that new to Electron. Intending to learn as I go. What you've mentioned here is likely true, I just need more context that'll take me a while to come by. I'll get there eventually but not this exact moment.

It's fine, I'm only looking at this stuff in depth for the first time myself.

The BrowserWindow is still part of Electron, the only difference in the linked examples being it used on the renderer process (I assume the remote module provides a mock of the BrowserWindow that will send all requests to the main process). But using it that way has issues.

@DeeDeeG You might like this for a flattened overview of the relevant steps Electron is taking under the hood. The regular boxes mostly correspond to functions, which can be searched for in the Electron code base. Each emit is synchronous. Could be useful for hypothesising and testing the cause of the original issue too.

That's interesting. (I assume based on the notes in the yellow boxes this is your work and made for debugging this issue?)

[Edit to add: What do the different shapes mean on this diagram? [Edit 2: This I suppose: https://www.rff.com/flowchart_shapes.php ] I don't immediately understand some of the symbols either, like > before a line of text in the label.]

I believe we (Atom) will be in a newer version of Electron soon, so before hypothesizing too much about the Electron side of it, I'd be interested to see if Atom on Electron 9 (with this fix reverted) still exhibits the issue.

Random question if you get a moment: Do you happen to have an SSD? I had reproduced the issue with an HDD.

It is an SSD.

It's possible it's been fixed in Electron. I haven't seen many changes in the codebase for this area (though they've since renamed a lot of files making blames harder to follow), but it's also useful to me for just getting a sense of Electron itself so I'm not too concerned even if it has been patched.

The > is just how I notated "function is called". Those boxes aren't required to understand the event order, but help relating it back to the Electron source code.

• The ovals are code entry points,
• the deformed boxes are user actions / JS entry points,
• the barred boxes are references to other entry points in the diagram,
• diamonds are choices (outgoing arrows answer question / boolean in diamond),
• the rounded edged boxes represent emitting a JS event, and
• the wavy boxes are things I don't fully understand yet.