Remove locks from StrictTransportSecurityState.

These locks can cause the IO thread to block on the FILE thread writing to
disk, which is bad news bears.

BUG=21518
TEST=No behavior change.

Review URL: http://codereview.chromium.org/904005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41538 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 56 insertions, 49 deletions

diff --git a/chrome/browser/browsing_data_remover.cc b/chrome/browser/browsing_data_remover.cc
index 0614969..614c781 100644
--- a/chrome/browser/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data_remover.cc
@@ -157,9 +157,12 @@ void BrowsingDataRemover::Remove(int remove_mask) {
               delete_begin_));
     }
 
-    net::TransportSecurityState* ts_state =
-        profile_->GetTransportSecurityState();
-    ts_state->DeleteSince(delete_begin_);
+    ChromeThread::PostTask(
+        ChromeThread::IO, FROM_HERE,
+        NewRunnableMethod(
+            profile_->GetTransportSecurityState(),
+            &net::TransportSecurityState::DeleteSince,
+            delete_begin_));
 
     waiting_for_clear_appcache_ = true;
     ChromeThread::PostTask(
diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 5550496..cc19e5b 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -842,12 +842,8 @@ ChromeURLRequestContextFactory::ChromeURLRequestContextFactory(Profile* profile)
   referrer_charset_ = default_charset;
 
   host_content_settings_map_ = profile->GetHostContentSettingsMap();
-
   host_zoom_map_ = profile->GetHostZoomMap();
-
   privacy_blacklist_ = profile->GetPrivacyBlacklist();
-
-  // TODO(eroman): this doesn't look safe; sharing between IO and UI threads!
   transport_security_state_ = profile->GetTransportSecurityState();
 
   if (profile->GetExtensionsService()) {
diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h
index 07e2f0e..8f3c260 100644
--- a/chrome/browser/net/chrome_url_request_context.h
+++ b/chrome/browser/net/chrome_url_request_context.h
@@ -383,7 +383,7 @@ class ChromeURLRequestContextFactory {
   scoped_refptr<HostContentSettingsMap> host_content_settings_map_;
   scoped_refptr<HostZoomMap> host_zoom_map_;
   scoped_refptr<Blacklist> privacy_blacklist_;
-  net::TransportSecurityState* transport_security_state_;
+  scoped_refptr<net::TransportSecurityState> transport_security_state_;
   scoped_refptr<net::SSLConfigService> ssl_config_service_;
 
   FilePath profile_dir_path_;
diff --git a/chrome/browser/transport_security_persister.cc b/chrome/browser/transport_security_persister.cc
index a00dfb1..5ffaf80 100644
--- a/chrome/browser/transport_security_persister.cc
+++ b/chrome/browser/transport_security_persister.cc
@@ -13,7 +13,7 @@
 #include "net/base/transport_security_state.h"
 
 TransportSecurityPersister::TransportSecurityPersister()
-    : state_is_dirty_(false) {
+  : ALLOW_THIS_IN_INITIALIZER_LIST(save_coalescer_(this)) {
 }
 
 TransportSecurityPersister::~TransportSecurityPersister() {
@@ -22,30 +22,37 @@ TransportSecurityPersister::~TransportSecurityPersister() {
 
 void TransportSecurityPersister::Initialize(
     net::TransportSecurityState* state, const FilePath& profile_path) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   transport_security_state_ = state;
   state_file_ =
       profile_path.Append(FILE_PATH_LITERAL("TransportSecurity"));
   state->SetDelegate(this);
 
   Task* task = NewRunnableMethod(this,
-      &TransportSecurityPersister::LoadState);
+      &TransportSecurityPersister::Load);
   ChromeThread::PostDelayedTask(ChromeThread::FILE, FROM_HERE, task, 1000);
 }
 
-void TransportSecurityPersister::LoadState() {
+void TransportSecurityPersister::Load() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  bool dirty = false;
-  {
-    AutoLock locked_(lock_);
 
-    std::string state;
-    if (!file_util::ReadFileToString(state_file_, &state))
-      return;
+  std::string state;
+  if (!file_util::ReadFileToString(state_file_, &state))
+    return;
+
+  ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
+      NewRunnableMethod(this,
+                        &TransportSecurityPersister::CompleteLoad,
+                        state));
+}
 
-    if (!transport_security_state_->Deserialise(state, &dirty)) {
-      LOG(ERROR) << "Failed to deserialize state: " << state;
-      return;
-    }
+void TransportSecurityPersister::CompleteLoad(const std::string& state) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+
+  bool dirty = false;
+  if (!transport_security_state_->Deserialise(state, &dirty)) {
+    LOG(ERROR) << "Failed to deserialize state: " << state;
+    return;
   }
   if (dirty)
     StateIsDirty(transport_security_state_);
@@ -53,30 +60,32 @@ void TransportSecurityPersister::LoadState() {
 
 void TransportSecurityPersister::StateIsDirty(
     net::TransportSecurityState* state) {
-  // Runs on arbitary thread, may not block nor reenter
-  // |transport_security_state_|.
-  AutoLock locked_(lock_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DCHECK(state == transport_security_state_);
 
-  if (state_is_dirty_)
-    return;  // we already have a serialisation scheduled
+  if (!save_coalescer_.empty())
+    return;
 
-  Task* task = NewRunnableMethod(this,
-      &TransportSecurityPersister::SerialiseState);
-  ChromeThread::PostDelayedTask(ChromeThread::FILE, FROM_HERE, task, 1000);
-  state_is_dirty_ = true;
+  Task* task = save_coalescer_.NewRunnableMethod(
+      &TransportSecurityPersister::Save);
+  MessageLoop::current()->PostDelayedTask(FROM_HERE, task, 1000);
 }
 
-void TransportSecurityPersister::SerialiseState() {
-  AutoLock locked_(lock_);
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-
-  DCHECK(state_is_dirty_);
-  state_is_dirty_ = false;
+void TransportSecurityPersister::Save() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   std::string state;
   if (!transport_security_state_->Serialise(&state))
     return;
 
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
+      NewRunnableMethod(this,
+                        &TransportSecurityPersister::CompleteSave,
+                        state));
+}
+
+void TransportSecurityPersister::CompleteSave(const std::string& state) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+
   file_util::WriteFile(state_file_, state.data(), state.size());
 }
diff --git a/chrome/browser/transport_security_persister.h b/chrome/browser/transport_security_persister.h
index 660faa7..5e006ea 100644
--- a/chrome/browser/transport_security_persister.h
+++ b/chrome/browser/transport_security_persister.h
@@ -30,12 +30,13 @@
 //   copies the current state of the TransportSecurityState, serialises
 //   and writes to disk.
 
-#ifndef CHROME_BROWSER_STRICT_TRANSPORT_SECURITY_PERSISTER_H_
-#define CHROME_BROWSER_STRICT_TRANSPORT_SECURITY_PERSISTER_H_
+#ifndef CHROME_BROWSER_TRANSPORT_SECURITY_PERSISTER_H_
+#define CHROME_BROWSER_TRANSPORT_SECURITY_PERSISTER_H_
 
 #include "base/file_path.h"
 #include "base/lock.h"
 #include "base/ref_counted.h"
+#include "base/task.h"
 #include "net/base/transport_security_state.h"
 
 class TransportSecurityPersister
@@ -54,22 +55,20 @@ class TransportSecurityPersister
 
   ~TransportSecurityPersister();
 
-  // a Task callback for when the state needs to be written out.
-  void SerialiseState();
+  void Load();
+  void CompleteLoad(const std::string& state);
 
-  // a Task callback for when the state needs to be loaded from disk at startup.
-  void LoadState();
+  void Save();
+  void CompleteSave(const std::string& state);
 
-  Lock lock_;  // protects all the members
-
-  // true when the state object has signaled that we're dirty and we haven't
-  // serialised the state yet.
-  bool state_is_dirty_;
+  // Used on the IO thread to coalesce writes to disk.
+  ScopedRunnableMethodFactory<TransportSecurityPersister> save_coalescer_;
 
   scoped_refptr<net::TransportSecurityState>
-      transport_security_state_;
+      transport_security_state_;  // IO thread only.
+
   // The path to the file in which we store the serialised state.
   FilePath state_file_;
 };
 
-#endif  // CHROME_BROWSER_STRICT_TRANSPORT_SECURITY_PERSISTER_H_
+#endif  // CHROME_BROWSER_TRANSPORT_SECURITY_PERSISTER_H_