[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..5b442fdef323fe59455a607672d585352750b79d 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"
@@ -1533,4 +1534,127 @@ base::TimeTicks TimeSource::Now() {
   return base::TimeTicks::Now();
 }
 
+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
+}
+
+// Decide whether to upload based on a shared breadcrumb file in the database
+// directory.  The breadcrumb file records whether a past upload has occurred,
+// and also acts as a probe to determine if basic filesystem actions work in the
+// profile directory.
+//
+// Top level is a dictionary storing a version number, and an array of databases
+// which have been dumped.
+bool Connection::ShouldUploadDiagnosticDump() const {

[pkotwicz, 2015/10/15 18:02:29]

Nit: Can you please move this function so that it better matches the order of
the functions in the .h file?

[Scott Hess - ex-Googler, 2015/10/15 21:12:20]

I can put it after OnMemoryDump(), but the functions in this file are not
well-ordered in the first place...

+  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")));
+
+  // 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(NULL, NULL));

[pkotwicz, 2015/10/15 18:02:30]

Nit: NULL -> nullptr

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

Acknowledged.

+    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 = NULL;

[pkotwicz, 2015/10/15 18:02:29]

Nit: NULL -> nullptr

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

Acknowledged.

+    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"));

[pkotwicz, 2015/10/15 18:02:29]

Should we use base::CreateTemporaryFile() instead?

[Scott Hess - ex-Googler, 2015/10/15 21:12:20]

I'll assume you intended CreateTemporaryFileInDir(), due to the rename.

I used a reproducible path because I didn't want to worry about writing cleanup
code for orphaned temporary files, and I could not find such cleanup code for
CreateTemporaryFile() and kin.  The following delete line is not strictly
necessary, since the underlying WriteFile() should truncate if it already
exists.

Also, CreateTemporaryFileInDir() on POSIX calls mkstemp() and then closees the
resulting file descriptor.  In this case I definitely don't need a unique file,
so that seems excessive (and a little weird).  I don't even know what to make of
the Windows implementation.

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

Fair enough

+  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;
+}
+
 }  // namespace sql