New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change macos app termination process #11625

Merged
merged 3 commits into from Jan 17, 2018

Conversation

Projects
None yet
5 participants
@nitsakh
Copy link
Contributor

nitsakh commented Jan 12, 2018

#230 happens because applicationShouldTerminate returns NSTerminateCancel. One of the ways to fix that would have been returning NSTerminateLater as was suggested in one of the responses the #230. However, the app will switch to NSModalPanelRunLoopMode and will wait for replyToApplicationShouldTerminate being called.
The chrome message loop however, does not end and there's possibility of abruptly terminating the app, causing loss of data.
The approach taken in this PR, is by implementing application terminate instead of applicationShouldTerminate delegate method. Terminate will try to close the browser windows, and chrome message loop will proceed to termination. If browser windows are not closed (user cancelled through the popup), then termination will halt and the app will keep running. [Chrome seems to follow a similar approach.]

@nitsakh nitsakh requested a review from electron/reviewers as a code owner Jan 12, 2018

@nitsakh nitsakh requested a review from zcbenz Jan 12, 2018

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Jan 12, 2018

@nitsakh looks like a whitespace linter error is causing most of the failures

@codebytere
Copy link
Member

codebytere left a comment

just two quick observations :)

@@ -4,6 +4,7 @@

#import "atom/browser/mac/atom_application.h"

#import "atom/browser/mac/atom_application_delegate.h"

This comment has been minimized.

@codebytere

codebytere Jan 12, 2018

Member

for consistency this should be an #include

This comment has been minimized.

@nitsakh

nitsakh Jan 12, 2018

Author Contributor

Shouldn't mac includes be #import? The one above it is also #import.
I might have missed any guidelines though, will check. Thanks.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 13, 2018

Member

In general we #include, the #import above and the other still hanging around should be replaced unless they point to mac libraries like <AppKit/AppKit.h> afaik

This comment has been minimized.

@zcbenz

zcbenz Jan 17, 2018

Member

atom_application_delegate.h is an Objective-C++ only header without header guards, and we should only use #include when the header has header guards, otherwise recursive includes may happen.

We should probably add header guards to all headers and use #include for consistency, but let's do it in another PR.

@@ -157,6 +157,24 @@ describe('app module', () => {
done()
})
})

it('exits gracefully on macos', (done) => {
if (process.platform !== 'darwin') {

This comment has been minimized.

@codebytere

codebytere Jan 12, 2018

Member

this arrow function should be changed to function(done) otherwise the implicit this will prevent the this.skip() below from working properly

@nitsakh nitsakh force-pushed the nitsakh:mac-terminate branch 2 times, most recently from 6e1145e to b4d7880 Jan 12, 2018


it('exits gracefully on macos', function (done) {
if (process.platform !== 'darwin') {
this.skip()

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 13, 2018

Member

In general we don't call skip on platform specific tests, we just return and let the test "pass"

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 15, 2018

Contributor

@MarshallOfSound Actually we do the opposite )
If a test isn't run, it should we marked as skipped, not passed.
In rare cases, when we have issues with Mocha, we do call a return instead of this.skip().

ChildProcess.exec('osascript -e \'quit app "Electron"\'', (err, stdout, stderr) => {
assert(!err)
assert(!stderr.trim())
done()

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 13, 2018

Member

Should we also assert that the spawned electron app exits with 0?

This comment has been minimized.

@zcbenz

zcbenz Jan 17, 2018

Member

Asserting err should have guarded the exit code.

// Termination can proceed if all windows are closed or window close can be cancelled
// which will abort termination.
- (void)tryToTerminateApp:(NSApplication*)app {
atom::Browser::Get()->Quit();

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jan 13, 2018

Member

It's possible this request could be received while the app is already quitting, we should probably still check that browser->is_quitting() is false

This comment has been minimized.

@zcbenz

zcbenz Jan 17, 2018

Member

Browser::Quit already does the check itself.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Jan 17, 2018

I'm rebasing this on master to fix the CI failure.

@zcbenz zcbenz force-pushed the nitsakh:mac-terminate branch from b4d7880 to ea2056b Jan 17, 2018

@zcbenz

zcbenz approved these changes Jan 17, 2018

Copy link
Member

zcbenz left a comment

Tested this for a few cases, it works like a charm!

@zcbenz zcbenz merged commit 4dab824 into electron:master Jan 17, 2018

7 of 8 checks passed

ci/circleci: electron-linux-ia32 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@nitsakh nitsakh deleted the nitsakh:mac-terminate branch Jan 18, 2018

juturu pushed a commit that referenced this pull request May 21, 2018

Change macos app termination process
Port of #11625
without changes in test files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment