[sql] Track uploads of diagnostic data to prevent duplication.

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index 61aebbe0c94f8640904875d1a7d86a5627906181..122158f6519ffe5a8c219914bfb24248cc9c5851 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_file_value_serializer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ -257,6 +258,112 @@ bool Connection::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
   return true;
 }
 
+// Data is persisted in a file shared between databases in the same directory.
+// Any error reading or writing the file is considered failure.

[pkotwicz, 2015/10/15 23:31:06]

This seems to be covered by the comment in the .h file

+//
+// Top level is a dictionary storing a version number, and an array of histogram
+// tags for databases which have been dumped.

[pkotwicz, 2015/10/15 23:31:06]

It is unclear what "Top level" refers to.
How about: "The sqlite-diag file contains a dictionary with the version number
and an array of ..."

+bool Connection::RegisterIntentToUpload() const {
+  static const char* kVersionKey = "version";
+  static const char* kDiagnosticDumpsKey = "DiagnosticDumps";
+  static int kVersion = 1;
+
+  AssertIOAllowed();
+
+  if (histogram_tag_.empty())
+    return false;
+
+  if (!is_open())
+    return false;
+
+  if (in_memory_)
+    return false;
+
+  const base::FilePath db_path = DbPath();
+  if (db_path.empty())
+    return false;
+
+  // Put the collection of diagnostic data next to the databases.  In most
+  // cases, this is the profile directory, but safe-browsing stores a Cookies
+  // file in the directory above the profile directory.
+  base::FilePath breadcrumb_path(
+      db_path.DirName().Append(FILE_PATH_LITERAL("sqlite-diag")));

[Scott Hess - ex-Googler, 2015/10/15 21:24:44]

Hmm.  I am considering using this file to also track certain out-of-band status
about databases, the main short-term case being to track if a database shows
errors which are known to cause problems with memory-mapped I/O.  To the extent
that this info is optional and reflects records about the operation of the
database, I'm comfortable with "sqlite-diag", but I could see room for dispute.

Another case which has occurred to me would be to run periodic integrity checks.
 I had considered that at various points in the past, but kept running against
the question of how to prevent it from running continuously in the worst-case
scenarios.  If the technique used here works, then I think a very similar
approach would work for rate-limiting integrity checks.

[pkotwicz, 2015/10/15 23:31:06]

Fair enough

+
+  // Lock against multiple updates to the diagnostics file.  This code should
+  // seldom be called in the first place, and when called it should seldom be
+  // called for multiple databases, and when called for multiple databases there
+  // is _probably_ something systemic wrong with the user's system.  So the lock
+  // should never be contended, but when it is the database experience is
+  // already bad.
+  base::AutoLock lock(g_sqlite_init_lock.Get());
+
+  scoped_ptr<base::Value> root;
+  if (!base::PathExists(breadcrumb_path)) {
+    scoped_ptr<base::DictionaryValue> root_dict(new base::DictionaryValue());
+    root_dict->SetInteger(kVersionKey, kVersion);
+
+    scoped_ptr<base::ListValue> dumps(new base::ListValue);
+    dumps->AppendString(histogram_tag_);
+    root_dict->Set(kDiagnosticDumpsKey, dumps.Pass());
+
+    root = root_dict.Pass();
+  } else {
+    // Failure to read a valid dictionary implies that something is going wrong
+    // on the system.
+    JSONFileValueDeserializer deserializer(breadcrumb_path);
+    scoped_ptr<base::Value> read_root(
+        deserializer.Deserialize(nullptr, nullptr));
+    if (!read_root.get())
+      return false;
+    scoped_ptr<base::DictionaryValue> root_dict =
+        base::DictionaryValue::From(read_root.Pass());
+    if (!root_dict)
+      return false;
+
+    // Don't upload if the version is missing or newer.
+    int version = 0;
+    if (!root_dict->GetInteger(kVersionKey, &version) || version > kVersion)
+      return false;
+
+    base::ListValue* dumps = nullptr;
+    if (!root_dict->GetList(kDiagnosticDumpsKey, &dumps))
+      return false;
+
+    const size_t size = dumps->GetSize();
+    for (size_t i = 0; i < size; ++i) {
+      std::string s;
+
+      // Don't upload if the value isn't a string, or indicates a prior upload.
+      if (!dumps->GetString(i, &s) || s == histogram_tag_)
+        return false;
+    }
+
+    // Record intention to proceed with upload.
+    dumps->AppendString(histogram_tag_);
+    root = root_dict.Pass();
+  }
+
+  const base::FilePath breadcrumb_new =
+      breadcrumb_path.AddExtension(FILE_PATH_LITERAL("new"));
+  base::DeleteFile(breadcrumb_new, false);
+
+  // No upload if the breadcrumb file cannot be updated.
+  // TODO(shess): Consider ImportantFileWriter::WriteFileAtomically() to land
+  // the data on disk.  For now, losing the data is not a big problem, so the
+  // sync overhead would probably not be worth it.
+  JSONFileValueSerializer serializer(breadcrumb_new);
+  if (!serializer.Serialize(*root))
+    return false;
+  if (!base::PathExists(breadcrumb_new))
+    return false;
+  if (!base::ReplaceFile(breadcrumb_new, breadcrumb_path, nullptr)) {
+    base::DeleteFile(breadcrumb_new, false);
+    return false;
+  }
+
+  return true;
+}
+
 // static
 void Connection::SetErrorIgnorer(Connection::ErrorIgnorerCallback* cb) {
   CHECK(current_ignorer_cb_ == NULL);
@@ -582,6 +689,22 @@ void Connection::ReleaseCacheMemoryIfNeeded(bool implicit_change_performed) {
   sqlite3_db_release_memory(db_);
 }
 
+base::FilePath Connection::DbPath() const {
+  if (!is_open())
+    return base::FilePath();
+
+  const char* path = sqlite3_db_filename(db_, "main");
+  const base::StringPiece db_path(path);
+#if defined(OS_WIN)
+  return base::FilePath(base::UTF8ToWide(db_path));
+#elif defined(OS_POSIX)
+  return base::FilePath(db_path);
+#else
+  NOTREACHED();
+  return base::FilePath();
+#endif
+}
+
 void Connection::TrimMemory(bool aggressively) {
   if (!db_)
     return;