🍎 Don't quit upon 'window-all-closed' event on macOS #21847

DeeDeeG · 2021-01-04T07:07:31Z

This prevents a lot of buggy partial-quitting stuff from happening.

Requirements for Contributing a Bug Fix (from template, click to expand)

Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
The pull request must only fix an existing bug. To contribute other changes, you must use a different template. You can see all templates at https://github.com/atom/.github/tree/master/.github/PULL_REQUEST_TEMPLATE.
The pull request must update the test suite to demonstrate the changed functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/.github/tree/master/CONTRIBUTING.md#pull-requests.

Identify the Bug

Fixes: #17672
Fixes: #20831

Description of the Change

In order to avoid a buggy, "half-quit" state, don't attempt to quit on macOS when the last window is closed.

More technically: listen for and handle the 'window-all-closed' event in a custom way, overriding the default behavior Electron provides. (Electron's default behavior for this event simply calls app.quit(), regardless of OS. We can do essentially the same, but conditionally not quit if the OS is 'darwin' meaning macOS.)

'window-all-closed' event documentation at electronjs.org

In this way, we can allow Atom to stay running in the background ready to open new windows quickly, as it is intended to.

(For reasons that aren't entirely clear to me, Atom's code has been partly preventing this quitting, but still setting an atomApplication.quitting variable to true, which later on causes some buggy behavior around quitting and closing windows. This pull request addresses and prevents the bugginess.)

Alternate Designs

Remove the "Atom process in the background" feature altogether, to avoid these bugs: Fix Atom not quitting on MacOS #21843
- Having Atom run in the background, ready to open new windows quickly, is a desirable feature. So we should fix the underlying issue rather than disable the feature.
Introduce some way of force quitting from within the app, that is call app.exit()?
- Not necessary now that Atom behaves itself and quits properly when asked to. Most of the time. 😛

Possible Drawbacks

None.

Verification Process

I built Atom locally with this change. I confirmed that the steps to reproduce identified in #17672 (comment) are no-longer reproducible.

CI passes.

Release Notes

Fix buggy quitting behavior on macOS after all windows are closed

This prevents a lot of buggy partial-quitting stuff from hapening. Fixes: atom#17672 Fixes: atom#20831

Aerijo · 2021-01-04T08:37:35Z

I would put this in atom-application with the other lifetime event handlers. But otherwise seems fine. I was worried it would lead to a double quit on non-macOS, but then realised that the behaviour would be no different to the original default handler, so this PR wouldn't be introducing any issues relating to that.

It would be better to be consistent with the existing check, so picking one of ['win32', 'linux'].includes(process.platform) or process.platform !== 'darwin' and make both do it. This is mostly a style change, as I don't imagine Atom will ever support another OS. The latter is technically better if it were to though, as it's safer to close the app when all windows are gone if the behaviour gets overlooked.

Co-authored-by: Benjamin Gray <[email protected]>

DeeDeeG · 2021-01-04T11:06:46Z

Thanks for the review.

I would put this in atom-application with the other lifetime event handlers.

I can do that. This appears to work just as well to fix the bug: DeeDeeG@af1daa9

Can you give some thoughts as to why that's better? I'm not intimately familiar with these files, and would like to know what the benefit is.

It would be better to be consistent with the existing check, so picking one of ['win32', 'linux'].includes(process.platform) or process.platform !== 'darwin' and make both do it.

Hmm. I don't know which is faster, but since these are done once in a blue moon each, and they aren't in loops, it likely doesn't matter, they're both blazingly fast, I'd imagine. I think that !== 'darwin' seems more honest than ['win32', 'linux'].includes(). If Atom got ported to BSD, for example, the code that runs there tends to be more Linux-ey than macOS-ey. And other things are probably more Windows-ey than macOS-ey. (macOS is rather proprietary and secretive, so it's less likely people will attempt a compatible or closely-related piece of software that doesn't get to run on actual macOS...) If there is an actual fourth OS supported by Atom, they surely will look at these sorts of checks as a first order of business when porting... So whoever tries to port this to a fourth OS, I think this is ultimately their problem to sort out. Not that we can't try to leave things in good order.

I think I'd like a second opinion on the need for updating all of those, as smaller diffs are usually my preference. I'd be a tiny bit sad to go from !== 'darwin' to ['win32', 'linux'].includes() for this little listener, but I don't have any technical reason, just aesthetic preference mostly.

The latter [!== 'darwin'] is technically better if [Atom] were to [support another OS] though, as it's safer to close the app when all windows are gone if the behaviour gets overlooked.

Hmm, makes sense. 👍

Overall: Going to think this over and/or wait for some more feedback before I go ahead and add either of these changes to the PR.

DeeDeeG · 2021-01-04T11:15:13Z

#17672 (comment)
@DeeDeeG You can try moving the other 0 windows logic to that handler too, like this.

It would be nice to take a look at this suggestion again before this PR got merged, if possible, but I can't really properly test this right at the moment. Leaving this as a note-to-self to try it out, and for others to comment on if they have any feedback on that suggestion.

Aerijo · 2021-01-04T13:50:00Z

This appears to work just as well to fix the bug

It does, I just think it's better to have similar code together. The atom-application file already has the other 0 window handling logic, as well as the quit and most of the other app event handlers.

I've only looked at these files for the first time myself mind.

If there is an actual fourth OS supported by Atom, they surely will look at these sorts of checks as a first order of business when porting

Maybe, or they might just try to build it and fix errors as they come, like with the electron upgrades. This wouldn't error at build time, and isn't tested, so it would be easily missed. Speed has nothing to do with it (as you point out).

I think I'd like a second opinion on the need for updating all of those

I only meant this other instance. It just seems weird to me to be using two different ways of checking the same condition in the same file (assuming it's moved).

These were just small things though, I'm happy with the change itself and believe it would work as is without adding regressions.

DeeDeeG · 2021-01-04T16:21:58Z

I think I'll just test your commit some more (when I get a good moment to!) -- It's a pretty clean-looking change and tidies up the now-redundant code a bit. Seems like nothing wrong with that.

I'm mildly nervous at changing the exact timing of things, but as I'm looking at this code, it doesn't feel like every bit of it has been cautiously audited for race conditions every step of the way, to be quite honest. So the status quo may or may not be something to look to as an example. (So... testing is the way!)

DeeDeeG · 2021-01-06T15:58:47Z

~~TODO:~~

Test on Windows and/or Linux to confirm quitting works as intended there.
Decide exactly which tweaks/cleanups to do or not do. Testing a few variations on this and inspecting/debugging.

[Update: Done.]

Co-authored-by: Benjamin Gray <[email protected]>

DeeDeeG · 2021-01-06T18:27:10Z

src/main-process/atom-application.js

-      if (['win32', 'linux'].includes(process.platform)) {
-        app.quit();
-        return;
-      }


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.

This is needed to ensure Atom quits completely and cleanly on Windows (and Linux?).

DeeDeeG · 2021-01-06T19:57:44Z

Yeah, it seems to me that the 'before-quit' event handler doesn't actually make Atom quit unless it runs twice.

That comes from running app.quit() (at least) two times.

On Windows and Linux, that includes the one from the 'window-all-closed' event handler and another one in removeWindow().
On macOS, that includes the nested app.quit() call relatively deep inside some conditions and behind some awaits in the 'before-quit' handler itself!
application:quit also calls app.quit(), so... very frequently it's called three times. I'm seeing Linux call app.quit() four times in one shutdown!

Opinion: I would love if this were re-coded to be simpler...

Taking a step back to summarize: The way things are coded in Atom right now, the first app.quit() really just emits the before-quit event, which is intercepted the first go around, and then a lot of other stuff gets done (closing windows, saving state) before Atom makes itself really quit.

DeeDeeG · 2021-01-07T00:11:24Z

@sadick254 Please consider reviewing/merging this PR. More details below:

There has been an issue for a long time on macOS where closing the last window puts Atom in a "half-quit" state. (In this state, opening and closing another window instantly quits Atom, and opening a new project then attempting to quit causes Atom to become un-quitable unless resorting to some sort of "Force Quit" action. Either of these outcomes is unexpected and undesirable for Atom users on macOS.)

This Pull Request solves the underlying problem: Atom did not handle the 'window-all-closed' event, so the default behavior from Electron was used. The default behavior from Electron is to quit, regardless of OS. (This is not intended behavior for Atom on macOS; Atom should stay open in the background, even with no windows up, unless and until it is Quit.)

(Technical detail: According to my testing, Atom must run its 'before-quit'event handler at least twice in order to properly quit. This bug was caused by activating the 'before-quit' event and the associated handler only once.)

I have tested this Pull Request on macOS, Windows and Linux. On all three operating systems, I was able to open and close windows as expected, without running into the bug on macOS. I can quit with multiple windows open and restore them. And so on. Generally everything is working. CI also passes.

So if you or another Atom maintainer can take a look, it would be much appreciated.

Thank you.

- DeeDeeG

Aerijo

LGTM @sadick254

sadick254 · 2021-01-07T04:36:13Z

@DeeDeeG This is on my radar. It is a small change and I will review it ASAP.

rseichter · 2021-01-07T11:46:39Z

I'm very much looking forward to this fix becoming available as part of an official Atom release. My thanks to @DeeDeeG and all others involved.

* 🍎 Don't quit on 'window-all-closed' on macOS This prevents a lot of buggy partial-quitting stuff from happening. Fixes: atom#17672 Fixes: atom#20831 (cherry picked from commit 72709dc) Co-authored-by: Benjamin Gray <[email protected]>

* 🍎 Don't quit upon 'window-all-closed' event on macOS (#21847) * 🍎 Don't quit on 'window-all-closed' on macOS This prevents a lot of buggy partial-quitting stuff from happening. Fixes: #17672 Fixes: #20831 (cherry picked from commit 72709dc) Co-authored-by: Benjamin Gray <[email protected]> * Fix buggy partial startup on macOS due to flag added by Gatekeeper/XProtect (#21861) macOS Gatekeeper adds a flag ("-psn_0_[six or seven digits here]") when it intercepts Atom launches. This happens for fresh downloads, new installs, or first launches after upgrading). We don't need this flag, and yargs interprets it as many short flags. So, we filter it out. (cherry picked from commit 7928a1b) Co-authored-by: Benjamin Gray <[email protected]>

jen801

#10967

🍎 Don't quit on 'window-all-closed' on macOS

fda3c27

This prevents a lot of buggy partial-quitting stuff from hapening. Fixes: atom#17672 Fixes: atom#20831

DeeDeeG changed the title ~~🍎 Don't quit on 'window-all-closed' on macOS~~ 🍎 Don't quit on 'window-all-closed' on macOS Jan 4, 2021

DeeDeeG mentioned this pull request Jan 4, 2021

Fix Atom not quitting on MacOS #21843

Closed

DeeDeeG changed the title ~~🍎 Don't quit on 'window-all-closed' on macOS~~ 🍎 Don't quit upon 'window-all-closed' event on macOS Jan 4, 2021

Move 'window-all-closed' handler to atom-application.js

af1daa9

Co-authored-by: Benjamin Gray <[email protected]>

DeeDeeG marked this pull request as draft January 6, 2021 15:57

Tweak window-all-closed and removeWindow behavior

3780a02

Co-authored-by: Benjamin Gray <[email protected]>

DeeDeeG commented Jan 6, 2021

View reviewed changes

Restore app.quit() in removeWindow()

a152699

This is needed to ensure Atom quits completely and cleanly on Windows (and Linux?).

Adjust comments for 'window-all-closed' handler

4696032

DeeDeeG marked this pull request as ready for review January 7, 2021 00:01

Aerijo approved these changes Jan 7, 2021

View reviewed changes

ThatXliner mentioned this pull request Jan 7, 2021

Atom does not open from clicking a file when Atom is already closed #21826

Open

sadick254 merged commit 72709dc into atom:master Jan 7, 2021

This was referenced Jan 7, 2021

Update flow on mac poor ux #16385

Open

Atom quitting when closing window (macOS) #21143

Open

Atom not closing totally even after doing cmd+q or Quit atom from menu #21823

Open

Electron 9.4.1 #21815

Merged

ThatXliner mentioned this pull request Feb 1, 2021

Language Ruby #21915

Closed

rseichter mentioned this pull request Feb 7, 2021

Atom does not quit on OS X #17672

Closed

DeeDeeG mentioned this pull request Feb 10, 2021

Backport macOS startup and shutdown fixes to Atom 1.55 #21948

Merged

jen801 reviewed Jun 6, 2021

View reviewed changes

🍎 Don't quit upon 'window-all-closed' event on macOS #21847

🍎 Don't quit upon 'window-all-closed' event on macOS #21847

Uh oh!

Conversation

DeeDeeG commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Identify the Bug

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Release Notes

Uh oh!

Aerijo commented Jan 4, 2021

Uh oh!

DeeDeeG commented Jan 4, 2021

Uh oh!

DeeDeeG commented Jan 4, 2021

Uh oh!

Aerijo commented Jan 4, 2021

Uh oh!

DeeDeeG commented Jan 4, 2021

Uh oh!

DeeDeeG commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeeDeeG Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aerijo Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aerijo Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

Aerijo Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

DeeDeeG Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aerijo Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

DeeDeeG commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeeDeeG commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aerijo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadick254 commented Jan 7, 2021

Uh oh!

rseichter commented Jan 7, 2021

Uh oh!

jen801 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

DeeDeeG commented Jan 4, 2021 •

edited

Loading

DeeDeeG commented Jan 6, 2021 •

edited

Loading

DeeDeeG Jan 6, 2021 •

edited

Loading

DeeDeeG Jan 6, 2021 •

edited

Loading

DeeDeeG Jan 7, 2021 •

edited

Loading

DeeDeeG Jan 7, 2021 •

edited

Loading

DeeDeeG Jan 7, 2021 •

edited

Loading

DeeDeeG Jan 8, 2021 •

edited

Loading

DeeDeeG commented Jan 6, 2021 •

edited

Loading

DeeDeeG commented Jan 7, 2021 •

edited

Loading

Aerijo left a comment •

edited

Loading