nw.js icon indicating copy to clipboard operation
nw.js copied to clipboard

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

Open bastimeyer opened this issue 3 years ago • 11 comments

Issue Type

  • [x] Bug Report
  • [x] 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

bastimeyer avatar Oct 16 '22 06:10 bastimeyer

I see crash too.

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

panther7 avatar Oct 19 '22 13:10 panther7

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.

arudnev avatar Oct 28 '22 19:10 arudnev

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

panther7 avatar Oct 31 '22 16:10 panther7

NWjs 0.70 is broken too

panther7 avatar Nov 03 '22 08:11 panther7

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

bastimeyer avatar Nov 03 '22 10:11 bastimeyer

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 avatar Nov 04 '22 01:11 rogerwang

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.

bastimeyer avatar Nov 04 '22 07:11 bastimeyer

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 avatar Nov 08 '22 23:11 pd-l2ork

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 avatar Nov 08 '22 23:11 pd-l2ork

@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.

pd-l2ork avatar Nov 08 '22 23:11 pd-l2ork

Duplicate of #7808

ayushmanchhabra avatar Apr 21 '23 19:04 ayushmanchhabra