579236 - [e10s] Shutdown crash and other problems with ContentChild pref observers

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Problems I fixed

 - ContentChild::mPrefObservers can hold the last ref to its observers, and some of those observers try to unregister themselves on deletion if they're still registered (apparently).  This caused crashes on shutdown when we Clear() the observer array.

 - The class formerly known as nsPrefObserverStorage claimed to support weak refs to observers, but always kept a strong ref regardless.

 - RemoveRemotePrefObserver() removed all observers matching <observer, domain, *>.  I don't understand very well the relationship of domain and prefRoot, but it seems to me that the match should have been against <observer, domain, prefRoot>.  Maybe Dan can comment on this?

 - AddRemotePrefObserver() doesn't prevent duplicate observers and RemoveRemotePrefObserver() didn't take them into consideration.  I don't consider this a big deal, but as there are no comments to the effect of "don't do that", it should be guarded against.

Problems I found but didn't fix

 - Add/RemovePrefObserver() aren't re-entrant and don't guard against re-entrancy.  I'm OK with leaving this bug in until it shows up in a scenario we can't fix.

 - RemovePrefObserver() and RecvNotifyRemotePrefObserver() are both O(n) in the length of ContentChild::mPrefObservers.  I suspect that this has the potential to be Very Bad in the field, but again am comfortable with fixing that "later".  It wouldn't be hard to roll a smarter observer container.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Forgot to add: blocking nom because this causes very frequent shutdown crashes, >50% of the time I would guess.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix shutdown crash and lesser bugs with remote pref observers

Comment 3

[[e-mail]]

Comment on attachment 457740 [details] [diff] [review]
Fix shutdown crash and lesser bugs with remote pref observers

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

>+class PrefObserver
>+{
>+public:
>+    /**
>+     * Pass |aHoldWeak=true| to force this to hold a weak ref to
>+     * |aObserver|.  Otherwise, this holds a strong ref.
>+     *
>+     * XXX/cjones: what do domain and prefRoot mean?

'domain' is the leaf name; 'prefRoot' is the prefix. (But it looks like you figured that out below).

>+    PrefObserver(nsIObserver *aObserver, bool aHoldWeak,
>+                 const nsACString& aPrefRoot, const nsACString& aDomain)

I prefer the style of using nsCString& instead of nsACString& where possible, but up to you.

>+        : mPrefRoot(aPrefRoot)
>+        , mDomain(aDomain)
>+    {
>+        if (aHoldWeak) {
>+            nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>+                do_QueryInterface(aObserver);

Factory is sooo 90's.

>+    {
>+        nsCOMPtr<nsIObserver> observer = GetObserver();
>+        return (observer == aObserver &&
>+                mDomain == aDomain && mPrefRoot == aPrefRoot);
>+    }
>+
>+    /**
>+     * Return true iff this should be notified of changes to |aPref|.
>+     */
>+    bool Observes(const nsACString& aPref) const

Same.

>+ContentChild::RemoveRemotePrefObserver(const nsCString& aDomain, 
>+                                       const nsCString& aPrefRoot, 
>+                                       nsIObserver* aObserver)
> {
>-    if (mPrefObserverArray.IsEmpty())
>+    if (mDead) {
>+        // Silently ignore, we're about to exit.  See comment in
>+        // ActorDestroy().
>         return NS_OK;
>+    }
> 
>-    nsPrefObserverStorage *entry;
>-    for (PRUint32 i = 0; i < mPrefObserverArray.Length(); ++i) {
>-        entry = mPrefObserverArray[i];
>-        if (entry && entry->GetObserver() == aObserver &&
>-                     entry->GetDomain().Equals(aDomain)) {
>-            // Remove this observer from our array
>-            mPrefObserverArray.RemoveElementAt(i);
>-            return NS_OK;
>+    for (PRUint32 i = 0; i < mPrefObservers.Length();
>+         /*we mutate the array during the loop; ++i iff no mutation*/) {
>+        PrefObserver* observer = mPrefObservers[i];
>+        if (observer->IsDead() ||
>+            observer->ShouldRemoveFrom(aObserver, aPrefRoot, aDomain)) {
>+            mPrefObservers.RemoveElementAt(i);
>+            continue;
>         }
>+        ++i;
>     }
>-    NS_WARNING("No preference Observer was matched !");
>-    return NS_ERROR_UNEXPECTED;

You're changing this from returning after a single removal to potentially having multiple removals. I suppose that's OK since if someone does try to remove themselves multiple times, successive removals will be nops and succeed just fine.

r=dwitte

Comment 4

[[e-mail]]

(In reply to comment #3)
> Same.

Whoops, I meant re nsACString& here.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> Comment on attachment 457740 [details] [diff] [review]
> Fix shutdown crash and lesser bugs with remote pref observers
> 
> >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> 
> >+class PrefObserver
> >+{
> >+public:
> >+    /**
> >+     * Pass |aHoldWeak=true| to force this to hold a weak ref to
> >+     * |aObserver|.  Otherwise, this holds a strong ref.
> >+     *
> >+     * XXX/cjones: what do domain and prefRoot mean?
> 
> 'domain' is the leaf name; 'prefRoot' is the prefix. (But it looks like you
> figured that out below).
> 

OK.  I added a comment to |AddRemotePrefObserver()| to this effect.

> >+    PrefObserver(nsIObserver *aObserver, bool aHoldWeak,
> >+                 const nsACString& aPrefRoot, const nsACString& aDomain)
> 
> I prefer the style of using nsCString& instead of nsACString& where possible,
> but up to you.
> 

Changed.

> >+        : mPrefRoot(aPrefRoot)
> >+        , mDomain(aDomain)
> >+    {
> >+        if (aHoldWeak) {
> >+            nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
> >+                do_QueryInterface(aObserver);
> 
> Factory is sooo 90's.
> 

Heh.  Actually, looking back over this, I think renaming this to |supportsWeakRef| might make the code a little clearer.

> >+    {
> >+        nsCOMPtr<nsIObserver> observer = GetObserver();
> >+        return (observer == aObserver &&
> >+                mDomain == aDomain && mPrefRoot == aPrefRoot);
> >+    }
> >+
> >+    /**
> >+     * Return true iff this should be notified of changes to |aPref|.
> >+     */
> >+    bool Observes(const nsACString& aPref) const
> 
> Same.
> 

Done.

> >+ContentChild::RemoveRemotePrefObserver(const nsCString& aDomain, 
> >+                                       const nsCString& aPrefRoot, 
> >+                                       nsIObserver* aObserver)
> > {
> >-    if (mPrefObserverArray.IsEmpty())
> >+    if (mDead) {
> >+        // Silently ignore, we're about to exit.  See comment in
> >+        // ActorDestroy().
> >         return NS_OK;
> >+    }
> > 
> >-    nsPrefObserverStorage *entry;
> >-    for (PRUint32 i = 0; i < mPrefObserverArray.Length(); ++i) {
> >-        entry = mPrefObserverArray[i];
> >-        if (entry && entry->GetObserver() == aObserver &&
> >-                     entry->GetDomain().Equals(aDomain)) {
> >-            // Remove this observer from our array
> >-            mPrefObserverArray.RemoveElementAt(i);
> >-            return NS_OK;
> >+    for (PRUint32 i = 0; i < mPrefObservers.Length();
> >+         /*we mutate the array during the loop; ++i iff no mutation*/) {
> >+        PrefObserver* observer = mPrefObservers[i];
> >+        if (observer->IsDead() ||
> >+            observer->ShouldRemoveFrom(aObserver, aPrefRoot, aDomain)) {
> >+            mPrefObservers.RemoveElementAt(i);
> >+            continue;
> >         }
> >+        ++i;
> >     }
> >-    NS_WARNING("No preference Observer was matched !");
> >-    return NS_ERROR_UNEXPECTED;
> 
> You're changing this from returning after a single removal to potentially
> having multiple removals. I suppose that's OK since if someone does try to
> remove themselves multiple times, successive removals will be nops and succeed
> just fine.
> 

Per IRC chat, going back to single-removal semantics here.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Running with stricter matching and the restored warning on fail-to-remove, I see a bunch of

WARNING: RemoveRemotePrefObserver(): no observer was matched!: file /home/cjones/mozilla/mozilla-central/dom/ipc/ContentChild.cpp, line 324

on stdout.  Will investigate tomorrow.

Comment 7

[Josh Matthews [:jdm]]

To my knowledge, that's not a regression from your changes.  I remember seeing those warnings in clumps since the pref changes landed previously.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The warnings can be fixed in a followup.

http://hg.mozilla.org/projects/electrolysis/rev/9028bde53425

Comment 9

[Josh Matthews [:jdm]]

I dug into the warnings: they're a consequence of calling ClearPrefObservers() in ActorDestroy.  Some of the observers themselves end up trying to remove observers when they die, as demonstrated in this backtrace:

#0  mozilla::dom::ContentProcessChild::RemoveRemotePrefObserver (this=0xb762d038, aDomain=..., aPrefRoot=..., aObserver=0xb761045c) at /home/t_mattjo/src/firefox/mobilebase/dom/ipc/ContentProcessChild.cpp:197
#1  0x013f409c in nsPrefBranch::RemoveObserver (this=0xb76ae580, aDomain=0x2b3dc20 "accessibility.accesskeycausesactivation", aObserver=0xb761045c)
    at /home/t_mattjo/src/firefox/mobilebase/modules/libpref/src/nsPrefBranch.cpp:781
#2  0x013f8fa4 in nsPrefService::RemoveObserver (this=0xb76ae550, aDomain=0x2b3dc20 "accessibility.accesskeycausesactivation", aObserver=0xb761045c)
    at /home/t_mattjo/src/firefox/mobilebase/modules/libpref/src/nsPrefService.h:61
#3  0x018ba35e in nsEventStateManager::Shutdown (this=0xb7610450) at /home/t_mattjo/src/firefox/mobilebase/content/events/src/nsEventStateManager.cpp:883
#4  0x018ba10d in nsEventStateManager::~nsEventStateManager (this=0xb7610450, __in_chrg=<value optimized out>) at /home/t_mattjo/src/firefox/mobilebase/content/events/src/nsEventStateManager.cpp:862
#5  0x018ba2fd in nsEventStateManager::~nsEventStateManager (this=0xb7610450, __in_chrg=<value optimized out>) at /home/t_mattjo/src/firefox/mobilebase/content/events/src/nsEventStateManager.cpp:875
#6  0x018baa67 in nsEventStateManager::Release (this=0xb7610450) at /home/t_mattjo/src/firefox/mobilebase/content/events/src/nsEventStateManager.cpp:977
#7  0x0127abd8 in nsCOMPtr<nsIObserver>::~nsCOMPtr (this=0xb41c59c0, __in_chrg=<value optimized out>) at ../../dist/include/nsCOMPtr.h:533
#8  0x0249f54b in mozilla::dom::ContentProcessChild::nsPrefObserverStorage::~nsPrefObserverStorage (this=0xb41c59c0, __in_chrg=<value optimized out>)
    at /home/t_mattjo/src/firefox/mobilebase/dom/ipc/ContentProcessChild.h:86
#9  0x0249fd28 in nsAutoPtr<mozilla::dom::ContentProcessChild::nsPrefObserverStorage>::~nsAutoPtr (this=0xb7647360, __in_chrg=<value optimized out>) at ../../dist/include/nsAutoPtr.h:104
#10 0x0249fcae in nsTArrayElementTraits<nsAutoPtr<mozilla::dom::ContentProcessChild::nsPrefObserverStorage> >::Destruct (e=0xb7647360) at ../../dist/include/nsTArray.h:204
#11 0x0249fc29 in nsTArray<nsAutoPtr<mozilla::dom::ContentProcessChild::nsPrefObserverStorage> >::DestructRange (this=0xb762d1b4, start=0, count=279) at ../../dist/include/nsTArray.h:987
#12 0x0249fa7b in nsTArray<nsAutoPtr<mozilla::dom::ContentProcessChild::nsPrefObserverStorage> >::RemoveElementsAt (this=0xb762d1b4, start=0, count=279) at ../../dist/include/nsTArray.h:718
#13 0x0249f802 in nsTArray<nsAutoPtr<mozilla::dom::ContentProcessChild::nsPrefObserverStorage> >::Clear (this=0xb762d1b4) at ../../dist/include/nsTArray.h:729
#14 0x0249f7d8 in mozilla::dom::ContentProcessChild::ClearPrefObservers (this=0xb762d038) at /home/t_mattjo/src/firefox/mobilebase/dom/ipc/ContentProcessChild.h:159

Comment 10

[Josh Matthews [:jdm]]

Ignore the previous message - it looks like the warnings come from real, valid removal requests, but the removal loop automatically removes dead observers it finds, including the one that is supposed to be removed.