AtExit in a mobile world

[Stuart Morgan]

Currently, all of our singletons register with an AtExit manager so
that their destructors will be run when the app quits. That’s fine on
desktop, but Chrome now runs on two platforms—Android and iOS—where in
normal use the app never actually exits. In fact, I’m not sure about
Android, but on iOS the app *never* quits; there’s simply no such
concept for a backgroundable app. Instead apps are suspended/paused,
and then either re-activated later, or silently killed without any
further code being run.

On mobile, there’s no moment where you can definitively say the app is
going to quit, and thus run the AtExit handlers. In effect, all
singletons are leaky for mobile.

We’ve been auditing singletons as we’ve needed them to make sure they
don’t do anything critical in their destructors (good news: they
pretty much never do!), but that approach isn’t sustainable/scalable.
We check that the destructors are empty, but of course:
- People may add things to them later, and
- “Empty” constructors aren’t; what we really need to know if anything
in the transitive chain of destructors of member variables does
anything important. That’s very hard to determine at one point in
time, and impossible for us to monitor long-term.


What do people think about making the default mode for
Singleton/LazyInstance be leaky, and having a (very rare) trait for
non-leaky? That would make it nearly impossible for someone designing
a singleton to rely on something happening on the destructor without
thinking about the effect on the mobile platforms. It would also make
us less likely to end up with code that can’t gracefully recover from
a crash (I assume this is why mostly the destructors aren’t doing
anything already). And it would avoid the kind of destruction errors
in singletons that have been discussed in earlier threads here.

We would need to add a new hook for the cases where a singleton needs
to, say, persist some state to disk; this would be called at shutdown
for desktop, and suspend time on mobile. Unlike AtExit it wouldn’t
tear anything down, so it could be used the same way on both desktop
and mobile.

Thoughts? Suggestions for alternate solutions to this problem? (For
instance, willchan’s previous suggestion of COMPILE_ASSERT(false,
IHaveNoIdeaWhatIAmDoing) as the implementation of Singleton and
LazyInstance would solve this problem neatly!)

-Stuart

[Marc-Antoine Ruel]

When I chose the default Singleton trait to not be leaky, I had in mind ChromeFrame usage. But since even this use case cannot really unload the DLL safely, "leaking by default" Singleton trait makes sense to me.

Sorry for the initial design error. :) Feel free to fix.

M-A

[Robert Shield]

On Wed, Jul 25, 2012 at 9:05 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote:

When I chose the default Singleton trait to not be leaky, I had in mind ChromeFrame usage. But since even this use case cannot really unload the DLL safely, "leaking by default" Singleton trait makes sense to me.

Yeah, in practice the Chrome Frame DLL is never unloaded from an IE process that loads it. Please remove CF from any mental lists of "reasons to not leak singletons" :-)

Also, +1 on the COMPILE_ASSERT suggestion in the OP.

 

Sorry for the initial design error. :) Feel free to fix.

M-A

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

+1. Exit time constructors slow down app shutdown in desktop chrome
too, so I think that's a good change anyhow.

Nico

> That would make it nearly impossible for someone designing
> a singleton to rely on something happening on the destructor without
> thinking about the effect on the mobile platforms. It would also make
> us less likely to end up with code that can’t gracefully recover from
> a crash (I assume this is why mostly the destructors aren’t doing
> anything already). And it would avoid the kind of destruction errors
> in singletons that have been discussed in earlier threads here.
>
> We would need to add a new hook for the cases where a singleton needs
> to, say, persist some state to disk; this would be called at shutdown
> for desktop, and suspend time on mobile. Unlike AtExit it wouldn’t
> tear anything down, so it could be used the same way on both desktop
> and mobile.
>
> Thoughts? Suggestions for alternate solutions to this problem? (For
> instance, willchan’s previous suggestion of COMPILE_ASSERT(false,
> IHaveNoIdeaWhatIAmDoing) as the implementation of Singleton and
> LazyInstance would solve this problem neatly!)
>
> -Stuart
>

[Felipe Goldstein]

I totally agree.

It seems that most of the singletons in the Chrome for Android side we just let it leak.

( 

  Although I am usually against the use of singletons, which makes me remember of this old discussion: 

  http://www.object-oriented-security.org/lets-argue/singletons

  http://googletesting.blogspot.fr/2008/08/root-cause-of-singletons.html

  But, I don't think we should go into that discussion.

 )

Instead, my suggestion is that we should just think twice when adding new singletons.

Willchan's/stuart's idea is a way of making this explicitly.

-Stuart

[Felipe Goldstein]

FYI, another data point to the discussion.

In Chrome for Android we already had a bug where we got a sub-class inheriting from ContentMainDelegate.

We have an assert in the destructor of the sub-class as: NOTREACHED() since we want the singletons to leak.

But in that case the destructor of that singleton was (incorrectly) being called, so it broke and we got a stack trace.

We had a hard time finding out why and how it was called, but at least we got the stack trace, due to the assertion.

The singleton in question lives here:

LazyInstance<scoped_ptr<ContentMainDelegate> > g_content_main_delegate = LAZY_INSTANCE_INITIALIZER;

http://src.chromium.org/chrome/trunk/src/content/app/android/content_main.cc

http://src.chromium.org/chrome/trunk/src/content/public/app/content_main_delegate.h

In case we decide making the default mode for Singleton/LazyInstance be leaky as Stuart suggested, we may want to promote the use of NOTREACHED() assertion in the destructor of singletons as well.

On Wed, Jul 25, 2012 at 4:58 PM, Felipe Goldstein <fel...@google.com> wrote:

I totally agree.

It seems that most of the singletons in the Chrome for Android side we just let it leak.

( 

  Although I am usually against the use of singletons, which makes me remember of this old discussion: 

  http://www.object-oriented-security.org/lets-argue/singletons

  http://googletesting.blogspot.fr/2008/08/root-cause-of-singletons.html

  But, I don't think we should go into that discussion.

 )

Instead, my suggestion is that we should just think twice when adding new singletons.

Willchan's/stuart's idea is a way of making this explicitly.

On Wed, Jul 25, 2012 at 9:06 AM, Stuart Morgan <stuart...@chromium.org> wrote:

-Stuart

[Scott Hess]

On Wed, Jul 25, 2012 at 12:06 AM, Stuart Morgan
<stuart...@chromium.org> wrote:
> We would need to add a new hook for the cases where a singleton needs
> to, say, persist some state to disk; this would be called at shutdown
> for desktop, and suspend time on mobile. Unlike AtExit it wouldn’t
> tear anything down, so it could be used the same way on both desktop
> and mobile.

Since I haven't seen this point argued - if anyone is using the
singleton destructor to do things like this at shutdown, they are
doing it wrong in just about the worst possible way that they could be
doing it wrong. If you can't figure out how to hook shutdown
correctly (*), you definitely aren't going to be able to hook
destructor ordering correctly.

-scott

(*) I'm not saying it's easy, I certainly do not know how to hook
shutdown correctly.

[William Chan (陈智昌)]

Yep, let's switch the default to leaky. Thanks for raising this issue again.

On Wed, Jul 25, 2012 at 12:06 AM, Stuart Morgan <stuart...@chromium.org> wrote:

-Stuart

[Jonathan Dixon]

Just one thought, the main hurdle you may hit is with test code that relies on the AtExit manager to achieve cleanup between test steps.

It may not be too big a job though... "git gs AtExit | grep test" only throws up 7 cases, all using ShadowingAtExitManager. So it maybe just a case of working out which 7 specific things they need cleaned up in those tests, and switch each of those to use explicit AtExit enabled version of Singleton etc - at least as a first step.

[Scott Byer]

That's not quite sufficient; there are some hidden implicit orderings
among tests. Running unit_tests with shuffle doesn't get very far;
there are additional clean-ups needed which aren't currently covered
by uses of ShadowingAtExitManager.

[Ryan Hamilton]

I wonder if it would make sense to do something like run each test in isolation.  If that test fails, file a bug (with someone) to fix it.  With some effort, and a bit of luck, I think we would be able to eliminate these hidden dependencies.  (We could then run the tests in this matter as part of a waterfall).

[Marc-Antoine Ruel]

2012/7/27 Ryan Hamilton <r...@chromium.org>

I wonder if it would make sense to do something like run each test in isolation.  If that test fails, file a bug (with someone) to fix it.  With some effort, and a bit of luck, I think we would be able to eliminate these hidden dependencies.  (We could then run the tests in this matter as part of a waterfall).

./tools/isolate/run_test_cases.py out/Release/unit_tests

You can use -j1 if you want them to be run one at a time. --help for additional info.

M-A