Add callback for HSTS deletion

TransportSecurityState::DeleteAllDynamicDataSince provides no way to
wait until the deletion is finished and written to disk.
This adds a callback, so that clients can be informed when the deletion
is finished.

Bug: 795872
Change-Id: Ia0af06926d0ccfed07e229cd53b9d8ae587781c5
Reviewed-on: https://chromium-review.googlesource.com/c/1335939
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612193}

base/files/important_file_writer.cc

@@ -102,11 +102,11 @@ void LogFailure(const FilePath& path,
 void WriteScopedStringToFileAtomically(
     const FilePath& path,
     std::unique_ptr<std::string> data,
-    Closure before_write_callback,
-    Callback<void(bool success)> after_write_callback,
+    OnceClosure before_write_callback,
+    OnceCallback<void(bool success)> after_write_callback,
     const std::string& histogram_suffix) {
   if (!before_write_callback.is_null())
-    before_write_callback.Run();
+    std::move(before_write_callback).Run();
 
   TimeTicks start_time = TimeTicks::Now();
   bool result =
@@ -117,7 +117,7 @@ void WriteScopedStringToFileAtomically(
   }
 
   if (!after_write_callback.is_null())
-    after_write_callback.Run(result);
+    std::move(after_write_callback).Run(result);
 }
 
 void DeleteTmpFile(const FilePath& tmp_file_path,
@@ -297,10 +297,10 @@ void ImportantFileWriter::DoScheduledWrite() {
 }
 
 void ImportantFileWriter::RegisterOnNextWriteCallbacks(
-    const Closure& before_next_write_callback,
-    const Callback<void(bool success)>& after_next_write_callback) {
-  before_next_write_callback_ = before_next_write_callback;
-  after_next_write_callback_ = after_next_write_callback;
+    OnceClosure before_next_write_callback,
+    OnceCallback<void(bool success)> after_next_write_callback) {
+  before_next_write_callback_ = std::move(before_next_write_callback);
+  after_next_write_callback_ = std::move(after_next_write_callback);
 }
 
 void ImportantFileWriter::ClearPendingWrite() {

base/files/important_file_writer.h

@@ -106,8 +106,8 @@ class BASE_EXPORT ImportantFileWriter {
   // If called more than once before a write is scheduled on |task_runner|, the
   // latest callbacks clobber the others.
   void RegisterOnNextWriteCallbacks(
-      const Closure& before_next_write_callback,
-      const Callback<void(bool success)>& after_next_write_callback);
+      OnceClosure before_next_write_callback,
+      OnceCallback<void(bool success)> after_next_write_callback);
 
   TimeDelta commit_interval() const {
     return commit_interval_;
@@ -125,8 +125,8 @@ class BASE_EXPORT ImportantFileWriter {
   void ClearPendingWrite();
 
   // Invoked synchronously on the next write event.
-  Closure before_next_write_callback_;
-  Callback<void(bool success)> after_next_write_callback_;
+  OnceClosure before_next_write_callback_;
+  OnceCallback<void(bool success)> after_next_write_callback_;
 
   // Path being written to.
   const FilePath path_;

ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm

@@ -8,6 +8,7 @@
 #include <set>
 #include <utility>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
@@ -347,13 +348,14 @@ void ChromeBrowserStateImplIOData::ClearNetworkingHistorySinceOnIOThread(
     const base::Closure& completion) {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
   DCHECK(initialized());
-
   DCHECK(transport_security_state());
-  // Completes synchronously.
-  transport_security_state()->DeleteAllDynamicDataSince(time);
-  http_server_properties()->Clear(base::BindOnce(
-      [](const base::Closure& completion) {
-        base::PostTaskWithTraits(FROM_HERE, {web::WebThread::UI}, completion);
-      },
-      completion));
+  auto barrier = base::BarrierClosure(
+      2, base::BindOnce(
+             [](base::OnceClosure callback) {
+               base::PostTaskWithTraits(FROM_HERE, {web::WebThread::UI},
+                                        std::move(callback));
+             },
+             completion));
+  transport_security_state()->DeleteAllDynamicDataSince(time, barrier);
+  http_server_properties()->Clear(barrier);
 }

net/http/transport_security_persister.cc

@@ -246,6 +246,25 @@ void TransportSecurityPersister::StateIsDirty(TransportSecurityState* state) {
   writer_.ScheduleWrite(this);
 }
 
+void TransportSecurityPersister::WriteNow(TransportSecurityState* state,
+                                          base::OnceClosure callback) {
+  DCHECK(foreground_runner_->RunsTasksInCurrentSequence());
+  DCHECK_EQ(transport_security_state_, state);
+
+  writer_.RegisterOnNextWriteCallbacks(
+      base::OnceClosure(),
+      base::BindOnce(&TransportSecurityPersister::OnWriteFinished,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  auto data = std::make_unique<std::string>();
+  SerializeData(data.get());
+  writer_.WriteNow(std::move(data));
+}
+
+void TransportSecurityPersister::OnWriteFinished(base::OnceClosure callback,
+                                                 bool result) {
+  foreground_runner_->PostTask(FROM_HERE, std::move(callback));
+}
+
 bool TransportSecurityPersister::SerializeData(std::string* output) {
   DCHECK(foreground_runner_->RunsTasksInCurrentSequence());
 

net/http/transport_security_persister.h

@@ -66,6 +66,9 @@ class NET_EXPORT TransportSecurityPersister
 
   // Called by the TransportSecurityState when it changes its state.
   void StateIsDirty(TransportSecurityState*) override;
+  // Called when the TransportSecurityState should be written immediately.
+  void WriteNow(TransportSecurityState* state,
+                base::OnceClosure callback) override;
 
   // ImportantFileWriter::DataSerializer:
   //
@@ -118,6 +121,7 @@ class NET_EXPORT TransportSecurityPersister
                           TransportSecurityState* state);
 
   void CompleteLoad(const std::string& state);
+  void OnWriteFinished(base::OnceClosure callback, bool result);
 
   TransportSecurityState* transport_security_state_;
 

net/http/transport_security_persister_unittest.cc

@@ -153,16 +153,13 @@ TEST_F(TransportSecurityPersisterTest, SerializeData3) {
   std::string serialized;
   EXPECT_TRUE(persister_->SerializeData(&serialized));
 
-  // Persist the data to the file. For the test to be fast and not flaky, we
-  // just do it directly rather than call persister_->StateIsDirty. (That uses
-  // ImportantFileWriter, which has an asynchronous commit interval rather
-  // than block.) Use a different basename just for cleanliness.
-  base::FilePath path =
-      temp_dir_.GetPath().AppendASCII("TransportSecurityPersisterTest");
-  EXPECT_EQ(static_cast<int>(serialized.size()),
-            base::WriteFile(path, serialized.c_str(), serialized.size()));
+  // Persist the data to the file.
+  base::RunLoop run_loop;
+  persister_->WriteNow(&state_, run_loop.QuitClosure());
+  run_loop.Run();
 
   // Read the data back.
+  base::FilePath path = temp_dir_.GetPath().AppendASCII("TransportSecurity");
   std::string persisted;
   EXPECT_TRUE(base::ReadFileToString(path, &persisted));
   EXPECT_EQ(persisted, serialized);

net/http/transport_security_state.cc

@@ -897,7 +897,9 @@ void TransportSecurityState::ClearDynamicData() {
   enabled_expect_ct_hosts_.clear();
 }
 
-void TransportSecurityState::DeleteAllDynamicDataSince(const base::Time& time) {
+void TransportSecurityState::DeleteAllDynamicDataSince(
+    const base::Time& time,
+    base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
   bool dirtied = false;
@@ -934,8 +936,10 @@ void TransportSecurityState::DeleteAllDynamicDataSince(const base::Time& time) {
     ++expect_ct_iterator;
   }
 
-  if (dirtied)
-    DirtyNotify();
+  if (dirtied && delegate_)
+    delegate_->WriteNow(this, std::move(callback));
+  else
+    std::move(callback).Run();
 }
 
 TransportSecurityState::~TransportSecurityState() {

net/http/transport_security_state.h

@@ -52,6 +52,10 @@ class NET_EXPORT TransportSecurityState {
     // This function may not block and may be called with internal locks held.
     // Thus it must not reenter the TransportSecurityState object.
     virtual void StateIsDirty(TransportSecurityState* state) = 0;
+    // Same as StateIsDirty but instructs the Delegate to persist the data
+    // immediately and call |callback| when done.
+    virtual void WriteNow(TransportSecurityState* state,
+                          base::OnceClosure callback) = 0;
 
    protected:
     virtual ~Delegate() {}
@@ -417,8 +421,9 @@ class NET_EXPORT TransportSecurityState {
   // time.
   //
   // If any entries are deleted, the new state will be persisted through
-  // the Delegate (if any).
-  void DeleteAllDynamicDataSince(const base::Time& time);
+  // the Delegate (if any). Calls |callback| when data is persisted to disk.
+  void DeleteAllDynamicDataSince(const base::Time& time,
+                                 base::OnceClosure callback);
 
   // Deletes any dynamic data stored for |host| (e.g. HSTS or HPKP data).
   // If |host| doesn't have an exact entry then no action is taken. Does

net/http/transport_security_state_unittest.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/base64.h"
+#include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/json/json_reader.h"
 #include "base/metrics/field_trial.h"
@@ -731,11 +732,11 @@ TEST_F(TransportSecurityStateTest, DeleteAllDynamicDataSince) {
                 GetSampleSPKIHashes(), GURL());
   state.AddExpectCT("example.com", expiry, true, GURL());
 
-  state.DeleteAllDynamicDataSince(expiry);
+  state.DeleteAllDynamicDataSince(expiry, base::DoNothing());
   EXPECT_TRUE(state.ShouldUpgradeToSSL("example.com"));
   EXPECT_TRUE(state.HasPublicKeyPins("example.com"));
   EXPECT_TRUE(state.GetDynamicExpectCTState("example.com", &expect_ct_state));
-  state.DeleteAllDynamicDataSince(older);
+  state.DeleteAllDynamicDataSince(older, base::DoNothing());
   EXPECT_FALSE(state.ShouldUpgradeToSSL("example.com"));
   EXPECT_FALSE(state.HasPublicKeyPins("example.com"));
   EXPECT_FALSE(state.GetDynamicExpectCTState("example.com", &expect_ct_state));

services/network/network_context.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/barrier_closure.h"
 #include "base/base64.h"
 #include "base/command_line.h"
 #include "base/containers/unique_ptr_adapters.h"
@@ -731,20 +732,19 @@ size_t NetworkContext::GetNumOutstandingResolveHostRequestsForTesting() const {
 void NetworkContext::ClearNetworkingHistorySince(
     base::Time time,
     base::OnceClosure completion_callback) {
-  // TODO(mmenke): Neither of these methods waits until the changes have been
-  // commited to disk. They probably should, as most similar methods net/
-  // exposes do.
+  auto barrier = base::BarrierClosure(2, std::move(completion_callback));
 
-  // Completes synchronously.
   url_request_context_->transport_security_state()->DeleteAllDynamicDataSince(
-      time);
+      time, barrier);
 
+  // TODO(mmenke): Neither of these methods waits until the changes have been
+  // commited to disk. They probably should, as most similar methods net/
+  // exposes do.
   // May not be set in all tests.
   if (network_qualities_pref_delegate_)
     network_qualities_pref_delegate_->ClearPrefs();
 
-  url_request_context_->http_server_properties()->Clear(
-      std::move(completion_callback));
+  url_request_context_->http_server_properties()->Clear(barrier);
 }
 
 void NetworkContext::ClearHttpCache(base::Time start_time,