Theia: [electron] Backend processes are already terminated before 'will-prevent-unload' is called

[kittaakos]

Bug Description:

Start the app, make an editor dirty or set the application.confirmExit to true. Terminate the app with Ctrl/Cmd+Q on macOS, you get the Any unsaved changes will not be saved. dialog, but the BE has already terminated so hitting No in the dialog will result in an offline app.

We need to figure out the followings:

• ~is it a macOS issue only?~ It is not a macOS only problem. Same bug on Windows when quitting the app with Alt+F4. See here.
• ~was there a change in electron 9.x?~ It is not related to electron 9.x. See here
• is it related? ~Maybe; see here.~ Most likely not: https://github.com/eclipse-theia/theia/issues/8186#issuecomment-659415028
• should we switch back to this?

Mentioning @marechal-p, @mcgordonite here. If you have any idea what's causing it, please let me know.

Steps to Reproduce:

1.
2.
3.

Additional Information

• Operating System:
• Theia Version:

[kittaakos]

• was there a change in electron 9.x?

I can confirm, it is not an electron 9.x regression. The app has the same issue. If I terminate the app with Cmd+q on macOS, and hit No, I have an offline app. Steps to reproduce:

git checkout a0e635aedf5e670ac2580dc7277164dc067c63ac -b a0e635aedf5e670ac2580dc7277164dc067c63ac \
&& yarn \
&& yarn rebuild:browser \
&& yarn rebuild:electron \
&& yarn --cwd ./example/electron start

[kittaakos]

• is it a macOS issue only?

Confirm, it is not a macOS issue. I can see an offline app on Windows too.

[kittaakos]

• is it related?

This seems to be related. So I did the following to verify. Went back to the parent commit of #6285 and tried that version. It worked. Steps:

git checkout fcccdf44dc23a31d1bab63c330df75a7aeb33a0c -b fcccdf44dc23a31d1bab63c330df75a7aeb33a0c

The prepare-travis script is not backward compatible with latest yarn, so you either "disable" the script:

diff --git a/package.json b/package.json
index f31d68165..1a1b610d1 100644
--- a/package.json
+++ b/package.json
@@ -48,7 +48,7 @@
   "scripts": {
     "postinstall": "node scripts/post-install.js",
     "prepare": "node scripts/skip-prepare || ( yarn prepare:travis && yarn rebuild:clean && yarn build:clean && yarn prepare:hoisting )",
-    "prepare:travis": "node scripts/prepare-travis",
+    "prepare:travis": "echo foo",
     "prepare:hoisting": "theia check:hoisted -s",
     "preinstall": "node-gyp install",
     "build": "run build",

Or you downgrade your yarn.
Then build and start the app:

yarn \
&& yarn rebuild:browser \
&& yarn rebuild:electron \
&& yarn --cwd ./examples/electron start

Can someone else also verify it? @vince-fugnitto, @lmcbout, do you happen to have time to check it? I think we have to handle both cases when we close the browser window and it unloads, and when we quit the app with Cmd+Q on macOS and Alt+F4 on Windows.

@mcgordonite, do you have time to figure out a solution for all platforms together? I can take care of the macOS part and can verify it on Windows.

[vince-fugnitto]

Can someone else also verify it? @vince-fugnitto

@kittaakos I verified that the bug occurs on Linux as well (escaping the dialog results in an offline application), do you want me to perform any additional checks?

[kittaakos]

do you want me to perform any additional checks?

No, thank you! It is just a guess, but I think we need to cover two different cases:

• when closing the app by gently closing the last window (with the X),
• and quitting the app.

Back when I verified this PR, I could not reproduce the bug in the first place. See my screencast, I closed the app with Cmd+Q and see this comment, the app was closed with X.

[vince-fugnitto]

When trying to esc the 'confirmExit' dialog, I noticed the following errors on the FE:


There are a lot of issues and discussions on the official electron repository regarding beforeUnload so I'm currently looking at what might be the cause or fix.

[mcgordonite]

Hi @kittaakos, I've investigated this a bit on Windows:

• On commit fcccdf44dc23a31d1bab63c330df75a7aeb33a0c, just before https://github.com/eclipse-theia/theia/pull/6285, I never see the "confirm exit" dialog.
• On commit aff11bec2a0424a63e506613d9e23f2e6bdd1d42, just after https://github.com/eclipse-theia/theia/pull/6285, I see the "confirm exit" dialog if the preferences are set appropriately. If I cancel exit, the backend does not crash, there is no yellow bar, and I can save any unsaved files.
• On master, I see the bug reported in this issue. The electron master process and the BrowserWindow process are still running, but the frontend becomes disconnected from the backend somehow.

[kittaakos]

Thanks a lot for your time, @mcgordonite 🙏

• On commit aff11be, just after #6285, I see the "confirm exit" dialog if the preferences are set appropriately. If I cancel exit, the backend does not crash, there is no yellow bar, and I can save any unsaved files.

That's a good idea, let me try the same on macOS.

• On master, I see the bug reported in this issue. The electron master process and the BrowserWindow process are still running, but the frontend becomes disconnected from the backend somehow.

So we broke it somewhere else. Apologies for pulling you into this issue. I'll try to find the breaking change.

[mcgordonite]

Putting this back to unload seems to fix it: https://github.com/eclipse-theia/theia/commit/d7f7c90376e3660f15149a2de8286d8d2b015c75#diff-b3c47e20ff0569909accc8030cc40ea8R164

I think there would be consequences to making that change though.

[zhaomenghuan]

Hello @kittaakos, is there any new progress on this question now? I temporarily circumvented it with the following configuration:

"frontend": {
    "config": {
        "preferences": {
            "application.confirmExit": "never"
        }
    }
}

[kittaakos]

No, there aren't any updates 😕 Feel free to help us; your most recent contribution was super valuable.

[zhaomenghuan]

@kittaakos I tried to comment out the beforeunload event in the FrontendApplication class. At this time, if the modified file is not saved and clicked to exit, the front end will not be disconnected. I guess this problem is related to the code here.

window.addEventListener('beforeunload', () => {
    this.stateService.state = 'closing_window';
    this.layoutRestorer.storeLayout(this);
    this.stopContributions();
});

In addition, clicking the yes button did not exit normally, and I feel that the logic is not right.Looks like a bug in electron, please see here: https://github.com/electron/electron/issues/24994

[kittaakos]

Possible related: https://github.com/eclipse-theia/theia/issues/8595

[kittaakos]

@zhaomenghuan, could you please give it a try with this change?

[zhaomenghuan]

@zhaomenghuan OK，I' will have a try and give you feedback then.

[danieltomasku]

Hi, sorry to open up a closed issue, however on master I'm still encountering the bug where confirming the exit does not work properly on Electron (it appears to working fine in the Browser version). When prompted with the "Are you sure you want to quit?" dialog, hitting "No" immediately closes the application. This is particularly worrisome because now it will exit the application without allowing the user to recover their work, whereas before it would kill the backend process but there was at least an opportunity to copy over the work that was unsaved.

To reproduce (I'm running this on RHEL7):

• Open the Electron version of Theia from master
• Open a file and make unsaved changes
• Try to exit the application
• You should get the "Are you sure you want to quit?" dialog
• Then hit "No" and confirm that the application just exits and does not give you an opportunity to save
• When you open the application again, you'll notice that the layout is not preserved and the dirty file was not saved

The odd part about this is if you open the DevTools and then do the above steps, everything works as expected (this might be why when testing it slipped through the cracks).

Any thoughts @kittaakos @marechal-p ?

I'd be happy to open up a new ticket if you'd prefer that instead.

[vince-fugnitto]

@danieltomasku thank you for reporting the issue :+1: I've confirmed the following:

• linux (ubuntu): the bug is reproduced, closing the app and selecting no still closes the application incorrectly.
• macos: the bug cannot be reproduced on my end.

[marechal-p]

I cannot reproduce on Windows either. It must be specific to Linux.

[danieltomasku]

@danieltomasku thank you for reporting the issue 👍 I've confirmed the following:

• linux (ubuntu): the bug is reproduced, closing the app and selecting no still closes the application incorrectly.
• macos: the bug cannot be reproduced on my end.

Ok thanks for confirming @vince-fugnitto @marechal-p. Good to know that it is only happening on Linux. Still puzzled why having the DevTools open makes the functionality work correctly 🤔 it certainly makes it hard to debug the issue! I tried a few things on my end to get it to work but was unsuccessful. The handling of the beforeunload/close events seems to be a big point of confusion on different issues/threads regarding Electron.

[kittaakos]

We have tried so many ways so far; we can try this too. It's a gamble, though and I do not know if it works at all. 😅 At least we have not tried with the window close approach.

Update: with the showMessageBoxSync way.