Get rid of PrefService::GetMutableDictionary/GetMutableList

chrome/browser/net/predictor_api.cc

Index: chrome/browser/net/predictor_api.cc
diff --git a/chrome/browser/net/predictor_api.cc b/chrome/browser/net/predictor_api.cc
index 232cb9fffe3ca0795abe9e688b39af67a1709a05..d9434f308337b404efd80c0f90bcb86295b9856d 100644
--- a/chrome/browser/net/predictor_api.cc
+++ b/chrome/browser/net/predictor_api.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/net/url_info.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -407,7 +408,7 @@ static void InitNetworkPredictor(TimeDelta max_dns_queue_delay,
       GetPredictedUrlListAtStartup(user_prefs, local_state);
 
   ListValue* referral_list =
-      static_cast<ListValue*>(user_prefs->GetMutableList(
+      static_cast<ListValue*>(user_prefs->GetList(
           prefs::kDnsPrefetchingHostReferralList)->DeepCopy());
 
   // Remove obsolete preferences from local state if necessary.
@@ -484,12 +485,15 @@ void SavePredictorStateForNextStartupAndTrim(PrefService* prefs) {
 
   base::WaitableEvent completion(true, false);
 
+  ListPrefUpdate update_startup_list(prefs, prefs::kDnsPrefetchingStartupList);
+  ListPrefUpdate update_referral_list(prefs,
+                                      prefs::kDnsPrefetchingHostReferralList);
   bool posted = BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       NewRunnableFunction(SaveDnsPrefetchStateForNextStartupAndTrimOnIOThread,
-          prefs->GetMutableList(prefs::kDnsPrefetchingStartupList),
-          prefs->GetMutableList(prefs::kDnsPrefetchingHostReferralList),
+          update_startup_list.Get(),

[jar (doing other things), 2011/04/04 19:18:33]

This makes very little sense to me.  I'm *guessing* that Get() pulls out the
underlying list (since you don't have any other changes... such as changing the
called function).  Hence your destructor for lies 488 and 489 will fire, despite
the fact that the changes have not been made to the list (they will be made deep
in the post tasks... which won't happen or a while... and will be on another
thread).

Bottom line: This does not appear to address the concern of the bug (other than
adding odd complexity).

What am I missing??

[jar (doing other things), 2011/04/05 06:18:39]

I suspect this may actually be the source of a larger bug.  The list is being
modified on a second thread (presumably the IO thread). This mutable list is
probably being scanned asynchronously on the UI thread to persist the values. 
The resulting race may be very dangerous.   I assume the fix is to call to the
IO thread to get the data, but than transcribe it onto the MutableList back on
the UI thread.  

Your change merely highlights the fact that the list value is bein manipulated
on another thread.  This larger bug should be fixed, and then you can wrap the
UI thread changes in your planned ListPrefUpdate notifier.

On 2011/04/04 19:18:33, jar wrote:

[battre, 2011/04/05 14:09:50]

I thought about this.

I think the code is safe because of the "completion.Wait()" below. We grab the
dictionaries on the UI thread, post the task that does the modifications and
halt the UI thread until the modifications are completed. As only the UI thread
can access the preferences and this thread is blocked, nothing serious can
happen.

The scope of update_startup_list and update_referral_list ends after the Wait()
returns. Therefore, the "preference changed" signal is sent after the changes
have been done.

I wonder however about the impact of the Wait() on the UI thread, which would be
nice to avoid. OTOH, the only caller of SavePredictorStateForNextStartupAndTrim
is the browser shutdown code.

How do you suggest to proceed?


On 2011/04/05 06:18:39, jar wrote:

[jar (doing other things), 2011/04/05 16:03:11]

<sigh>  Ok... I'm convinced your change is correct, for this context.

This approach of pausing a thread, and waiting for another thread to complete
(which existed before you touched the code) is fraught with peril... and is
indeed the thing that deadlocks are made of.  If another programmer on the IO
thread decided to do the same thing, waiting for a UI completion... then both
threads would block.  :-(.

We need to look closer and see who else uses this questionable pattern.  It may
be that such are required at shutdown... but there needs to be a LOT more
coordination about who can do such things IMO.  This could easily be a major
source of hung (dirty/deadlocked) shutdowns.

On 2011/04/05 14:09:50, battre wrote:

+          update_referral_list.Get(),
           &completion));
 
   DCHECK(posted);
@@ -505,11 +509,11 @@ static UrlList GetPredictedUrlListAtStartup(PrefService* user_prefs,
   // This may catch secondary hostnames, pulled in by the homepages.  It will
   // also catch more of the "primary" home pages, since that was (presumably)
   // rendered first (and will be rendered first this time too).
-  ListValue* startup_list =
-      user_prefs->GetMutableList(prefs::kDnsPrefetchingStartupList);
+  const ListValue* startup_list =
+      user_prefs->GetList(prefs::kDnsPrefetchingStartupList);
 
   if (startup_list) {
-    ListValue::iterator it = startup_list->begin();
+    ListValue::const_iterator it = startup_list->begin();
     int format_version = -1;
     if (it != startup_list->end() &&
         (*it)->GetAsInteger(&format_version) &&