Add guards to metric values; erase bad events/metrics from db

chrome/browser/performance_monitor/database.cc

Index: chrome/browser/performance_monitor/database.cc
diff --git a/chrome/browser/performance_monitor/database.cc b/chrome/browser/performance_monitor/database.cc
index 27108e319b010b9969c25573ba90bc199a644942..5e4359c5da972d4b4a965a962b1b96ce0b3c9d86 100644
--- a/chrome/browser/performance_monitor/database.cc
+++ b/chrome/browser/performance_monitor/database.cc
@@ -18,6 +18,8 @@
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/iterator.h"
+#include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
 namespace performance_monitor {
 namespace {
@@ -145,6 +147,7 @@ Database::EventVector Database::GetEvents(EventType type,
       key_builder_->CreateEventKey(start, EVENT_UNDEFINED);
   std::string end_key =
       key_builder_->CreateEventKey(end, EVENT_NUMBER_OF_EVENTS);
+  leveldb::WriteBatch invalid_entries;
   scoped_ptr<leveldb::Iterator> it(event_db_->NewIterator(read_options_));
   for (it->Seek(start_key);
        it->Valid() && it->key().ToString() <= end_key;
@@ -155,18 +158,21 @@ Database::EventVector Database::GetEvents(EventType type,
       if (key_type != type)
         continue;
     }
+    Value* value = NULL;
     base::DictionaryValue* dict = NULL;
-    if (!base::JSONReader::Read(
-        it->value().ToString())->GetAsDictionary(&dict)) {
-      LOG(ERROR) << "Unable to convert database event to JSON.";
+    scoped_ptr<Event> event;
+    if (!(value = base::JSONReader::Read(it->value().ToString())) ||

[Yoyo Zhou, 2012/09/10 23:52:44]

Assignment inline inside conditionals isn't recommended for readability. Try a
helper function instead.

[Devlin, 2012/09/11 18:52:17]

Done.

+        !value->GetAsDictionary(&dict) ||
+        !(event =
+            Event::FromValue(scoped_ptr<base::DictionaryValue>(dict))).get()) {
+      invalid_entries.Delete(it->key());
+      LOG(ERROR) << "Unable to convert database event to JSON. Erasing the "

[Yoyo Zhou, 2012/09/10 23:52:44]

You should log something identifiable about the event; otherwise this message is
of very limited utility.

[Devlin, 2012/09/11 18:52:17]

Done.

+                    "event from the database.";
       continue;
     }
-    scoped_ptr<Event> event =
-        Event::FromValue(scoped_ptr<base::DictionaryValue>(dict));
-    if (!event.get())
-      continue;
     events.push_back(linked_ptr<Event>(event.release()));
   }
+  event_db_->Write(write_options_, &invalid_entries);
   return events;
 }
 
@@ -190,28 +196,32 @@ Database::EventTypeSet Database::GetEventTypes(const base::Time& start,
 }
 
 bool Database::AddMetric(const std::string& activity,
-                         MetricType metric,
-                         const std::string& value) {
+                         const Metric& metric) {
   CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!metric.Validate()) {
+    LOG(ERROR) << "Metric is not valid; ignoring.";

[Yoyo Zhou, 2012/09/10 23:52:44]

Same here. What is the invalid metric?

[Devlin, 2012/09/11 18:52:17]

Done.

+    return false;
+  }
+
   UpdateActiveInterval();
-  base::Time timestamp = clock_->GetTime();
   std::string recent_key =
-      key_builder_->CreateRecentKey(timestamp, metric, activity);
+      key_builder_->CreateRecentKey(metric.time, metric.type, activity);
   std::string metric_key =
-      key_builder_->CreateMetricKey(timestamp, metric, activity);
+      key_builder_->CreateMetricKey(metric.time, metric.type, activity);
   std::string recent_map_key =
-      key_builder_->CreateRecentMapKey(metric, activity);
+      key_builder_->CreateRecentMapKey(metric.type, activity);
   // Use recent_map_ to quickly find the key that must be removed.
   RecentMap::iterator old_it = recent_map_.find(recent_map_key);
   if (old_it != recent_map_.end())
     recent_db_->Delete(write_options_, old_it->second);
   recent_map_[recent_map_key] = recent_key;
   leveldb::Status recent_status =
-      recent_db_->Put(write_options_, recent_key, value);
+      recent_db_->Put(write_options_, recent_key, metric.ValueAsString());
   leveldb::Status metric_status =
-      metric_db_->Put(write_options_, metric_key, value);
+      metric_db_->Put(write_options_, metric_key, metric.ValueAsString());
 
-  bool max_value_success = UpdateMaxValue(activity, metric, value);
+  bool max_value_success =
+      UpdateMaxValue(activity, metric.type, metric.ValueAsString());
   return recent_status.ok() && metric_status.ok() && max_value_success;
 }
 
@@ -321,7 +331,9 @@ bool Database::GetRecentStatsForActivityAndMetric(const std::string& activity,
   std::string result;
   leveldb::Status status = recent_db_->Get(read_options_, recent_key, &result);
   if (status.ok())
-    *metric = Metric(key_builder_->SplitRecentKey(recent_key).time, result);
+    *metric = Metric(metric_type,
+                     key_builder_->SplitRecentKey(recent_key).time,
+                     result);
   return status.ok();
 }
 
@@ -336,15 +348,27 @@ Database::MetricVector Database::GetStatsForActivityAndMetric(
       key_builder_->CreateMetricKey(start, metric_type, activity);
   std::string end_key =
       key_builder_->CreateMetricKey(end, metric_type, activity);
+  leveldb::WriteBatch invalid_entries;
   scoped_ptr<leveldb::Iterator> it(metric_db_->NewIterator(read_options_));
   for (it->Seek(start_key);
        it->Valid() && it->key().ToString() <= end_key;
        it->Next()) {
     MetricKey split_key =
         key_builder_->SplitMetricKey(it->key().ToString());
-    if (split_key.activity == activity)
-      results.push_back(Metric(split_key.time, it->value().ToString()));
+    if (split_key.activity == activity) {
+      Metric metric(metric_type, split_key.time, it->value().ToString());
+      if (!metric.Validate()) {
+        LOG(ERROR) << "Found bad metric in database. Erasing the metric from "

[Yoyo Zhou, 2012/09/10 23:52:44]

Same comment, etc.

[Devlin, 2012/09/11 18:52:17]

Done.

+                      "the database.";
+        invalid_entries.Delete(it->key());
+        continue;
+      }
+      results.push_back(Metric(metric_type,

[Yoyo Zhou, 2012/09/10 23:52:44]

This is already called 'metric'

[Devlin, 2012/09/11 18:52:17]

Done.

+                               split_key.time,
+                               it->value().ToString()));
+    }
   }
+  metric_db_->Write(write_options_, &invalid_entries);
   return results;
 }
 
@@ -358,6 +382,7 @@ Database::MetricVectorMap Database::GetStatsForMetricByActivity(
       key_builder_->CreateMetricKey(start, metric_type, std::string());
   std::string end_key =
       key_builder_->CreateMetricKey(end, metric_type, std::string());
+  leveldb::WriteBatch invalid_entries;
   scoped_ptr<leveldb::Iterator> it(metric_db_->NewIterator(read_options_));
   for (it->Seek(start_key);
        it->Valid() && it->key().ToString() <= end_key;
@@ -367,9 +392,17 @@ Database::MetricVectorMap Database::GetStatsForMetricByActivity(
       results[split_key.activity] =
           linked_ptr<MetricVector >(new MetricVector());
     }
+    Metric metric(metric_type, split_key.time, it->value().ToString());
+    if (!metric.Validate()) {
+      LOG(ERROR) << "Found bad metric in database. Erasing the metric from "
+                    "the database.";
+      invalid_entries.Delete(it->key());
+      continue;
+    }
     results[split_key.activity]->push_back(
-        Metric(split_key.time, it->value().ToString()));
+        Metric(metric_type, split_key.time, it->value().ToString()));

[Yoyo Zhou, 2012/09/10 23:52:44]

ditto

[Devlin, 2012/09/11 18:52:17]

Done.

   }
+  metric_db_->Write(write_options_, &invalid_entries);
   return results;
 }