Closed Bug 579236 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Preferences: Backend, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file)

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.
Forgot to add: blocking nom because this causes very frequent shutdown crashes, >50% of the time I would guess.
Assignee: nobody → jones.chris.g
Attachment #457740 - Flags: review?(dwitte)
tracking-fennec: ? → 2.0a1+
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
Attachment #457740 - Flags: review?(dwitte) → review+
(In reply to comment #3)
> Same.

Whoops, I meant re nsACString& here.
(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.
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.
To my knowledge, that's not a regression from your changes.  I remember seeing those warnings in clumps since the pref changes landed previously.
The warnings can be fixed in a followup.

http://hg.mozilla.org/projects/electrolysis/rev/9028bde53425
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
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.
You need to log in before you can comment on or make changes to this bug.