506269 - Electrolysis: implement IPC remoting for preferences

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Benjamin Smedberg]

Remote preference calls from the child process to the parent process: prefs access should be readonly: preference observers should work

Comment 1

[Frederic Plourde (:fred23)]

Attachment: W.I.P patch v1.0

This is a w.i.p patch that is not to be officially reviewed, but looked at and commented. For some joy with HG, apply this patch on top of this e10s-changeset : 605d093281ba

note : Get/SetComplexValue functions are yet to be done/tested.
note : I inserted some questions/comments in the code about whether or not we should assert here and there... I'll need your input on those. 

thanks !

Comment 2

[Frederic Plourde (:fred23)]

k guys, there's a chance that you might not be especially glad to apply the patch on a old revision, so I'll make it easy and update the patch on tip for you and post v2.0.

Comment 3

[Frederic Plourde (:fred23)]

Wow!, As of this morning, patch v1.0 *does* apply correctly on tip revision (with some fuzz and offsets). but it works. No need to post patch v2.0 for now.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: W.I.P patch v2.0

w.i.p patch v2.0 that does away with all "SET" operations and such, leading to a readonly remote pref system.

Comment 5

[Frederic Plourde (:fred23)]

Attachment: patch v.1

First patch to review. I've tested it extensively, It looks complete.
thanks!

Comment 6

[Frederic Plourde (:fred23)]

Attachment: TESTpatch for v1.0

This patch adds some chrome on both sides (chrome and content) to show/demonstrate that prefs are correctly read from the parent side, even if initiated from Content. 

to use it, do :
 1- Add the following pref to <user>/<...>/mozilla/Firefox/profile/prefs.js
    "user_pref("browser.download.manager.showAlertInterval", 23);"

 2- With an 'unpatched' version of e10s (no patch v1.0 and no TESTpatch), Apply the TESTpatch and rebuild

 3- Run e10s and press both buttons "pref from Content" and "pref from Chrome" and notice they return different pref values.

 4- apply patch v1.0 on top of your stack and rebuild

 5- Run e10s and press the buttons again. They both should return the same value, taken from your system user prefs!

Comment 7

[Benjamin Smedberg]

Fred, the best way to test this is to use an automated xpcshell test. We've already added a new methods to the xpcshell test framework, "sendCommand", which can be called two ways:

sendCommand("JS to eval remotely"); // called with RPC semantics, blocks until the JS is done

sendCommand("JS to eval remotely", function(result) {
  // function to notify when the remote code is done
});
// returns immediately, remote code runs asynchronously and the result is returned later.

This function is typically used to load() a script to perform actual work, see http://mxr.mozilla.org/projects-central/source/electrolysis/ipc/testshell/tests/ for our only in-tree example: test_ipcshell.js is what runs in the parent process, it load()s test_ipcshell_child.js and calls a function in it.

When we created this function, prefs were one of the obvious cases where we could use it, see http://groups.google.com/group/mozilla.dev.tech.dom/browse_thread/thread/5c2e4c5926cfe010?pli=1

See https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests for information about writing xpcshell tests in general.

Comment 8

[Frederic Plourde (:fred23)]

yeah, I posted that little test patch just for enthusiasts who wanted to see it workin' ;-) However, I haven't wasted time on it since this patch was my working testing patch when developing the whole thing. But I'll make more official xpcshell test(s) for sure! thanks.

Comment 9

[Frederic Plourde (:fred23)]

Attachment: pref Observers W.I.P patch

I'm going on vacation for last week of December, so here's a W.I.P patch for pref observers. Basically, it's working okay (apart from some bugs... e.g. if the observer is a .js function in an nsXPTCStubBase object, it crashes).

If you have time, I'd like to get your general input about it. When I'll get back to it beg. 2010, I'd like to push all that forward and attack some other more urgent stuff.

note : the patch applies on top of the other patches attached above.

Comment 10

[Frederic Plourde (:fred23)]

Attachment: patch v.2

RemotePrefs patch WITH remote prefs observers.

Comment 11

[Oleg Romashin (:romaxa)]

Last patch is not applying on latest e10s branch, could you provide some updated version?

Comment 12

[Frederic Plourde (:fred23)]

Attachment: patch v.3

Fixed bitrot.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 434269 [details] [diff] [review]
patch v.3


>+nsresult
>+ContentProcessChild::AddRemotePrefObserver(nsCString aDomain, 
>+                                           nsCString aPrefRoot, 
>+                                           nsIObserver *aObserver, 
>+                                           PRBool aHoldWeak)
>+{
>+    // keep references to remote observers
>+    if (aHoldWeak) {
>+        nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>+                                                 do_QueryInterface(aObserver);
>+        if (!weakRefFactory) {
>+            // the caller didn't give us a object that supports weak reference
>+            return NS_ERROR_INVALID_ARG;
>+        }
>+    }
>+    mPrefObserverArray.AppendElement(
>+         new nsPrefObserverStorage(aObserver, aDomain, aPrefRoot, aHoldWeak));
I think you must return something here...
also where is new alloc check?

>+        bool NotifyObserver() {
>+            nsCOMPtr<nsIObserver> observer;
>+            if (this->mWeakRef) {
Is it really good style to use "this->" ?

>+                observer = do_QueryReferent(this->mWeakRef);
>+                if (!observer) {
>+                    // this weak referenced observer went away, tell
>+                    // the caller so he can remove the observer from the list
>+                    return false;
>+                }
>+            } else {
>+                observer = this->mObserver;
>+            }
>+
>+            nsCOMPtr<nsIPrefBranch> prefBranch;
>+            nsCOMPtr<nsIPrefService> prefService = 
>+                                      do_GetService(NS_PREFSERVICE_CONTRACTID);
>+            if (prefService) {
>+                prefService->GetBranch(this->mPrefRoot.get(), 
>+                                                   getter_AddRefs(prefBranch));
>+                observer->Observe(prefBranch, "nsPref:changed",
>+                                   NS_ConvertUTF8toUTF16(this->mDomain).get());
>+            }

I think you must return something here, otherwise it just not compiling for me

Comment 14

[Frederic Plourde (:fred23)]

Attachment: patch v.4

Patch corrected per Oleg's comments. thanks !

Comment 15

[[e-mail]]

Comment on attachment 435662 [details] [diff] [review]
patch v.4

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

>@@ -85,8 +86,8 @@ ContentProcessChild::Init(MessageLoop* a

>+    sSingleton = this;
>     Open(aChannel, aParentHandle, aIOLoop);
>-    sSingleton = this;

Why this change?

>+nsresult
>+ContentProcessChild::AddRemotePrefObserver(nsCString aDomain, 
>+                                           nsCString aPrefRoot, 

These need to be nsCString&. Same with all the other prototypes.

>+    if (!newObserver)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    if (!mPrefObserverArray.AppendElement(newObserver))
>+        return NS_ERROR_FAILURE;

We have infallible 'new' now, so you can ditch the failure checks here.

>+nsresult
>+ContentProcessChild::RemoveRemotePrefObserver(nsCString aDomain, 

>+    // need to find the index of observer, 
>+    // so we can remove it from the domain list too
>+    nsPrefObserverStorage *entry;
>+    while (i < mPrefObserverArray.Length()) {
>+        entry = mPrefObserverArray[i];
>+        if (entry && entry->GetObserver() == aObserver &&
>+                     entry->GetDomain().Equals(aDomain)) {
>+            // Remove this observer from our array
>+            mPrefObserverArray.RemoveElementAt(i);
>+            delete entry;

A cleaner way to do this would be to declare mPrefObserverArray as:

  nsTArray< nsAutoPtr<nsPrefObserverStorage> > mPrefObserverArray;

and then you don't need to 'delete' when removing elements from the array.

I also worry about how you're removing all entries from the array that match the observer and domain. Ideally, one AddRemotePrefObserver() call should be matched with one RemoveRemotePrefObserver() call.

It seems that nsPrefBranch matches one add with one remove, but accepts multiple adds of the same observer:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#582
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#627

So let's match its behavior here, and just remove the first entry we hit in RemoveRemotePrefObserver(). You could also assert if no matching entry was found, since that indicates a programmatic mistake.

>+nsresult
>+ContentProcessChild::ClearPrefObservers()
>+{
>+    nsPrefObserverStorage *entry;
>+    // no need to enumerate the entries... it's faster to delete from the end
>+    for (int i=mPrefObserverArray.Length()-1;i>=0;i--) {
>+        entry = mPrefObserverArray[i];
>+        mPrefObserverArray.RemoveElementAt(i);
>+        delete entry;

Similarly, here you could delete this entire function, and just call Clear() on the array, which will call nsAutoPtr's dtor on each entry and delete them.

>+bool
>+ContentProcessChild::RecvNotifyRemotePrefObserver(
>+        const nsCString& aDomain)
>+{

Indenting is off.

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

>+bool
>+ContentProcessParent::RecvGetPrefType(const nsCString& prefName,
>+        PRInt32* retValue, nsresult* rv)
>+{
>+    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+    if (prefs) {
>+        *rv = prefs->GetPrefType(prefName.get(), retValue);
>+    } else {
>+        *rv = NS_ERROR_UNEXPECTED;
>+    }

One thing you need to make sure of is that both 'retValue' and 'rv' are always written to -- this is more important here than ever, since they're both guaranteed to be read in order to serialize them back to the child. Not doing so will generate a valgrind error, and could result in a security vulnerability, since random data is being sent parent -> child.

For result types like nsCString, the default constructor (called when IPDL instantiates it on the stack, in preparation for passing into the Recv method) will initialize itself appropriately. For POD types, like PRBool, PRInt32, nsresult etc, this won't happen.

So, you could make this:

    *retValue = 0;

    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, rv));
    if (prefs)
      *rv = prefs->GetPrefType(prefName.get(), retValue);

    return true;

Same applies to the other Recv methods.

>+bool
>+ContentProcessParent::RecvSecurityGetBoolPref(const nsCString& prefName,
>+                                  PRBool* retValue, nsresult* rv)

Ugh. This whole Security* thing is stupid. We need to kill it off. I'm pretty sure that's fine to do; I'll file another bug on it.

For now, I think you should pretend it doesn't exist. Get rid of these RecvSecurity* methods, and in the senders have them call the regular Send* methods.

>@@ -164,12 +311,31 @@ ContentProcessParent::Observe(nsISupport

>+    // listening for remotePrefs...
>+    if (!strcmp(aTopic, "nsPref:changed")) {
>+        nsString wstrData(aData);
>+        nsCString strData;
>+        strData.Assign(NS_ConvertUTF16toUTF8(wstrData));

  NS_ConvertUTF16toUTF8 strData(aData);

will accomplish the same thing. However, you can use NS_ConvertUTF16toASCII, since nsPrefBranch deals in ASCII.

>+        // TODO: get the prefNAme here and pass the whole prefName
>+        SendNotifyRemotePrefObserver(strData);

Isn't 'strData' the domain that's being modified? (In most cases, the full pref name, but the domain if an entire branch f.e. is being deleted.) In which case, it's the correct thing to pass, and your StringBeginsWith() check in the Recv method is also correct. Why the comment about passing prefName?

>diff --git a/modules/libpref/src/nsPrefBranch.cpp b/modules/libpref/src/nsPrefBranch.cpp

>@@ -141,6 +144,22 @@ NS_IMETHODIMP nsPrefBranch::GetPrefType(

>+    pref = getPrefName(aPrefName);
>+    nsCString prefName(pref, strlen(pref));
>+    if (cpc->SendGetPrefType(prefName, _retval, &rv)) {

No need for the failure check here -- the content process will be terminated if Send fails.

You should also be able to use nsDependentCString instead of nsCString here:

    nsDependentCString prefName(pref);

It will do the 'strlen' for you, and should autocast to nsCString for the Send call.

Same comments apply to the other Get methods below.

>@@ -154,6 +173,25 @@ NS_IMETHODIMP nsPrefBranch::GetBoolPref(

>+    PRBool retval;
>+    pref = getPrefName(aPrefName);
>+    nsCString prefName(pref, strlen(pref));
>+    if (cpc->SendGetBoolPref(prefName, &retval, &rv)) {
>+      if (NS_SUCCEEDED(rv))
>+        *_retval = retval;

You should use the conditional write to _retval in GetPrefType() above, too.

>@@ -178,6 +216,28 @@ NS_IMETHODIMP nsPrefBranch::GetCharPref(

>+    pref = getPrefName(aPrefName);
>+    nsCString prefName(pref, strlen(pref));
>+    nsCString prefValue;
>+    if (cpc->SendGetCharPref(prefName, &prefValue, &rv)) {
>+      if (NS_SUCCEEDED(rv)) {
>+        char *retval;
>+        retval = strdup(prefValue.get());
>+        *_retval = retval;

You can use nsCAutoString for prefValue, and also simplify those three lines down to

    *_retval = strdup(prefValue.get());

>@@ -243,7 +322,10 @@ NS_IMETHODIMP nsPrefBranch::GetComplexVa

>-        if (!PREF_HasUserPref(pref) && !PREF_PrefIsLocked(pref)) {
>+        PRBool hasUserValue, isLocked;
>+        PrefHasUserValue(pref, &hasUserValue);
>+        PrefIsLocked(pref, &isLocked);

Why this change? 'pref' is already a validated pref name, so this seems like it's doing unnecessary work.

> static nsresult NotifyObserver(const char *newpref, void *data)
> {
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    // The chrome process is notifying content prefs observers through IPDL
>+    return NS_OK;

We shouldn't ever get here, since we never register NotifyObserver in the content process. You should use an NS_NOTREACHED here.

>diff --git a/modules/libpref/src/nsPrefBranch.h b/modules/libpref/src/nsPrefBranch.h

>+#ifdef MOZ_IPC
>+#include "nsXULAppAPI.h"
>+#endif

Can you do this in nsPrefBranch.cpp instead?

>diff --git a/modules/libpref/src/prefapi.h b/modules/libpref/src/prefapi.h

>-NSPR_BEGIN_EXTERN_C
>+//NSPR_BEGIN_EXTERN_C
>+extern "C" {

Why this change?

Those are my comments for now -- I can take another pass when you've got a new patch.

Comment 16

[[e-mail]]

Er, I meant NS_LossyConvertUTF16toASCII above.

Comment 17

[[e-mail]]

(In reply to comment #15)
> >@@ -243,7 +322,10 @@ NS_IMETHODIMP nsPrefBranch::GetComplexVa
> 
> >-        if (!PREF_HasUserPref(pref) && !PREF_PrefIsLocked(pref)) {
> >+        PRBool hasUserValue, isLocked;
> >+        PrefHasUserValue(pref, &hasUserValue);
> >+        PrefIsLocked(pref, &isLocked);
> 
> Why this change? 'pref' is already a validated pref name, so this seems like
> it's doing unnecessary work.

Followup comment: if you're trying to make GetComplexValue remotable, I think you're missing a remoting implementation of:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#256

I don't like how this is going to generate multiple IPC calls in some cases (e.g. the PrefHasUserValue/PrefIsLocked example above), but if it's rarely used then it's OK for now. mxr turns up 98 hits, which doesn't exactly strike me as rarely used. And I think nsIPrefLocalizedString, the case here, is a major use case. :(

I'd say you should optimize this one a bit more, by combining them. Maybe a remoted implementation called GetLocalizedString, or something, for just this case. (And maybe others for the remaining cases, if any; I didn't look.) Sound OK?

Comment 18

[Josh Matthews [:jdm]]

For the record, running Fennec with this patch applied cause the child process to crash during component registration.  Apparently some components try to access preferences, grab the (nonexistent) ContentProcessChild singleton and die on null-deref.

Comment 19

[Frederic Plourde (:fred23)]

Thanks Josh, I'm working on that now.

Comment 20

[Frederic Plourde (:fred23)]

Attachment: patch v.5

(In reply to comment #15)
> (From update of attachment 435662 [details] [diff] [review])
> >diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp
> 
> >@@ -85,8 +86,8 @@ ContentProcessChild::Init(MessageLoop* a
> 
> >+    sSingleton = this;
> >     Open(aChannel, aParentHandle, aIOLoop);
> >-    sSingleton = this;
> 
> Why this change?

This change seems irrelevant. I've reverted it.
Done.

> >+nsresult
> >+ContentProcessChild::AddRemotePrefObserver(nsCString aDomain, 
> >+                                           nsCString aPrefRoot, 
> 
> These need to be nsCString&. Same with all the other prototypes.

Done.

> >+    if (!newObserver)
> >+        return NS_ERROR_OUT_OF_MEMORY;
> >+
> >+    if (!mPrefObserverArray.AppendElement(newObserver))
> >+        return NS_ERROR_FAILURE;
> 
> We have infallible 'new' now, so you can ditch the failure checks here.

Done.

> >+nsresult
> >+ContentProcessChild::RemoveRemotePrefObserver(nsCString aDomain, 
> 
> >+    // need to find the index of observer, 
> >+    // so we can remove it from the domain list too
> >+    nsPrefObserverStorage *entry;
> >+    while (i < mPrefObserverArray.Length()) {
> >+        entry = mPrefObserverArray[i];
> >+        if (entry && entry->GetObserver() == aObserver &&
> >+                     entry->GetDomain().Equals(aDomain)) {
> >+            // Remove this observer from our array
> >+            mPrefObserverArray.RemoveElementAt(i);
> >+            delete entry;
> 
> A cleaner way to do this would be to declare mPrefObserverArray as:
> 
>   nsTArray< nsAutoPtr<nsPrefObserverStorage> > mPrefObserverArray;
> 
> and then you don't need to 'delete' when removing elements from the array.
> 
> I also worry about how you're removing all entries from the array that match
> the observer and domain. Ideally, one AddRemotePrefObserver() call should be
> matched with one RemoveRemotePrefObserver() call.
> 
> It seems that nsPrefBranch matches one add with one remove, but accepts
> multiple adds of the same observer:
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#582
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#627
> 
> So let's match its behavior here, and just remove the first entry we hit in
> RemoveRemotePrefObserver(). You could also assert if no matching entry was
> found, since that indicates a programmatic mistake.
 
Done.

> >+nsresult
> >+ContentProcessChild::ClearPrefObservers()
> >+{
> >+    nsPrefObserverStorage *entry;
> >+    // no need to enumerate the entries... it's faster to delete from the end
> >+    for (int i=mPrefObserverArray.Length()-1;i>=0;i--) {
> >+        entry = mPrefObserverArray[i];
> >+        mPrefObserverArray.RemoveElementAt(i);
> >+        delete entry;
> 
> Similarly, here you could delete this entire function, and just call Clear() on
> the array, which will call nsAutoPtr's dtor on each entry and delete them.

Done.

> >+bool
> >+ContentProcessChild::RecvNotifyRemotePrefObserver(
> >+        const nsCString& aDomain)
> >+{
> 
> Indenting is off.

Done.

> >diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
> 
> >+bool
> >+ContentProcessParent::RecvGetPrefType(const nsCString& prefName,
> >+        PRInt32* retValue, nsresult* rv)
> >+{
> >+    nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> >+    if (prefs) {
> >+        *rv = prefs->GetPrefType(prefName.get(), retValue);
> >+    } else {
> >+        *rv = NS_ERROR_UNEXPECTED;
> >+    }
> 
> One thing you need to make sure of is that both 'retValue' and 'rv' are always
> written to -- this is more important here than ever, since they're both
> guaranteed to be read in order to serialize them back to the child. Not doing
> so will generate a valgrind error, and could result in a security
> vulnerability, since random data is being sent parent -> child.
> 
> For result types like nsCString, the default constructor (called when IPDL
> instantiates it on the stack, in preparation for passing into the Recv method)
> will initialize itself appropriately. For POD types, like PRBool, PRInt32,
> nsresult etc, this won't happen.
> 
> So, you could make this:
> 
>     *retValue = 0;
> 
>     nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID,
> rv));
>     if (prefs)
>       *rv = prefs->GetPrefType(prefName.get(), retValue);
> 
>     return true;
> 
> Same applies to the other Recv methods.

Done.

> >+bool
> >+ContentProcessParent::RecvSecurityGetBoolPref(const nsCString& prefName,
> >+                                  PRBool* retValue, nsresult* rv)
> 
> Ugh. This whole Security* thing is stupid. We need to kill it off. I'm pretty
> sure that's fine to do; I'll file another bug on it.
> 
> For now, I think you should pretend it doesn't exist. Get rid of these
> RecvSecurity* methods, and in the senders have them call the regular Send*
> methods.

Done.
 
> >@@ -164,12 +311,31 @@ ContentProcessParent::Observe(nsISupport
> 
> >+    // listening for remotePrefs...
> >+    if (!strcmp(aTopic, "nsPref:changed")) {
> >+        nsString wstrData(aData);
> >+        nsCString strData;
> >+        strData.Assign(NS_ConvertUTF16toUTF8(wstrData));
> 
>   NS_ConvertUTF16toUTF8 strData(aData);
> 
> will accomplish the same thing. However, you can use NS_ConvertUTF16toASCII,
> since nsPrefBranch deals in ASCII.

Done.

> >+        // TODO: get the prefNAme here and pass the whole prefName
> >+        SendNotifyRemotePrefObserver(strData);
> 
> Isn't 'strData' the domain that's being modified? (In most cases, the full pref
> name, but the domain if an entire branch f.e. is being deleted.) In which case,
> it's the correct thing to pass, and your StringBeginsWith() check in the Recv
> method is also correct. Why the comment about passing prefName?

Removing it for now, but I'm still wondering about this one. Had a good reason for doing it, I *think*.
So I'll trace the code and see what's happenning, but for now, I'm going your way on this one.

> >diff --git a/modules/libpref/src/nsPrefBranch.cpp b/modules/libpref/src/nsPrefBranch.cpp
> 
> >@@ -141,6 +144,22 @@ NS_IMETHODIMP nsPrefBranch::GetPrefType(
> 
> >+    pref = getPrefName(aPrefName);
> >+    nsCString prefName(pref, strlen(pref));
> >+    if (cpc->SendGetPrefType(prefName, _retval, &rv)) {
> 
> No need for the failure check here -- the content process will be terminated if
> Send fails.
> 
> You should also be able to use nsDependentCString instead of nsCString here:
> 
>     nsDependentCString prefName(pref);
> 
> It will do the 'strlen' for you, and should autocast to nsCString for the Send
> call.
> 
> Same comments apply to the other Get methods below.

Done.

> >@@ -154,6 +173,25 @@ NS_IMETHODIMP nsPrefBranch::GetBoolPref(
> 
> >+    PRBool retval;
> >+    pref = getPrefName(aPrefName);
> >+    nsCString prefName(pref, strlen(pref));
> >+    if (cpc->SendGetBoolPref(prefName, &retval, &rv)) {
> >+      if (NS_SUCCEEDED(rv))
> >+        *_retval = retval;
> 
> You should use the conditional write to _retval in GetPrefType() above, too.

Done.

> >@@ -178,6 +216,28 @@ NS_IMETHODIMP nsPrefBranch::GetCharPref(
> 
> >+    pref = getPrefName(aPrefName);
> >+    nsCString prefName(pref, strlen(pref));
> >+    nsCString prefValue;
> >+    if (cpc->SendGetCharPref(prefName, &prefValue, &rv)) {
> >+      if (NS_SUCCEEDED(rv)) {
> >+        char *retval;
> >+        retval = strdup(prefValue.get());
> >+        *_retval = retval;
> 
> You can use nsCAutoString for prefValue, and also simplify those three lines
> down to
> 
>     *_retval = strdup(prefValue.get());

Done.

> >@@ -243,7 +322,10 @@ NS_IMETHODIMP nsPrefBranch::GetComplexVa
> 
> >-        if (!PREF_HasUserPref(pref) && !PREF_PrefIsLocked(pref)) {
> >+        PRBool hasUserValue, isLocked;
> >+        PrefHasUserValue(pref, &hasUserValue);
> >+        PrefIsLocked(pref, &isLocked);
> 
> Why this change? 'pref' is already a validated pref name, so this seems like
> it's doing unnecessary work.

yeah. this is remains from first attempt to remote GetComplexValue.
reverting for now.

> > static nsresult NotifyObserver(const char *newpref, void *data)
> > {
> >+#ifdef MOZ_IPC
> >+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> >+    // The chrome process is notifying content prefs observers through IPDL
> >+    return NS_OK;
> 
> We shouldn't ever get here, since we never register NotifyObserver in the
> content process. You should use an NS_NOTREACHED here.

Done.

> >diff --git a/modules/libpref/src/nsPrefBranch.h b/modules/libpref/src/nsPrefBranch.h
> 
> >+#ifdef MOZ_IPC
> >+#include "nsXULAppAPI.h"
> >+#endif
>
> Can you do this in nsPrefBranch.cpp instead?

Done.
 
> >diff --git a/modules/libpref/src/prefapi.h b/modules/libpref/src/prefapi.h
> 
> >-NSPR_BEGIN_EXTERN_C
> >+//NSPR_BEGIN_EXTERN_C
> >+extern "C" {
> 
> Why this change?
> 
> Those are my comments for now -- I can take another pass when you've got a new
> patch.

That was just a quick dirty hack I forgot there, because I was getting this error : 
http://pastebin.mozilla.org/725413

At fist, I thought something was busted in prtypes.h, because the C++ extern block delimiters were
not kicking in properly. The "__cplusplus" block should apply : 

#ifdef __cplusplus
#define NSPR_BEGIN_EXTERN_C     extern "C" {
#define NSPR_END_EXTERN_C       }
#else
#define NSPR_BEGIN_EXTERN_C
#define NSPR_END_EXTERN_C
#endif

... but it doesn't.
Leaving this change untouched for now, but I'd definitely like your feeling/input on this one.

Comment 21

[Frederic Plourde (:fred23)]

by the way, (In reply to comment #18)
> For the record, running Fennec with this patch applied cause the child process
> to crash during component registration.  Apparently some components try to
> access preferences, grab the (nonexistent) ContentProcessChild singleton and
> die on null-deref.

by the way, patch v.5 corrects this issue too.

Comment 22

[Frederic Plourde (:fred23)]

Attachment: patch v.6

oops... small nit in ContentProcessParent.h.
Please, review this one instead.
Thanks !

Comment 23

[[e-mail]]

(In reply to comment #20)
> Leaving this change untouched for now, but I'd definitely like your
> feeling/input on this one.

Hmm. I don't see why it's happening, but it must be related to your #include changes in nsPrefBranch.h/cpp. What happens if you reorder the includes to be after prefapi.h?

Comment 24

[Josh Matthews [:jdm]]

>172	    // keep references to remote observers
>173	    if (aHoldWeak) {
>174	        nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>175	                                                 >do_QueryInterface(aObserver);
>176	        if (!weakRefFactory) {
>177	            // the caller didn't give us an object that supports weak reference
>178	            return NS_ERROR_INVALID_ARG;
>179	        }
>180	    }

This block serves no purpose.

Comment 25

[Josh Matthews [:jdm]]

Actually, the way you're using the mObserver and mWeakRef variables is pretty strange.  Why not make mObserver an nsCOMPtr and mWeakRef an nsWeakPtr, removing all need for manual AddRef/Release calls?  Furthermore, the fact that you're AddRefing a weak reference sort of negates the purpose of a weak reference.

>76	                nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>77	                                           do_QueryInterface(aObserver);
>78	                nsCOMPtr<nsIWeakReference> tmp = 
>79	                                           do_GetWeakReference(weakRefFactory);
>80	                NS_ADDREF(this->mWeakRef = tmp);

Would be better written as
    nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
        do_QueryInterface(aObserver);
    mWeakRef = do_GetWeakReference(weakRefFactory);

Also, you left a |delete entry| in RecvNotifyRemotePrefObserver.

>258	            if (gSingleton) {
>259	                prefs->RemoveObserver("", gSingleton);
>260	            }
>261	        }

Why not just pass |this| instead of gSingleton?

>	#include "mozilla/dom/ContentProcessChild.h"

Needs a MOZ_IPC sandwich.

There are more instances of the |nsCString(str, strlen(str))| which can be changed:
>nsCString domain(aDomain, strlen(aDomain));

Finally, there are some indentation issues which need some love.  Specifically,
>311	        // if there is no user (or locked) value
>312	          if (!PREF_HasUserPref(pref) && !PREF_PrefIsLocked(pref)) {

>112	            if (prefService) {
>113	                prefService->GetBranch(this->mPrefRoot.get(), 
>114	                                                   >getter_AddRefs(prefBranch));
>115	                observer->Observe(prefBranch, "nsPref:changed",
>116	                                   NS_ConvertUTF8toUTF16(this->mDomain).get());

And the style guide says */& snuggle the variable type.  There are |type *name| and |type * name| declarations all over the place.

Comment 26

[Frederic Plourde (:fred23)]

(In reply to comment #24)
> >172	    // keep references to remote observers
> >173	    if (aHoldWeak) {
> >174	        nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
> >175	                                                 >do_QueryInterface(aObserver);
> >176	        if (!weakRefFactory) {
> >177	            // the caller didn't give us an object that supports weak reference
> >178	            return NS_ERROR_INVALID_ARG;
> >179	        }
> >180	    }
> 
> This block serves no purpose.

Look, I had inspired myself from already in-place nsPrefBranch::AddObserver code.
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#607

> >76	                nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
> >77	                                           do_QueryInterface(aObserver);
> >78	                nsCOMPtr<nsIWeakReference> tmp = 
> >79	                                           do_GetWeakReference(weakRefFactory);
> >80	                NS_ADDREF(this->mWeakRef = tmp);
> 
> Would be better written as
>     nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>         do_QueryInterface(aObserver);
>     mWeakRef = do_GetWeakReference(weakRefFactory);

Here too, I had followed the way nsPrefBranch::AddObserver was doing it.
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#611

But I agree, this looks strange and I'm willing to change it, of course.
But tell me, should I change this already in-place code too?

Comment 27

[[e-mail]]

Go ahead, improving existing code is always a good thing. :)

I'm about halfway through this review.

Comment 28

[[e-mail]]

Comment on attachment 446053 [details] [diff] [review]
patch v.6

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

>+nsresult
>+ContentProcessChild::RemoveRemotePrefObserver(nsCString &aDomain, 

>+    int i=0;
>+    nsPrefObserverStorage *entry;
>+    bool entryFound = false;
>+    while (i < mPrefObserverArray.Length() && !entryFound) {

'i' should be of type PRUint32, since that's what nsTArray uses: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#55

It would also be more natural to write this as a 'for' loop:

  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);
>+            entryFound = true;

You can write this using a 'break' statement, and not need the 'entryFound' variable.

>+        }
>+        i++;
>+    }
>+    NS_ASSERTION(entryFound, "No preference Observer matched ");
>+    return NS_OK;

Returning an error here would be better.

>+nsresult
>+ContentProcessChild::ClearPrefObservers()
>+{
>+    mPrefObserverArray.Clear();
>+    return true;

No need for an nsresult return val here; you could also write this function inline in the .h file.

>+bool
>+ContentProcessChild::RecvNotifyRemotePrefObserver(const nsCString& aDomain)
>+{
>+    int i=0;
>+    nsPrefObserverStorage *entry;
>+    while (i < mPrefObserverArray.Length()) {
>+        entry = mPrefObserverArray[i];
>+        nsCString prefName(entry->GetPrefRoot() + entry->GetDomain());

nsCAutoString here. And same comment about using a for loop.

>+        // aDomain here is returned from our global pref observer from parent
>+        // so it's really a full pref name (i.e. root + domain)
>+        if (StringBeginsWith(aDomain, prefName)) {
>+            if (!entry->NotifyObserver()) {
>+                // remove the observer from the list
>+                mPrefObserverArray.RemoveElementAt(i);
>+                delete entry;

Hmm, this will crash. RemoveElementAt() will cause the entry to be deleted, since it's held onto by an nsAutoPtr. No need for the delete.

>diff --git a/dom/ipc/ContentProcessChild.h b/dom/ipc/ContentProcessChild.h

>+        bool NotifyObserver() {

>+            if (prefService) {
>+                prefService->GetBranch(this->mPrefRoot.get(), 
>+                                                   getter_AddRefs(prefBranch));
>+                observer->Observe(prefBranch, "nsPref:changed",
>+                                   NS_ConvertUTF8toUTF16(this->mDomain).get());

NS_ConvertASCIItoUTF16 here.

>+        nsCString GetDomain() {return mDomain;}
>+        nsCString GetPrefRoot() {return mPrefRoot;}

These should return const nsCString& references.

>+    nsresult AddRemotePrefObserver(nsCString &aDomain, nsCString &aPrefRoot, 
>+                                   nsIObserver *aObserver, PRBool aHoldWeak);
>+    nsresult RemoveRemotePrefObserver(nsCString &aDomain, nsCString &aPrefRoot, 
>+                                      nsIObserver *aObserver);

These should be const nsCString& references.

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

>+bool
>+ContentProcessParent::RecvGetPrefType(const nsCString& prefName,
>+                                      PRInt32* retValue, nsresult* rv)
>+{
>+    *retValue = 0;
>+    nsCOMPtr<nsIPrefBranch>prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, rv));

Need a space in '> prefs', and in other uses below.

>+    if (prefs)
>+        *rv = prefs->GetPrefType(prefName.get(), retValue);

And... I think you should cache the prefservice here, rather than looking it up each time. You could do this by adding an EnsurePrefService() function to ContentProcessParent, which calls up do_GetService if mPrefService is null.

Actually -- anyone in the content process that wants a prefbranch has to instantiate the prefservice first. We could remote nsPrefService::Init(), which instantiates and caches it in the parent; if that fails, we return failure to the child, which means nsIPrefBranch methods can't be called. Then you don't need an EnsurePrefService() method or the 'if (prefs)' checks here.

Speaking of which, you're not touching nsPrefService.cpp here -- specifically, nsPrefService::Init() fully loads up the pref file and such. All this work is still being done in the child, but we're just remoting the actual pref lookups. Fixing that can be a separate patch. Other prefservice methods, like nsPrefService::ReadUserPrefs(), should fail in the content process.

This will be a bunch more remoting to stuff onto ContentProcessChild/Parent; you probably want to add a PPrefService IPDL interface on PContentProcess. You don't need to do it in this patch though :)

>@@ -178,6 +251,15 @@ ContentProcessParent::Observe(nsISupport

>+        if (prefs) { 
>+            if (gSingleton) {
>+                prefs->RemoveObserver("", gSingleton);

What jdm said about 'this'.

>+    // listening for remotePrefs...
>+    if (!strcmp(aTopic, "nsPref:changed")) {
>+        NS_ConvertUTF16toUTF8 strData(aData);

Can use NS_LossyConvertUTF16toASCII here.

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

> bool
> ContentProcessProcess::Init()
> {
>-    mXREEmbed.Start();
>     mContentProcess.Init(IOThreadChild::message_loop(),
>                          ParentHandle(),
>                          IOThreadChild::channel());
>+    mXREEmbed.Start();

Why this change?

>diff --git a/modules/libpref/src/nsPrefBranch.cpp b/modules/libpref/src/nsPrefBranch.cpp

>@@ -142,6 +149,20 @@ NS_IMETHODIMP nsPrefBranch::GetPrefType(

>+    PRInt32 retval;
>+    cpc->SendGetPrefType(nsDependentCString(getPrefName(aPrefName)), &retval, &rv);
>+    if (NS_SUCCEEDED(rv))
>+        *_retval = retval;

4-space indenting here -- should be 2-space. This happens in other places in this file too.

With jdm's comments, this looks good.

The security pref bits should get removed in bug 564048.

Comment 29

[Josh Matthews [:jdm]]

>> bool
>> ContentProcessProcess::Init()
>> {
>>-    mXREEmbed.Start();
>>     mContentProcess.Init(IOThreadChild::message_loop(),
>>                          ParentHandle(),
>>                          IOThreadChild::channel());
>>+    mXREEmbed.Start();
>
>Why this change?

I suspect this works around the content process singleton not existing during component registration (comment 18).

Comment 30

[Frederic Plourde (:fred23)]

(In reply to comment #29)
> >> bool
> >> ContentProcessProcess::Init()
> >> {
> >>-    mXREEmbed.Start();
> >>     mContentProcess.Init(IOThreadChild::message_loop(),
> >>                          ParentHandle(),
> >>                          IOThreadChild::channel());
> >>+    mXREEmbed.Start();
> >
> >Why this change?
> 
> I suspect this works around the content process singleton not existing during
> component registration (comment 18).

That's right. The order here is important. We gotta have a fully built child before relaying prefs requests to it.

Comment 31

[Frederic Plourde (:fred23)]

Attachment: patch v.7

All the remaining comments from Dan and Josh have been addressed in this (hopefully) last version of the patch. Following are my comments : 

(In reply to comment #23)
> (In reply to comment #20)
> > Leaving this change untouched for now, but I'd definitely like your
> > feeling/input on this one.
> 
> Hmm. I don't see why it's happening, but it must be related to your #include
> changes in nsPrefBranch.h/cpp. What happens if you reorder the includes to be
> after prefapi.h?

Ha! I found it.
prefapi.h is complaining because in the MOZ_IPC case, we're including Chromium version
of prtypes, which does *not* support NSPR_10, thus making it fail to build.
See bug #566869 for more detail. For now, I'm adding this local hack that will make
prefapi.h happy : 

#ifdef __cplusplus
#define NSPR_BEGIN_EXTERN_C     extern "C" {
#define NSPR_END_EXTERN_C       }
#else
#define NSPR_BEGIN_EXTERN_C
#define NSPR_END_EXTERN_C
#endif

This issue's been taken care of in bug 566869.

(In reply to comment #24)
> >172	    // keep references to remote observers
> >173	    if (aHoldWeak) {
> >174	        nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
> >175	                                                 >do_QueryInterface(aObserver);
> >176	        if (!weakRefFactory) {
> >177	            // the caller didn't give us an object that supports weak reference
> >178	            return NS_ERROR_INVALID_ARG;
> >179	        }
> >180	    }
> 
> This block serves no purpose.

Right. It'll be taken care or in nsPrefObserverStorage ctor.
Done.

(In reply to comment #25)
> Actually, the way you're using the mObserver and mWeakRef variables is pretty
> strange.  Why not make mObserver an nsCOMPtr and mWeakRef an nsWeakPtr,
> removing all need for manual AddRef/Release calls?  Furthermore, the fact that
> you're AddRefing a weak reference sort of negates the purpose of a weak
> reference.
> 
> >76	                nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
> >77	                                           do_QueryInterface(aObserver);
> >78	                nsCOMPtr<nsIWeakReference> tmp = 
> >79	                                           do_GetWeakReference(weakRefFactory);
> >80	                NS_ADDREF(this->mWeakRef = tmp);
> 
> Would be better written as
>     nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
>         do_QueryInterface(aObserver);
>     mWeakRef = do_GetWeakReference(weakRefFactory);

Done.

> Also, you left a |delete entry| in RecvNotifyRemotePrefObserver.

Done.

> >258	            if (gSingleton) {
> >259	                prefs->RemoveObserver("", gSingleton);
> >260	            }
> >261	        }
> 
> Why not just pass |this| instead of gSingleton?

Done.

> >	#include "mozilla/dom/ContentProcessChild.h"
> 
> Needs a MOZ_IPC sandwich.

Done.

> There are more instances of the |nsCString(str, strlen(str))| which can be
> changed:
> >nsCString domain(aDomain, strlen(aDomain));

Done.

> Finally, there are some indentation issues which need some love.  Specifically,
> >311	        // if there is no user (or locked) value
> >312	          if (!PREF_HasUserPref(pref) && !PREF_PrefIsLocked(pref)) {
> 
> >112	            if (prefService) {
> >113	                prefService->GetBranch(this->mPrefRoot.get(), 
> >114	                                                   >getter_AddRefs(prefBranch));
> >115	                observer->Observe(prefBranch, "nsPref:changed",
> >116	                                   NS_ConvertUTF8toUTF16(this->mDomain).get());
> 
> And the style guide says */& snuggle the variable type.  There are |type *name|
> and |type * name| declarations all over the place.

Done. Some love provided.


(In reply to comment #27)
> Go ahead, improving existing code is always a good thing. :)
> 
> I'm about halfway through this review.

XXXXXXXXXXXXXXXX 
XXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXX wait !!


(In reply to comment #28)
> (From update of attachment 446053 [details] [diff] [review])
> >diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp
> 
> >+nsresult
> >+ContentProcessChild::RemoveRemotePrefObserver(nsCString &aDomain, 
> 
> >+    int i=0;
> >+    nsPrefObserverStorage *entry;
> >+    bool entryFound = false;
> >+    while (i < mPrefObserverArray.Length() && !entryFound) {
> 
> 'i' should be of type PRUint32, since that's what nsTArray uses:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#55
> 
> It would also be more natural to write this as a 'for' loop:
> 
>   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);
> >+            entryFound = true;
> 
> You can write this using a 'break' statement, and not need the 'entryFound'
> variable.
> >+        }
> >+        i++;
> >+    }
> >+    NS_ASSERTION(entryFound, "No preference Observer matched ");
> >+    return NS_OK;
> 
> Returning an error here would be better.

I used a 'return' statement instead.
And then, I systematically assert at the end of the function, and return 
and NS_ERROR_UNEXPECTED

> >+nsresult
> >+ContentProcessChild::ClearPrefObservers()
> >+{
> >+    mPrefObserverArray.Clear();
> >+    return true;
> 
> No need for an nsresult return val here; you could also write this function
> inline in the .h file.

Done. Inlined.

> >+bool
> >+ContentProcessChild::RecvNotifyRemotePrefObserver(const nsCString& aDomain)
> >+{
> >+    int i=0;
> >+    nsPrefObserverStorage *entry;
> >+    while (i < mPrefObserverArray.Length()) {
> >+        entry = mPrefObserverArray[i];
> >+        nsCString prefName(entry->GetPrefRoot() + entry->GetDomain());
> 
> nsCAutoString here. And same comment about using a for loop.

Done.
 
> >diff --git a/dom/ipc/ContentProcessChild.h b/dom/ipc/ContentProcessChild.h
> 
> >+        bool NotifyObserver() {
> 
> >+            if (prefService) {
> >+                prefService->GetBranch(this->mPrefRoot.get(), 
> >+                                                   getter_AddRefs(prefBranch));
> >+                observer->Observe(prefBranch, "nsPref:changed",
> >+                                   NS_ConvertUTF8toUTF16(this->mDomain).get());
> 
> NS_ConvertASCIItoUTF16 here.

Done.

> >+        nsCString GetDomain() {return mDomain;}
> >+        nsCString GetPrefRoot() {return mPrefRoot;}
> 
> These should return const nsCString& references.

Done.

> >+    nsresult AddRemotePrefObserver(nsCString &aDomain, nsCString &aPrefRoot, 
> >+                                   nsIObserver *aObserver, PRBool aHoldWeak);
> >+    nsresult RemoveRemotePrefObserver(nsCString &aDomain, nsCString &aPrefRoot, 
> >+                                      nsIObserver *aObserver);
> 
> These should be const nsCString& references.

Done.

> >diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
> 
> >+bool
> >+ContentProcessParent::RecvGetPrefType(const nsCString& prefName,
> >+                                      PRInt32* retValue, nsresult* rv)
> >+{
> >+    *retValue = 0;
> >+    nsCOMPtr<nsIPrefBranch>prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, rv));
> 
> Need a space in '> prefs', and in other uses below.

done.

> >+    if (prefs)
> >+        *rv = prefs->GetPrefType(prefName.get(), retValue);
> 
> And... I think you should cache the prefservice here, rather than looking it up
> each time. You could do this by adding an EnsurePrefService() function to
> ContentProcessParent, which calls up do_GetService if mPrefService is null.
>
> Actually -- anyone in the content process that wants a prefbranch has to
> instantiate the prefservice first. We could remote nsPrefService::Init(), which
> instantiates and caches it in the parent; if that fails, we return failure to
> the child, which means nsIPrefBranch methods can't be called. Then you don't
> need an EnsurePrefService() method or the 'if (prefs)' checks here.
> 
> Speaking of which, you're not touching nsPrefService.cpp here -- specifically,
> nsPrefService::Init() fully loads up the pref file and such. All this work is
> still being done in the child, but we're just remoting the actual pref lookups.
> Fixing that can be a separate patch. Other prefservice methods, like
> nsPrefService::ReadUserPrefs(), should fail in the content process.
> 
> This will be a bunch more remoting to stuff onto ContentProcessChild/Parent;
> you probably want to add a PPrefService IPDL interface on PContentProcess. You
> don't need to do it in this patch though :)

ok. For now, I've only implemented your EnsurePrefService idea.
I'll file another bug and start working on the rest of your idea.

Notice though that I chose to ASSERT here if we can't get the prefService
because I wanna be warned if we ever get XPCOM to start BEFORE full ContentProcess actors init.
 
> >+    // listening for remotePrefs...
> >+    if (!strcmp(aTopic, "nsPref:changed")) {
> >+        NS_ConvertUTF16toUTF8 strData(aData);
> 
> Can use NS_LossyConvertUTF16toASCII here.

Done.
 
> >diff --git a/modules/libpref/src/nsPrefBranch.cpp b/modules/libpref/src/nsPrefBranch.cpp
> 
> >@@ -142,6 +149,20 @@ NS_IMETHODIMP nsPrefBranch::GetPrefType(
> 
> >+    PRInt32 retval;
> >+    cpc->SendGetPrefType(nsDependentCString(getPrefName(aPrefName)), &retval, &rv);
> >+    if (NS_SUCCEEDED(rv))
> >+        *_retval = retval;
> 
> 4-space indenting here -- should be 2-space. This happens in other places in
> this file too.

Done.

Comment 32

[Frederic Plourde (:fred23)]

please, forget the "XXXXXXXXXXXXXXXX wait !!" comment, it's not relevant anymore.

Comment 33

[Frederic Plourde (:fred23)]

Josh: 

I'm not landing this huge patch yet because I haven't been able to test remote prefs observers. See, I used to have a nice test (ContentPref.xul) that let me add pref observers from 'content' and actually debug the observer notifs, but it doesn't work anymore because I'm constantly hitting this in nsChromeRegistryContent.cpp: 

NS_IMETHODIMP
nsChromeRegistryContent::GetStyleOverlays(nsIURI *aChromeURL,
                                          nsISimpleEnumerator **aResult)
{
  CONTENT_NOT_IMPLEMENTED();
}

My testapp adds some <toolbar> elements in content, but I only see a gray area that's not beeing reflowed/redrawn when resizing main window. Is this the expected behaviour at the moment in the e10s repo?  Or should that be already worked around ?  Well.. maybe you can help me shed some light oin this. thanks.

Comment 34

[Josh Matthews [:jdm]]

I'm curious as to when the test document worked properly; I'm guessing it was broken by the chrome registry changes landing a few months ago.  Also, the |    nsTArray<nsAutoPtr<nsPrefObserverStorage>> mPrefObserverArray;| construct breaks compilation under gcc 4.4.3, so you should fix that to be |<nsPrefObserverStorage> >| instead.

Comment 35

[Josh Matthews [:jdm]]

I just had another thought:

+    for (PRUint32 i = 0; i < mPrefObserverArray.Length(); ++i) {
+        entry = mPrefObserverArray[i];
+        nsCAutoString prefName(entry->GetPrefRoot() + entry->GetDomain());
+        // aDomain here is returned from our global pref observer from parent
+        // so it's really a full pref name (i.e. root + domain)
+        if (StringBeginsWith(aDomain, prefName)) {
+            if (!entry->NotifyObserver()) {
+                // remove the observer from the list
+                mPrefObserverArray.RemoveElementAt(i);
+                --i;
+            }
+        }
+    }

Suppose the first observer is notified, then removed.  The index goes to -1 (ie. a very large value), and the loop terminates without iterating through the rest of the array.  You should probably iterate the array in reverse order so as to avoid this problem.

While you're making that change, might as well remove the extra check for gSingleton here:

+            if (gSingleton) {
+                prefs->RemoveObserver("", this);
+            }

Comment 36

[Josh Matthews [:jdm]]

Fred: care to share the contents of ContentPref.xul?  I don't see it anywhere.

Comment 37

[Josh Matthews [:jdm]]

Nevermind, I looked in the obsolete attachments.  bsmedberg says the solution here is to make ContentPref.xul HTML instead.

Comment 38

[Frederic Plourde (:fred23)]

(In reply to comment #34)
> I'm curious as to when the test document worked properly; I'm guessing it was
> broken by the chrome registry changes landing a few months ago.  

yeah, I haven't been using this test for a long time..

> Also, the |   
> nsTArray<nsAutoPtr<nsPrefObserverStorage>> mPrefObserverArray;| construct
> breaks compilation under gcc 4.4.3, so you should fix that to be
> |<nsPrefObserverStorage> >| instead.

'cl' didn't complain. Ok, I'll correct for gcc too. thanks.

Comment 39

[Frederic Plourde (:fred23)]

> Suppose the first observer is notified, then removed.  The index goes to -1
> (ie. a very large value), and the loop terminates without iterating through the rest of the array.  You should probably iterate the array in reverse order so as to avoid this problem.

Ok, I'm not sure, but isn't there a first-in/first-notified priority here ?
If so, couldn't we just use a PRint32 'i' instead and accept this temporary -1 value ?

Comment 40

[[e-mail]]

Probably best you make it back into a 'while' loop, and only increment the counter if the element wasn't removed.

Comment 41

[[e-mail]]

http://hg.mozilla.org/projects/electrolysis/rev/90fa3f90c0f7

Pushed to e10s, with the suggested fixes. I'll post the patch I actually pushed, for reference...

Comment 42

[[e-mail]]

Attachment: pushed patch

Comment 43

[[e-mail]]

Also, it sounded like Fred wanted to follow up with some ContentPref.html tests here, so that should probably be done in another bug.

Comment 45

[[e-mail]]

Also, for followups:

1. Remote nsPrefService
2. Remote GetComplexValue
3. Fix the weird weak reference code here, and in nsPrefBranch (comment 26)

I'll file bugs on these. Fred, let me know which of those you plan to work on; I can take up whatever's left. :)