Skip to content
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

window close event gets ignored if nw2 is disabled / process signals get ignored and NW.js crashes #7981

Closed
2 tasks done
bastimeyer opened this issue Oct 16, 2022 · 11 comments
Labels

Comments

@bastimeyer
Copy link

bastimeyer commented Oct 16, 2022

Issue Type

  • Bug Report
  • Successfully reproduced against the latest version of NW.js?

Issue

I'm reporting two issues that are somewhat related to each other when trying to intercept the termination of NW.js apps. I noticed the second issue while trying to debug the first one.

  1. Starting with NW.js 0.68, when --disable-features=nw2 is set, attaching a close event listener to a NW.js window does not have any effect and the callback gets ignored.
    https://docs.nwjs.io/en/latest/References/Window/#event-close
  2. When calling process.removeAllListeners() at the beginning of the application code (e.g. for being able to reset global application state in-between application window reloads) and then attaching an event listener on the process for a SIGTERM signal for example, sending the signal to the running process will cause NW.js to crash if --disable-features=nw2 is set. If --disable-features=nw2 is not set, then the event listener gets ignored, which is bad.
    https://nodejs.org/api/process.html#signal-events

My application had to set --disable-features=nw2 twice in the past because of various issues with the new nw2 implementation (1, 2), and so far I haven't reverted this (again). I will need to test the latest releases and see if those issues still occur, but the issues are fixed, I will upgrade to nw2, so the issue shouldn't affect my application. However, I felt like this issue is still worth reporting, as it's a regression that has been introduced in 0.68.

The process event listener issue on the other hand looks like a NodeJS related issue (decided to test this after proof-reading my post). NW.js shouldn't crash though if --disable-features=nw2 is set.

Reproduction

Please see my reproduction repo here with a simple NW.js app and a simple GitHub actions workflow:
https://github.com/bastimeyer/nwjs-window-close

The resulting log output with NW.js ranging from 0.64 to 0.70 while running several tests can be found here:
https://github.com/bastimeyer/nwjs-window-close/actions/runs/3384916071

@panther7
Copy link

I see crash too.

  • NWjs 0.66.1 works.
  • NWjs 0.67.x not tested
  • NWjs 0.68.1 and 0.69.1 crash

@ayushmanchhabra ayushmanchhabra added bug nw2 has-min-repro Has a minimum reproduction labels Oct 20, 2022
@arudnev
Copy link

arudnev commented Oct 28, 2022

Yeah, we are still stuck on 0.66.1. I'm not sure it crashes, most likely it just exists the process without firing the event.

It can be reproduced just by posting the following in console (shows alert in 0.66.1 and before, just exits in more recent version)

nw.Window.get().on('close', (q) => { alert(q ? 'quit' : 'hide'); q && nw.App.quit(); })

@rogerwang, could you please take a look at this one? I thought I saw this defect in the past, then it got fixed, but was re-introduced a few months back and it's been blocking upgrade to more recent versions for us as we rely on that feature.

@panther7
Copy link

This is breaking bug for us. Close event is not triggered.

@panther7
Copy link

panther7 commented Nov 3, 2022

NWjs 0.70 is broken too

@bastimeyer
Copy link
Author

Yep... I've added v0.70.0 to my reproduction repo and updated the link in the OP.

@rogerwang
Copy link
Member

NW1 mode was deprecated three years ago: https://groups.google.com/g/nwjs-general/c/1YMHN3m1rtI/m/f3gF-Jl3AgAJ

so bugs in nw1 would not be fixed.

@bastimeyer
Copy link
Author

Yes, I am aware, but NW2 has had lots of bugs over the past years (#7230), and most recently this one #7982, which prevents me from switching (back) to NW2. It's a "pick your poison" situation for me and either one of those bugs needs to be fixed.

@pd-l2ork
Copy link

pd-l2ork commented Nov 8, 2022

Second this issue. Just realized that my #8000 (that I now closed) is a duplicate of this. Given that nw2 is about 10x or more slower in processing scripted events than nw1, we really need nw1 to continue to be supported until nw2 reaches the same level of efficiency as nw1. See #7979 for more info.

@pd-l2ork
Copy link

pd-l2ork commented Nov 8, 2022

NW1 mode was deprecated three years ago: https://groups.google.com/g/nwjs-general/c/1YMHN3m1rtI/m/f3gF-Jl3AgAJ

so bugs in nw1 would not be fixed.

@rogerwang nw2 is 10x (or more slower) using scripting (see #7979 ) and should not be considered a replacement for nw1 yet. It literally renders my application unusable. Something that loads within a second on nw1 takes 10+ seconds to load in nw2 mode using the same version of nwjs.

@pd-l2ork
Copy link

pd-l2ork commented Nov 8, 2022

@rogerwang It appears #7943 also affects only nw2 but not nw1 which is another potential showstopper (cannot use CMD+(1-9) in nw2). I kindly ask that you please fix this issue as that is the only way I can go beyond 0.67.1. Thank you for your consideration.

@ayushmanchhabra
Copy link
Contributor

Duplicate of #7808

@ayushmanchhabra ayushmanchhabra marked this as a duplicate of #7808 Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants