Closed
Bug 17839
Opened 25 years ago
Closed 25 years ago
fix goQuitApplication() to do a clean shutdown
Categories
(SeaMonkey :: General, defect, P3)
SeaMonkey
General
Tracking
(Not tracked)
VERIFIED
FIXED
M12
People
(Reporter: waterson, Assigned: davidm)
References
()
Details
(Keywords: verifyme)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•25 years ago
|
Target Milestone: M12
Comment 1•25 years ago
|
||
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.
Reporter | ||
Comment 4•25 years ago
|
||
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()?
Comment 5•25 years ago
|
||
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.
Reporter | ||
Comment 7•25 years ago
|
||
no, nsAppShellService::Quit() does _not_ do a smarter quit. It leaks.
Reporter | ||
Comment 9•25 years ago
|
||
Reporter | ||
Comment 10•25 years ago
|
||
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.
Reporter | ||
Comment 11•25 years ago
|
||
...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
Assignee | ||
Comment 12•25 years ago
|
||
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)?
Reporter | ||
Comment 13•25 years ago
|
||
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'.
Assignee | ||
Comment 14•25 years ago
|
||
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.
Reporter | ||
Comment 15•25 years ago
|
||
The old code -does- set an mQuiting [sic] flag. So the second call -is- a no-op.
What's your point?
Assignee | ||
Comment 16•25 years ago
|
||
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.
Reporter | ||
Comment 17•25 years ago
|
||
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. :-)
Assignee | ||
Comment 18•25 years ago
|
||
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.
Reporter | ||
Comment 19•25 years ago
|
||
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.
Assignee | ||
Comment 20•25 years ago
|
||
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.
Comment 21•25 years ago
|
||
Please make the obvious thing be the right thing,
i.e. nsAppShellService::Quit(). Otherwise name it something appropriate (e.g.
FinalQuit or something).
Reporter | ||
Updated•25 years ago
|
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 22•25 years ago
|
||
davidm: checked in my stuff, a=warren. if there's still changes you need to
make, you can re-open the bug.
thanks,
chris
Comment 24•24 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•