Moved JsonPrefStore to use SequencedWorkerPool

chrome/browser/profiles/profile_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #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"
 #include "chrome/browser/background/background_contents_service_factory.h"
 #include "chrome/browser/background/background_mode_manager.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_plugin_service_filter.h"
 #include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
@@ skipping 39 matching lines @@
 #include "chrome/browser/user_style_sheet_watcher.h"
 #include "chrome/browser/visitedlink/visitedlink_event_listener.h"
 #include "chrome/browser/visitedlink/visitedlink_master.h"
 #include "chrome/browser/web_resource/promo_resource_service.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_notification_types.h"
 #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"
 #include "content/public/browser/dom_storage_context.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/content_constants.h"
@@ skipping 113 matching lines @@
   NOTREACHED();
   return std::string();
 }
 
 }  // namespace
 
 // static
 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
       // profile directory.  We should eventually be able to run in this
       // situation.
       if (!file_util::CreateDirectory(path))
         return NULL;
     }
   } else {
     NOTREACHED();
   }
 
-  return new ProfileImpl(path, delegate, create_mode);
+  return new ProfileImpl(path, delegate, create_mode, sequenced_task_runner);
 }
 
 // static
 int ProfileImpl::create_readme_delay_ms = 60000;
 
 // static
 const char* const ProfileImpl::kPrefExitTypeNormal = "Normal";
 
 // static
 void ProfileImpl::RegisterUserPrefs(PrefService* prefs) {
@@ skipping 32 matching lines @@
   prefs->RegisterIntegerPref(prefs::kMediaCacheSize,
                              0,
                              PrefService::UNSYNCABLE_PREF);
 
   // Deprecated. Kept around for migration.
   prefs->RegisterBooleanPref(prefs::kClearSiteDataOnExit,
                              false,
                              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))),
       ALLOW_THIS_IN_INITIALIZER_LIST(io_data_(this)),
       host_content_settings_map_(NULL),
       last_session_exit_type_(EXIT_NORMAL),
       start_time_(Time::Now()),
       delegate_(delegate),
       predictor_(NULL) {
   DCHECK(!path.empty()) << "Using an empty path will attempt to write " <<
@@ skipping 29 matching lines @@
 #else
   policy_service_.reset(new policy::PolicyServiceStub());
 #endif
 
   if (create_mode == CREATE_MODE_ASYNCHRONOUS) {
     prefs_.reset(PrefService::CreatePrefService(
         GetPrefFilePath(),
         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,
                    content::Source<PrefService>(prefs_.get()));
   } else if (create_mode == CREATE_MODE_SYNCHRONOUS) {
     // Load prefs synchronously.
     prefs_.reset(PrefService::CreatePrefService(
         GetPrefFilePath(),
         policy_service_.get(),
         new ExtensionPrefStore(
             ExtensionPrefValueMapFactory::GetForProfile(this), false),
-        false));
+        false,
+        sequenced_task_runner));
     OnPrefsLoaded(true);
   } else {
     NOTREACHED();
   }
 }
 
 void ProfileImpl::DoFinalInit(bool is_new_profile) {
   PrefService* prefs = GetPrefs();
   pref_change_registrar_.Init(prefs);
   pref_change_registrar_.Add(prefs::kGoogleServicesUsername, this);
@@ skipping 808 matching lines @@
   if (!path.empty())
     *cache_path = path;
   *max_size = is_media_context ? prefs_->GetInteger(prefs::kMediaCacheSize) :
                                  prefs_->GetInteger(prefs::kDiskCacheSize);
 }
 
 base::Callback<ChromeURLDataManagerBackend*(void)>
     ProfileImpl::GetChromeURLDataManagerBackendGetter() const {
   return io_data_.GetChromeURLDataManagerBackendGetter();
 }