Closed Bug 384284 Opened 17 years ago Closed 10 years ago

Quitting from the dock leaks domwindows

Categories

(Core :: General, defect, P5)

Product:
Core
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: memory-leak)

Attachments

(1 file)

Possible patch
20.01 KB, patch
Details | Diff | Splinter Review 
Steps to reproduce:
1. Launch Firefox (debug) from the command line.
2. Quit Firefox by holding the mouse button down on the dock icon and selecting "Quit" when the menu appears.

Result: Firefox quits leaking all 6 DOMWINDOWs.  (No additional --DOMWINDOW lines appear.)

(If I quit with Cmd+Q instead, it usually doesn't leak any domwindows.)
Does it still happen if you move aside the safe browsing JS components (nsSafeBrowsing*, nsUrlClassifier*)?
How can I do that?
cd /Applications/Minefield/Contents/MacOS/components
mkdir noload
mv nsSafeBrowsing* noload
mv nsUrlClassifier* noload
Actually, "noload" can't be in the components/ directory. So add: 

mv noload ../

to those steps.
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/20070618 Minefield/3.0a6pre

The fix for bug 383269 doesn't seem to have fixed this. I can still reproduce in today's nightly. This needs a better home than Firefox: General...
Product: Firefox → Core
QA Contact: general → general
The fix for bug 377506 did not fix this.
Flags: blocking1.9+
Assignee: nobody → smichaud
Attached patch Possible patch — — Splinter Review 
It turns out that this bug is _very_ difficult, and probably
impossible to resolve completely.

The reason is that when you quit from the Dock menu (or more generally
when [NSApplication terminate:] is called) it's actually the OS that
terminates the browser -- not the browser itself.  So [NSApp run]
never returns (and neither do nsIAppShell::Run() or
nsIAppShell::Run()).  And the stack-based objects created above the
call to nsAppStartup->Run() in toolkit/xre/nsAppRunner.cpp's
XRE_main() never get deleted.

My patch for bug 377506 went a large part of the way toward fixing the
unclean shutdown that occurs when the OS terminates the browser (by
calling nsIAppStartup::Quit() and so forth from
[MacApplicationDelegate applicationShouldTerminate:].  But it didn't
fix the whole problem, because I didn't realize that there's at least
one more thing that needs to be done to accomplish a clean shutdown --
you need to shut down XPCOM.

In various places in toolkit/xre/nsAppRunner.cpp, XPCOM gets shut down
when a stack-based ScopedXPCOMStartup object is destroyed.  My patch
doesn't destroy this object, but instead does the next best thing --
it abstracts the calls needed to shut down XPCOM from the
ScopedXPCOMStartup object's destructor into a (new) Destroy method,
and calls that instead.

For good measure it also Releases the current nsIAppShell object one
extra time, so that object gets destroyed while XPCOM is shutting
down.

I didn't do anything about the other stack-based objects that don't
get destroyed -- I'm afraid that would make my patch too fragile (it
would need frequent changes), and in any case it seems that leaking
these objects (and not calling their destructors) causes less damage.

Even with my patch there are still a few leaks when you Quit from the
Dock menu ... but nothing like all the DOM windows.
There is no way to subclass NSApplication:terminate?

I would much prefer to leak and have a semi-unclean shutdown than try to shut down XPCOM in the middle of a stack while there are stack-based objects still alive. That is a recipe for crashes, when right now we're just leaking.
> There is no way to subclass NSApplication:terminate?

Yes, I suppose I could do that (using NSObject poseAsClass:).  But
that would mess up the OS shutdown sequence (what takes place when,
for example, you log out the current user and quit all apps), and I'm
not sure I'd be able to implement this functionality on my own.

> I would much prefer to leak and have a semi-unclean shutdown ...

Just to play devil's advocate:  What difference do leaks on shutdown
make anyway?  Will the OS just release all the RAM that the quitting
app allocated?

If that's the case, then maybe this bug isn't worth fixing.  We could
just leave things the way they are.

> than try to shut down XPCOM in the middle of a stack while there are
> stack-based objects still alive. That is a recipe for crashes,

Your anxiety is misplaced -- if [NSApplication terminate;] causes
[NSApp run] not to return (when the user doesn't cancel the
termination), you won't get any crashes.

But (as I said in comment #9), maybe memory leaks on quitting just
aren't worth a lot of effort to fix.

I guess I really have to make a synchronous shutdown possible. Linux suffers from a similar problem, but with worse symptoms.
I'm going to claim that any attempts to do synchronous shutdown in the 1.9 timeframe are misplaced effort. We should patch the symptoms (unsaved profile data) and abort cleanly.

smichaud, you still have to worry about static destructors, which will frequently crash if they still have XPCOM references after XPCOM shutdown.
> smichaud, you still have to worry about static destructors, which
> will frequently crash if they still have XPCOM references after
> XPCOM shutdown.

Then they'll crash now (without my patch).

A static destructor is called as the executable where its object is
located is unloaded.  Even now this always happens after XPCOM has
been shut down.

If it makes you feel more comfortable, though, I can stop calling an
extra Release() on the current nsIAppShell object, but continue
calling gScopedXPCOMStartup->Destroy() (via nsKillXPCOMEvent).
Leaking the current nsIAppShell object isn't a big deal (as far as I
know).

Holding onto cross-module references after XPCOM shutdown (stack COMPtrs) is an easy way to make the app unstable (crashy), even if you don't end up releasing those references.

I'm not sure I understand the issue with subclassing NSApplication:quit. Why couldn't we cause NSApplication:quit to end the eventloop gracefully, for both the case where you choose "Quit" from the dock menu and when the OS is shutting down/rebooting.

In case it wasn't clear, I agree completely that the memory leaks are not worth fixing. What does need to be fixed is saving profile data through the proper quit and profile-shutdown notifications.
> In case it wasn't clear, I agree completely that the memory leaks
> are not worth fixing. What does need to be fixed is saving profile
> data through the proper quit and profile-shutdown notifications.

I don't know much about what NS_ShutdownXPCOM() does.  If it doesn't
do anything essential (i.e. if that's already been taken care of by
the patch to bug 377506), maybe we should just leave things as they are
(and leave this bug unfixed).

> I'm not sure I understand the issue with subclassing
> NSApplication:quit. Why couldn't we cause NSApplication:quit to end
> the eventloop gracefully

As I said in comment #9, the problem is the OS's procedure for
shutting down running apps when the current user logs out or does a
shutdown/restart.  [NSApplication terminate:] causes [NSApp run] not
to return (a bad thing for us), but also is what the OS uses to
implement its shutdown procedure.  If we "subclass" [NSApplication
terminate:] to stop it from doing (in effect) a force-quit, we'll also
need to dig into undocumented OS internals to figure out how to
support the OS's shutdown procedure without doing a force quit.

Though I can't guarantee it'll work in this case, I've done this kind
of thing successfully many times before.  But it's a lot of work, and
we already have a decent solution that's a lot simpler (my current
patch).

However, if we're not worried about memory leaks on quitting, and
(therefore) don't need to guarantee that XPCOM is shut down as the
browser quits, the whole business is moot.

> Holding onto cross-module references after XPCOM shutdown (stack
> COMPtrs) is an easy way to make the app unstable (crashy), even if
> you don't end up releasing those references.

This doesn't sound right to me ... but maybe the whole question is
moot.

I certainly hope it is :-)

>> Holding onto cross-module references after XPCOM shutdown (stack
>> COMPtrs) is an easy way to make the app unstable (crashy), even if
>> you don't end up releasing those references.
>
> This doesn't sound right to me

I see the problem now (I think) -- a cross module reference will be
invalid if the module where the target is located is unloaded before
the current module is unloaded.

(Following up comment #16)

... but once again, this problem exists right now (without my patch).

Simple changes such as releasing a COMPtr earlier in shutdown have caused or fixed crashes in the past. Shutdown is very fragile, so even holding onto a reference unexpectedly may cause shutdown to reorder, which can cause crashes.
(In reply to comment #18)

Ah, the infamous black box that you don't dare to touch :-)

>> In case it wasn't clear, I agree completely that the memory leaks
>> are not worth fixing. What does need to be fixed is saving profile
>> data through the proper quit and profile-shutdown notifications.
>
> I don't know much about what NS_ShutdownXPCOM() does.  If it doesn't
> do anything essential (i.e. if that's already been taken care of by
> the patch to bug 377506), maybe we should just leave things as they
> are (and leave this bug unfixed).

So do you think it's OK to leave this bug unfixed?

If "this bug" means the leaks, then we don't have to fix it. But I'd have to file a different bug about "Quitting from the dock doesn't show quit confirmation and makes session-restore think we've crashed"...
> But I'd have to file a different bug about "Quitting from the dock
> doesn't show quit confirmation and makes session-restore think we've
> crashed"...

This problem was fixed by the patch for bug 377506, landed on
2007-07-02.

So -> WONTFIX?

That patch is insufficient if we don't ever walk back up the stack: If we don't actually return from nsIAppStartup->Run we will never fire the profile-shutdown notifications, which are used by various services including prefs and places to ensure that cached profile data is flushed to disk.
OK.

But even if I subclass [NSApplication terminate:], I'll need a method
I can call that does something like a synchronous shutdown, which I'd
call when [NSApplication terminate:] is called as part of the OS's
shutdown sequence (and perhaps also on other occasions, if I'm unable
to find out a way to distinguish calls to [NSApplication terminate:]
that are part of the OS's shutdown sequence).

Only after this synchronous shutdown method had been called could I
return from [NSApplication terminate:].

Is that a documented requirement of [NSApplication terminate:]? Why can't we just let the terminate call be a signal to shut down the event loop and end the process asynchronously?
Very little of this is documented, of course.

But the fact that the OS currently force-quits the current app in
[NSApplication terminate:] means that it expects the app to be
(effectively) dead by some point in that method (whether or not
[NSApplication terminate:] actually "returns").

Or here's another trick that might work:

In my subclassed [NSApplication terminate:] method I could spin the
browser's main-thread event loop (using calls to
NS_ProcessNextEvent(), like I do in my patch's [MacApplicationDelegate
applicationWillTerminate:] method) until something happens (a global
variable changes, an event is received) that signals that the browser
is really dead.

The bottom line is that, when I return control to the OS (from my
subclassed [NSApplication terminate:] method), the browser needs to be
near enough to actual death not to mess up the OS's shutdown sequence.

> Very little of this is documented, of course.

The fact that [NSApplication run] doesn't return when an app is
terminated via [NSApplication terminate:] is one of the few things
that _is_ documented:

http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSApplication_Class/Reference/Reference.html

The blurb on the "run" method tells you to run it from your
application's main() method.  And the blurb on the "terminate:" method
tells you not to put cleanup code in your main() method (after the
call to "run"), because it will never get called (since the "run"
method won't return if "terminate:" is called and not cancelled).

> The bottom line is that, when I return control to the OS (from my
> subclassed [NSApplication terminate:] method), the browser needs to
> be near enough to actual death not to mess up the OS's shutdown
> sequence.

On reflection, I _may_ not need a synchronous shutdown method to avoid
messing up the OS's shutdown sequence (what happens when you logout,
shutdown or restart and the OS quits all the currently running apps).

Over the next week or two, I'll be working on subclassing
[NSApplication terminate:].

(What encourages me is something that happened while testing my
"possible patch":  Earlier versions of the patch froze on shutdown
while the OS was trying to force-quit the browser.  But after even an
indefinitely long hang, the shutdown sequence always resumed after I'd
done a 'kill -9' on the browser.  This indicates that the OS may (in
its shutdown sequence) simply be waiting for the browser process to
die.)

I'm setting a priority on this so that it keeps its blocking1.9+ flag.

I don't believe this bug has any user-visible effects.  And in general
leaks on exit aren't very significant.  But I've already done quite a
bit of work on this ... so P5.
Priority: -- → P5
Given comment 31 moving this to the wanted list.
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
For all I know this may still be happening.  But I'll almost certainly never have time to work on it.
Assignee: smichaud → nobody
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INCOMPLETE
WFM on mozilla-central :)
Resolution: INCOMPLETE → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: