Moved JsonPrefStore to use SequencedWorkerPool

chrome/browser/profiles/profile_impl.cc

Index: chrome/browser/profiles/profile_impl.cc
diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
index 735cba8acb25dc9e8cff36bb2870cc7a987155a7..096e30bc656def0bce3e898885bcf035083ae95e 100644
--- a/chrome/browser/profiles/profile_impl.cc
+++ b/chrome/browser/profiles/profile_impl.cc
@@ -16,6 +16,7 @@
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/utf_string_conversions.h"
 #include "base/version.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
@@ -75,6 +76,7 @@
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
+#include "chrome/common/json_pref_store.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
@@ -208,12 +210,19 @@ std::string ExitTypeToSessionTypePrefValue(Profile::ExitType type) {
 Profile* Profile::CreateProfile(const FilePath& path,
                                 Delegate* delegate,
                                 CreateMode create_mode) {
+  // Get sequenced task runner for making sure that file operations of
+  // this profile (defined by |path|) are executed in expected order
+  // (what was previously assured by the FILE thread).
+  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
+      JsonPrefStore::GetTaskRunnerForFile(path,
+                                          BrowserThread::GetBlockingPool());
   if (create_mode == CREATE_MODE_ASYNCHRONOUS) {
     DCHECK(delegate);
-    // This is safe while all file operations are done on the FILE thread.
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&CreateDirectoryNoResult, path));
+    // We need to make sure that profile directory is created on the same
+    // blocking pool where we will run all its important file serialization
+    // tasks (i.e. JsonPrefStore).
+    sequenced_task_runner->PostTask(FROM_HERE,
+                                    base::Bind(&CreateDirectoryNoResult, path));

[Mattias Nissler (ping if slow), 2012/10/22 17:28:21]

This is dangerous if other code that is still on the FILE thread also access the
directory if that code assumes the directory exists. Have you checked?

edit: I found you've adjusted a couple of other file operations. If so, what are
we gaining? If the FILE thread was clogged before, so is the task runner
afterwards. Or is there more stuff going on on the FILE thread? It seems you
want the directory created and then have lots of parallelism instead of all file
consumers on the same task runner, no?

[zel, 2012/10/24 02:20:11]

Yes, I've tested this and adjusted such FILE dependencies through number of
initially failing (warning) browser tests. This was a yak shaving that caused my
to touch all these tests to begin with.
I need parallelism during shutdown which triggers JsonPrefStore serialization. I
need to ensure that FILE thread is not clogged by other less important
operations at that time (i.e. extension install). Otherwise, JsonPrefStore
sometimes had chance to get serialized or even gets corrupted (due to SIGABRT
timeout sent from session manager). This condition caused top crash report on
ChromeOS. Please read http://crbug.com/153367 for more details.

[Mattias Nissler (ping if slow), 2012/10/24 13:36:16]

Well, those were probably the easier ones :) I'm concerned with flakes showing
up due to timing issues. In fact, somebody has tried this same CL earlier and
got reverted: https://chromiumcodereview.appspot.com/10344007/ (you might want
to follow up about the reason).

Fixing all these flakes might result in more and more stuff ending up on your
new task runner instance, which is bad (see below), but see my other idea even
further below ;)
That's understood. What I'm trying to say is this: If we need to move additional
work to the task runner for correctness (because the directory needs to be
created before stuff accesses it), that'll mean all that stuff will be on the
very same task runner and thus the same thread on shutdown as well. If that's
what we end up doing, then we've just moved everything from BrowserThread::FILE
to a worker pool thread, but the level of parallelism is still the same.

Edit: I just realized that a better solution for the problem of having the
directory created before anything else happens on both the FILE thread and the
pref store task runner is to block both of them on creation. So how about
keeping directory creation on the task runner and posting a separate task to the
FILE thread that blocks on a signal from the other thread once the directory has
been created? That would make sure everything that gets posted to the FILE
thread afterwards can assume the directory is created.

[zel, 2012/10/24 16:49:48]

Yeah, that's a great idea. Done - PTAL. 

   } else if (create_mode == CREATE_MODE_SYNCHRONOUS) {
     if (!file_util::PathExists(path)) {
       // TODO(tc): http://b/1094718 Bad things happen if we can't write to the
@@ -226,7 +235,7 @@ Profile* Profile::CreateProfile(const FilePath& path,
     NOTREACHED();
   }
 
-  return new ProfileImpl(path, delegate, create_mode);
+  return new ProfileImpl(path, delegate, create_mode, sequenced_task_runner);
 }
 
 // static
@@ -279,9 +288,11 @@ void ProfileImpl::RegisterUserPrefs(PrefService* prefs) {
                              PrefService::SYNCABLE_PREF);
 }
 
-ProfileImpl::ProfileImpl(const FilePath& path,
-                         Delegate* delegate,
-                         CreateMode create_mode)
+ProfileImpl::ProfileImpl(
+    const FilePath& path,
+    Delegate* delegate,
+    CreateMode create_mode,
+    base::SequencedTaskRunner* sequenced_task_runner)
     : path_(path),
       ALLOW_THIS_IN_INITIALIZER_LIST(visited_link_event_listener_(
           new VisitedLinkEventListener(this))),
@@ -331,7 +342,8 @@ ProfileImpl::ProfileImpl(const FilePath& path,
         policy_service_.get(),
         new ExtensionPrefStore(
             ExtensionPrefValueMapFactory::GetForProfile(this), false),
-        true));
+        true,
+        sequenced_task_runner));
     // Wait for the notification that prefs has been loaded (successfully or
     // not).
     registrar_.Add(this, chrome::NOTIFICATION_PREF_INITIALIZATION_COMPLETED,
@@ -343,7 +355,8 @@ ProfileImpl::ProfileImpl(const FilePath& path,
         policy_service_.get(),
         new ExtensionPrefStore(
             ExtensionPrefValueMapFactory::GetForProfile(this), false),
-        false));
+        false,
+        sequenced_task_runner));
     OnPrefsLoaded(true);
   } else {
     NOTREACHED();