From 4cc5a8747adf8327ba0df645237c1d5f52690caf Mon Sep 17 00:00:00 2001
From: "abarth@chromium.org"
 <abarth@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Sun, 14 Mar 2010 00:16:15 +0000
Subject: 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
---
 chrome/browser/browsing_data_remover.cc          |  9 ++--
 chrome/browser/net/chrome_url_request_context.cc |  4 --
 chrome/browser/net/chrome_url_request_context.h  |  2 +-
 chrome/browser/transport_security_persister.cc   | 65 ++++++++++++++----------
 chrome/browser/transport_security_persister.h    | 25 +++++----
 net/base/transport_security_state.cc             | 11 ----
 net/base/transport_security_state.h              |  3 --
 7 files changed, 56 insertions(+), 63 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_
diff --git a/net/base/transport_security_state.cc b/net/base/transport_security_state.cc
index d033b94..c8b86f5 100644
--- a/net/base/transport_security_state.cc
+++ b/net/base/transport_security_state.cc
@@ -41,8 +41,6 @@ void TransportSecurityState::EnableHost(const std::string& host,
   if (IsEnabledForHost(&existing_state, host))
     state_copy.created = existing_state.created;
 
-  AutoLock lock(lock_);
-
   enabled_hosts_[std::string(hashed, sizeof(hashed))] = state_copy;
   DirtyNotify();
 }
@@ -62,7 +60,6 @@ bool TransportSecurityState::IsEnabledForHost(DomainState* result,
   }
 
   base::Time current_time(base::Time::Now());
-  AutoLock lock(lock_);
 
   for (size_t i = 0; canonicalised_host[i]; i += canonicalised_host[i] + 1) {
     char hashed_domain[base::SHA256_LENGTH];
@@ -194,8 +191,6 @@ bool TransportSecurityState::ParseHeader(const std::string& value,
 
 void TransportSecurityState::SetDelegate(
     TransportSecurityState::Delegate* delegate) {
-  AutoLock lock(lock_);
-
   delegate_ = delegate;
 }
 
@@ -221,8 +216,6 @@ static std::string ExternalStringToHashedDomain(const std::wstring& external) {
 }
 
 bool TransportSecurityState::Serialise(std::string* output) {
-  AutoLock lock(lock_);
-
   DictionaryValue toplevel;
   for (std::map<std::string, DomainState>::const_iterator
        i = enabled_hosts_.begin(); i != enabled_hosts_.end(); ++i) {
@@ -256,8 +249,6 @@ bool TransportSecurityState::Serialise(std::string* output) {
 
 bool TransportSecurityState::Deserialise(const std::string& input,
                                          bool* dirty) {
-  AutoLock lock(lock_);
-
   enabled_hosts_.clear();
 
   scoped_ptr<Value> value(
@@ -335,8 +326,6 @@ bool TransportSecurityState::Deserialise(const std::string& input,
 void TransportSecurityState::DeleteSince(const base::Time& time) {
   bool dirtied = false;
 
-  AutoLock lock(lock_);
-
   std::map<std::string, DomainState>::iterator i = enabled_hosts_.begin();
   while (i != enabled_hosts_.end()) {
     if (i->second.created >= time) {
diff --git a/net/base/transport_security_state.h b/net/base/transport_security_state.h
index 05a0fc1..934db78 100644
--- a/net/base/transport_security_state.h
+++ b/net/base/transport_security_state.h
@@ -102,9 +102,6 @@ class TransportSecurityState :
   // ('www.google.com') to the form used in DNS: "\x03www\x06google\x03com"
   std::map<std::string, DomainState> enabled_hosts_;
 
-  // Protect access to our data members with this lock.
-  Lock lock_;
-
   // Our delegate who gets notified when we are dirtied, or NULL.
   Delegate* delegate_;
 
-- 
cgit v1.1