Randell Jesup [:jesup] (needinfo me) Assignee
	Description • 9 years ago

While looking at a bug due to ClearOnShutdown happening after xpcom-shutdown-threads (where something wanted it cleared before xpcom-shutdown-threads), I realized a lot of cleanup observers just need to clear some smart pointers (by specific phases), and that it would be easy to make ClearOnShutdown clear at a specific phase.  This would allow considerable simplification of shutdown code across the tree.

	Nathan Froyd [:froydnj]
	Comment 2 • 9 years ago

Comment on attachment 8705370 [details] [diff] [review]
Extend ClearOnShutdown() to allow specifying the shutdown phase

Review of attachment 8705370 [details] [diff] [review]:
-----------------------------------------------------------------

I like the idea of not relying on the observer service to do cleanup, and we should extend this idea further in future bugs.  Replacing some of the shutdown observers with something that passes in a lambda for cleanup, or something like that, maybe.  Some comments below.

::: xpcom/base/ClearOnShutdown.cpp
@@ +9,5 @@
>  namespace mozilla {
>  namespace ClearOnShutdown_Internal {
>  
>  bool sHasShutDown = false;
> +StaticAutoPtr<LinkedList<ShutdownObserver>> sShutdownObservers[mozilla::ShutdownPhase_Length];

Let's make this

Array<StaticAutoPtr<LinkedList<ShutdownObserver>>, mozilla::NumShutdownPhases>

which is a mouthful, but enables bounds checking in debug builds.  Will need to #include "mozilla/Array.h", of course.

We may want to typedef LinkedList<ShutdownObserver> to something to make various places a bit shorter.

::: xpcom/base/ClearOnShutdown.h
@@ +20,5 @@
>   * This function takes a pointer to a smart pointer and nulls the smart pointer
> + * on shutdown (and a particular phase of shutdown as needed).  If a phase
> + * is specified, the ptr will be cleared at the start of that phase.  Also,
> + * if a phase has already occurred when ClearOnShutdown() is called, the
> + * pointer will be cleared on the next phase, or SHUTDOWN_FINAL.

This documentation will need to be updated for the suggested changes below.

@@ +40,5 @@
>  
>  namespace mozilla {
> +
> +// Must be contiguous starting at 0
> +enum ShutdownPhase {

Let's make this an |enum class| for type guarantees.  I have a slight preference for calling this XPCOMShutdownPhase, but I can go either way.

I think making this an enum class makes the callsites more verbose, but I'm not sure that's a bad thing.  Feel free to push back if you think the ergonomics on the |enum class| are awful, and we can make it a plain |enum|.

@@ +42,5 @@
> +
> +// Must be contiguous starting at 0
> +enum ShutdownPhase {
> +  PROFILE_CHANGE = 0,
> +  WILL_SHUTDOWN,

Let's call this WillShutdown, and so on throughout the enum names.  Also, PROFILE_CHANGE appears to be unused...is that an oversight?

@@ +48,5 @@
> +  SHUTDOWN_THREADS,
> +  SHUTDOWN_LOADERS,
> +  SHUTDOWN_FINAL
> +};
> +static const int ShutdownPhase_Length = SHUTDOWN_FINAL+1;

Usually this is done with an enum at the end; I guess you just didn't want people accidentally passing NumShutdownPhases to ClearOnShutdown?  I don't think that's much of a problem (and we can catch that in DEBUG builds), so let's make this part of the enum.

@@ +111,5 @@
>  
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // Out of caution, when we clear a phase, clear all "older" phases in
> +  // case something gets re-added to them after we passed that phase.

I applaud the caution, but I think doing something like that indicates a bug, and we should prohibit that sort of thing.  So we should instead check that all previous phases are empty.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 3 • 9 years ago

Should all be done; note I have minor concern about the static Array<> causing a bump in the number of static constructors, but that's minor.  I looked into making the phases iterable with c++11 iterators; lots of pain without much cost.  We could make an iterable enum class template (similar to some I saw proposed on stackoverflow) if we care for future uses

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 4 • 9 years ago

Comment on attachment 8707047 [details] [diff] [review]
Extend ClearOnShutdown() to allow specifying the shutdown phase

f? to glandium about the static construction question

	Nathan Froyd [:froydnj]
	Comment 5 • 9 years ago

Comment on attachment 8707047 [details] [diff] [review]
Extend ClearOnShutdown() to allow specifying the shutdown phase

Review of attachment 8707047 [details] [diff] [review]:
-----------------------------------------------------------------

Your modifications have raised some more questions.  Also, do you have an example of use?  Without the ability to remove observers, I'm wondering how much this thing will get used.

StaticAutoPtr doesn't create static constructors, and Array won't either (assuming its stored type has trivial or constexpr constructors), so I don't think there's a problem here.

::: xpcom/base/ClearOnShutdown.h
@@ +123,5 @@
> +       phase <= static_cast<size_t>(aPhase);
> +       phase++) {
> +    if (phase != static_cast<size_t>(aPhase)) {
> +      // Because I'm extra paranoid make this assert always
> +      MOZ_RELEASE_ASSERT(!sShutdownObservers[static_cast<size_t>(phase)]);

How debuggable do you think this assert is?  I'm a bit worried that people will crash here, but we don't have any good way to tell which bit of code added the bogus observer.

One way to make this more debuggable would be to define ShutdownPhase like so:

enum class ShutdownPhase {
  NotInShutdown,
  // everything else as before
};

static ShutdownPhase sCurrentShutdownPhase = NotInShutdown;

and have KillClearOnShutdown set |sCurrentShutdownPhase| to its argument.  (This variable would also obviate the need for sHasShutdown.)  Then ClearOnShutdown could MOZ_RELEASE_ASSERT that we're never adding something prior to the current shutdown phase.  We could leave this assert here, but it could be downgraded to a MOZ_ASSERT instead.

This method has the virtue of pointing us directly to the offending observer, rather than leaving us to scratch our heads about what's triggering the problem.  WDYT?

Also, beyond the straightforward logic error, what sort of problems would this assert catch?  Is there harm to the user that could result?

@@ +124,5 @@
> +       phase++) {
> +    if (phase != static_cast<size_t>(aPhase)) {
> +      // Because I'm extra paranoid make this assert always
> +      MOZ_RELEASE_ASSERT(!sShutdownObservers[static_cast<size_t>(phase)]);
> +    } else {

Style nit: let's make this:

if (phase != aPhase) {
  MOZ_ASSERT(...);
  continue;
}

if (has shutdown observers) {
  ...
}

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 6 • 9 years ago

> Your modifications have raised some more questions.  Also, do you have an
> example of use?  Without the ability to remove observers, I'm wondering how
> much this thing will get used.

As with ClearOnShutdown() today, it's mostly used for statics. Adding removal would be a useful thing, but a follow-on independent of when they fire.

> StaticAutoPtr doesn't create static constructors, and Array won't either
> (assuming its stored type has trivial or constexpr constructors), so I don't
> think there's a problem here.

Good; wasn't sure about Array().

> ::: xpcom/base/ClearOnShutdown.h
> @@ +123,5 @@
> > +       phase <= static_cast<size_t>(aPhase);
> > +       phase++) {
> > +    if (phase != static_cast<size_t>(aPhase)) {
> > +      // Because I'm extra paranoid make this assert always
> > +      MOZ_RELEASE_ASSERT(!sShutdownObservers[static_cast<size_t>(phase)]);
> 
> How debuggable do you think this assert is?  I'm a bit worried that people
> will crash here, but we don't have any good way to tell which bit of code
> added the bogus observer.

Well, it's trivially debuggable with rr, and pretty easy with gdb - just breakpoint KillClearOnShutdown, and when you get there the first time, breakpoint ClearOnShutdown.  I did that to test.  But that requires a reproducible sequence.

> One way to make this more debuggable would be to define ShutdownPhase like
> so:
> 
> enum class ShutdownPhase {
>   NotInShutdown,
>   // everything else as before
> };
> 
> static ShutdownPhase sCurrentShutdownPhase = NotInShutdown;
> 
> and have KillClearOnShutdown set |sCurrentShutdownPhase| to its argument. 
> (This variable would also obviate the need for sHasShutdown.)  Then
> ClearOnShutdown could MOZ_RELEASE_ASSERT that we're never adding something
> prior to the current shutdown phase.  We could leave this assert here, but
> it could be downgraded to a MOZ_ASSERT instead.

We could do that, sure.  Also - leaving something alive *usually* isn't a sec risk, excluding things like threads, so MOZ_ASSERT should be ok.

> This method has the virtue of pointing us directly to the offending
> observer, rather than leaving us to scratch our heads about what's
> triggering the problem.  WDYT?

Sure.

> Also, beyond the straightforward logic error, what sort of problems would
> this assert catch?  Is there harm to the user that could result?

Mostly sources for leaks.  Note that in my first patch I just re-cleared all older phases.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 7 • 9 years ago

I added profile-change notifications; I'm fairly certain they don't happen in content processes, so it handles that.  Also in theory (maybe in practice in SeaMonkey?) you can have more than one profile change, so I allow that.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 8 • 9 years ago

example of a use for the ShutdownPhase (where the old ClearOnShutdown didn't work)

	Nathan Froyd [:froydnj]
	Comment 9 • 9 years ago

Comment on attachment 8707499 [details] [diff] [review]
Extend ClearOnShutdown() to allow specifying the shutdown phase

Review of attachment 8707499 [details] [diff] [review]:
-----------------------------------------------------------------

Not really happy about the ProfileChange bits; does anything really register things for ProfileChange ClearOnShutdown?  If not, I'd prefer to eliminate that bit and keep this more XPCOM-focused.

Do file a followup bug for out-of-lining things in ClearOnShutdown.h now that they've become a bit more complicated.

::: xpcom/base/ClearOnShutdown.h
@@ +119,5 @@
>  
>  // Called when XPCOM is shutting down, after all shutdown notifications have
>  // been sent and after all threads' event loops have been purged.
>  inline void
> +KillClearOnShutdown(ShutdownPhase aPhase)

Sorry for missing this the first two times, but if we're going to be calling this from multiple places, especially outside of a single translation unit, this function should live in ClearOnShutdown.cpp.  I guess I'm OK with filing a followup bug for that if you want to this landed.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 10 • 9 years ago

(In reply to Nathan Froyd [:froydnj] from comment #9)
> Comment on attachment 8707499 [details] [diff] [review]
> Extend ClearOnShutdown() to allow specifying the shutdown phase
> 
> Review of attachment 8707499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not really happy about the ProfileChange bits; does anything really register
> things for ProfileChange ClearOnShutdown?  If not, I'd prefer to eliminate
> that bit and keep this more XPCOM-focused.

A quick survey of profile-* users shows a couple, like nsLayoutStylesheetCache, and imgLoader.  Both are clearing mFoo fields in a singleton.  Certainly we don't need it, so I've removed the firing of those.  I'll remove it for now, and leave it in a separate patch we can consider later if we want.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 11 • 9 years ago

Now with ProfileChange removed, and also updated the comment block

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 12 • 9 years ago

Record this for future use if we want

	Nathan Froyd [:froydnj]
	Comment 13 • 9 years ago

Comment on attachment 8707562 [details] [diff] [review]
Extend ClearOnShutdown() to allow specifying the shutdown phase

Review of attachment 8707562 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce37c190703

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6ce37c190703