Closed Bug 17839 Opened 25 years ago Closed 25 years ago

fix goQuitApplication() to do a clean shutdown

Categories

(SeaMonkey :: General, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: waterson, Assigned: davidm)

References

()

Details

(Keywords: verifyme)

Attachments

(1 file)

goQuitApplication() in globalOverlay.js currently calls nsAppShellService::Quit(). This does a "dirty shutdown", and fails to clean up after scripts. This causes the Tinderbox "leak report" to be unreliable.
Target Milestone: M12
David, Fixing this is critical to getting a reliable feedback loop going with tinderbox so that developers can see leaks they've caused. Thanks for looking at it.
There is C++ code in the appshellservice ( which I will be replacing with a modified version of the JS now that the related bugs have been fixed) which walks the window list and does a close. The window mediator code might be leaking ( took a quick look and didn't see anything obvious ) in which case switching to JS will not help.
I checked in my JS. Will have to figure out why the extra call to Quit is causing problems.
yeah, i tried removing the call to appshell 'quit' on the mac the hidden window never gets clobbered. slamm, mcafee: can we change the test s.t. we do a window.close() instead of app.quit()?
app.quit should do the right thing (close all the windows cleanly). If you don't think it should do the right thing for release builds (should just get the hell out), then perhaps we can at least make it do the right thing for debug builds. But I think we should fix it to close all the windows first, and then see if it's a problem.
What is app.quit? If that is a call to the appshellservice::Quit routine, you really should be calling the quit routine in globalOverlay.js. It does a smarter quit.
no, nsAppShellService::Quit() does _not_ do a smarter quit. It leaks.
I did NOT say to call nsAppShellService::Quit().
Attached patch proposed fix (deleted) — Splinter Review
danm, davidm, what do you think of my fix? I basically put the call to mAppShell->Exit() onto a timer callback with a zero timeout. I've tried this on Mac, Win32, and Linux, and it seems to work. And bloaty on Linux is showing ~100K less leakage.
...I also removed the code that enumerated all the windows and closed them, because either 1) this is done in JS now, or 2) it's called when the last window has closed on Unix/Win32
I would have posted a PLEvent but thats a minor quibble. I assume you tried both quiting mechanism ( the menu and closing the last open window on linux/win)? I thought that if we commented out the call from JS to exit the leak went a way. If that is true can't the code just be set to make the second call a no-op ( don't reset the mQuiting flag)?
Yes, I tried closing all windows on Unix & Win, and it behaved as expected. Not sure what you mean by "[commenting] out the call from JS to exit". If I comment out the call to to nsAppShellService::Quit() in the JS, then on Win32 and Linux, the leaks go away; but on Mac, the app will never terminate. If I comment out the call to mAppShell->Exit() in nsAppShellService, then the Unix event loop never terminates. If you meant, "why don't the leaks -all- go away," it's because the 100K (actually, 175K) was all the XUL junk that was being held from uncollected script contexts; there is still about 200K of stuff lying around. I took out the mQuiting (isn't it "quitting"?) flag, and just replaced that state with whether or not the mShutdownTimer was non-null. I think we do want to guard against re-entering the routine, although I'm not sure how it would happen now that I've removed the "close all the windows" loop from Quit()... Also, Win32 needs a makefile change to include timer_s.lib; mac must get the timer stuff from widget.shlb, and unix just doesn't need to link against nuthin'.
The point I am making is that in the old code what happened was the Quit was called twice. Once by the last window closing (from C++) and the second time by JS. I would think that if the JS call was made to be a noop ( by setting a quiting flag), that life should be good but that is just a guess.
The old code -does- set an mQuiting [sic] flag. So the second call -is- a no-op. What's your point?
Are you sure? My reading of the code has the mQuiting flag being reset after the first call. The flag was to prevent Quit being called when the windows were being closed by the quit code.
Ah, I see. Sorry for being dense :-). No, I didn't try that, but I don't think that would solve the problem. You see, on Unix, nsIAppShell::Exit() destroys the event loop (it basically tells gtk to shutdown, right now, do not pass go, do not collect $200). So in the old code, as soon as you call nsAppShellService::Quit(), the event loop is torn down, and -no- other events (e.g., processing that happens for window close callbacks) will happen. [Win32, OTOH, does a ::PostQuitEvent(), which'll at least flush the event queue. Not sure what Mac'll do.] So by putting the call to nsIAppShell::Exit() on its own event (aka, a zero-timeout timer), we do an XP flush of the event queue and guarantee that any post-window destroy cleanup will occur. At least that's my theory. :-)
The only reason I suggest it because you said commenting out the second exit prevented the leaks. If it doesn't your more complex solution is OK by me. I have to update the mac appleevent handling code since it is currently calling quit but that shouldn't be a big deal.
Maybe we should leave the native window mediator code in there: I just removed it because I thought the only other caller was JS, which is also enumerating and closing all the windows.
No the code has to go eventually since the JS routine is a bit more complex than just closing each window. I just have to make sure all the callers of Quit are switched over. Or maybe I should make Quit call the js routine and add another routine to actually shutdown the appshell. I have to think about this.
Please make the obvious thing be the right thing, i.e. nsAppShellService::Quit(). Otherwise name it something appropriate (e.g. FinalQuit or something).
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
davidm: checked in my stuff, a=warren. if there's still changes you need to make, you can re-open the bug. thanks, chris
Adding verifyme keyword.
Keywords: verifyme
Old bug...I am going with all he eng comments that this is clean now. Please reopen if still and issue. mrking Verified.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: