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

Improves the database by not inserting blatantly erroneous data, and by deleting
any events or metrics which cannot be properly retrieved from the database.
Also make DB take a metric for AddMetric() - this makes it so that we have a
more accurate timestamp since we record the time before the thread-jumping.

BUG=130212


Review URL: https://chromiumcodereview.appspot.com/10907121

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@157168 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 396 insertions, 106 deletions

diff --git a/chrome/browser/performance_monitor/constants.cc b/chrome/browser/performance_monitor/constants.cc
index 8efa06a..dd9a115 100644
--- a/chrome/browser/performance_monitor/constants.cc
+++ b/chrome/browser/performance_monitor/constants.cc
@@ -4,9 +4,9 @@
 
 #include "chrome/browser/performance_monitor/constants.h"
 
-namespace performance_monitor {
+#include "base/time.h"
 
-// TODO(chebert): i18n on all constants.
+namespace performance_monitor {
 
 // The error message displayed when a metric's details are not found.
 const char kMetricNotFoundError[] = "Metric details not found.";
@@ -28,4 +28,16 @@ const char kStateChromeVersion[] = "chrome_version";
 // collisions in the database.
 const char kStateProfilePrefix[] = "profile";
 
+// Unit values
+// Memory measurements
+const int64 kBytesPerKilobyte = 1 << 10;
+const int64 kBytesPerMegabyte = kBytesPerKilobyte * (1 << 10);
+const int64 kBytesPerGigabyte = kBytesPerMegabyte * (1 << 10);
+const int64 kBytesPerTerabyte = kBytesPerGigabyte * (1 << 10);
+
+// Time measurements
+const int64 kMicrosecondsPerMonth = base::Time::kMicrosecondsPerDay * 30;
+const int64 kMicrosecondsPerYear = base::Time::kMicrosecondsPerDay * 365;
+
+
 }  // namespace performance_monitor
diff --git a/chrome/browser/performance_monitor/constants.h b/chrome/browser/performance_monitor/constants.h
index 84681f9..24009da 100644
--- a/chrome/browser/performance_monitor/constants.h
+++ b/chrome/browser/performance_monitor/constants.h
@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_PERFORMANCE_MONITOR_CONSTANTS_H_
 #define CHROME_BROWSER_PERFORMANCE_MONITOR_CONSTANTS_H_
 
+#include "base/basictypes.h"
+
 namespace performance_monitor {
 
 // Constants which are used by the PerformanceMonitor and its related classes.
@@ -19,6 +21,21 @@ extern const int kDefaultGatherIntervalInSeconds;
 extern const char kStateChromeVersion[];
 extern const char kStateProfilePrefix[];
 
+// Unit values (for use in metric, and on the UI side).
+
+// Memory measurements
+extern const int64 kBytesPerKilobyte;
+extern const int64 kBytesPerMegabyte;
+extern const int64 kBytesPerGigabyte;
+extern const int64 kBytesPerTerabyte;
+
+// Time measurements - Most of these are imported from base/time.h
+// These units are used for display (and it's related calculations), not for
+// any mathematical analysis. Thus we can estimate for values without an exact
+// conversion.
+extern const int64 kMicrosecondsPerMonth;
+extern const int64 kMicrosecondsPerYear;
+
 }  // namespace performance_monitor
 
 #endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_CONSTANTS_H_
diff --git a/chrome/browser/performance_monitor/database.cc b/chrome/browser/performance_monitor/database.cc
index 27108e3..e67ab9c 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 {
@@ -50,6 +52,17 @@ double StringToDouble(const std::string& s) {
   return value;
 }
 
+// Returns an event from the given JSON string; the scoped_ptr will be NULL if
+// we are unable to properly parse the JSON.
+scoped_ptr<Event> EventFromJSON(const std::string& data) {
+  Value* value = base::JSONReader::Read(data);
+  DictionaryValue* dict = NULL;
+  if (!value || !value->GetAsDictionary(&dict))
+    return scoped_ptr<Event>();
+
+  return Event::FromValue(scoped_ptr<DictionaryValue>(dict));
+}
+
 }  // namespace
 
 const char Database::kDatabaseSequenceToken[] =
@@ -145,6 +158,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 +169,17 @@ Database::EventVector Database::GetEvents(EventType type,
       if (key_type != type)
         continue;
     }
-    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 = EventFromJSON(it->value().ToString());
+    if (!event.get()) {
+      invalid_entries.Delete(it->key());
+      LOG(ERROR) << "Found invalid event in the database. JSON: '"
+                 <<  it->value().ToString()
+                 << "'. Erasing 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 +203,34 @@ 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.IsValid()) {
+    LOG(ERROR) << "Metric to be added is invalid. Type: " << metric.type
+               << ", Time: " << metric.time.ToInternalValue()
+               << ", Value: " << metric.value << ". Ignoring.";
+    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 +340,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 +357,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.IsValid()) {
+        invalid_entries.Delete(it->key());
+        LOG(ERROR) << "Found bad metric in the database. Type: "
+                   << metric.type << ", Time: " << metric.time.ToInternalValue()
+                   << ", Value: " << metric.value
+                   << ". Erasing metric from database.";
+        continue;
+      }
+      results.push_back(metric);
+    }
   }
+  metric_db_->Write(write_options_, &invalid_entries);
   return results;
 }
 
@@ -358,6 +391,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 +401,18 @@ Database::MetricVectorMap Database::GetStatsForMetricByActivity(
       results[split_key.activity] =
           linked_ptr<MetricVector >(new MetricVector());
     }
-    results[split_key.activity]->push_back(
-        Metric(split_key.time, it->value().ToString()));
+    Metric metric(metric_type, split_key.time, it->value().ToString());
+    if (!metric.IsValid()) {
+      invalid_entries.Delete(it->key());
+      LOG(ERROR) << "Found bad metric in the database. Type: "
+                 << metric.type << ", Time: " << metric.time.ToInternalValue()
+                 << ", Value: " << metric.value
+                 << ". Erasing metric from database.";
+      continue;
+    }
+    results[split_key.activity]->push_back(metric);
   }
+  metric_db_->Write(write_options_, &invalid_entries);
   return results;
 }
 
diff --git a/chrome/browser/performance_monitor/database.h b/chrome/browser/performance_monitor/database.h
index 8cec7fc..ce41cb6 100644
--- a/chrome/browser/performance_monitor/database.h
+++ b/chrome/browser/performance_monitor/database.h
@@ -31,6 +31,7 @@ struct TimeRange {
 };
 
 class KeyBuilder;
+class DatabaseTestHelper;
 
 // The class supporting all performance monitor storage. This class wraps
 // multiple leveldb::DB objects. All methods must be called from a background
@@ -152,12 +153,10 @@ class Database {
   }
 
   // Add a metric instance to the database.
-  bool AddMetric(const std::string& activity,
-                 MetricType metric_type,
-                 const std::string& value);
+  bool AddMetric(const std::string& activity, const Metric& metric);
 
-  bool AddMetric(MetricType metric_type, const std::string& value) {
-    return AddMetric(kProcessChromeAggregate, metric_type, value);
+  bool AddMetric(const Metric& metric) {
+    return AddMetric(kProcessChromeAggregate, metric);
   }
 
   // Get the metrics that are active for the given process between |start|
@@ -236,8 +235,7 @@ class Database {
   }
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest, OpenClose);
-  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest, ActiveInterval);
+  friend class DatabaseTestHelper;
 
   typedef std::map<std::string, std::string> RecentMap;
   typedef std::map<std::string, double> MaxValueMap;
diff --git a/chrome/browser/performance_monitor/database_unittest.cc b/chrome/browser/performance_monitor/database_unittest.cc
index 4fef765..2a887e2 100644
--- a/chrome/browser/performance_monitor/database_unittest.cc
+++ b/chrome/browser/performance_monitor/database_unittest.cc
@@ -12,15 +12,72 @@
 #include "base/scoped_temp_dir.h"
 #include "base/time.h"
 #include "chrome/browser/performance_monitor/database.h"
+#include "chrome/browser/performance_monitor/key_builder.h"
 #include "chrome/browser/performance_monitor/metric.h"
 #include "chrome/browser/performance_monitor/performance_monitor_util.h"
 #include "chrome/common/extensions/extension.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/iterator.h"
 
 using extensions::Extension;
 
 namespace performance_monitor {
 
+// A class which is friended by Database, in order to hold the private methods
+// and avoid friending all the different test classes.
+class DatabaseTestHelper {
+ public:
+  explicit DatabaseTestHelper(Database* database) : database_(database) { };
+  ~DatabaseTestHelper() { };
+
+  bool Close() { return database_->Close(); }
+
+  // Override the check for a metric's validity and insert it in the database.
+  // Note: This does not do extraneous updates, like updating the recent_db or
+  // the max_value_map.
+  bool AddInvalidMetric(std::string activity, Metric metric) {
+    std::string metric_key =
+        database_->key_builder_->CreateMetricKey(database_->clock_->GetTime(),
+                                                 metric.type,
+                                                 activity);
+    leveldb::Status status =
+        database_->metric_db_->Put(database_->write_options_,
+                                   metric_key,
+                                   metric.ValueAsString());
+    return status.ok();
+  }
+
+  // Writes an invalid event to the database; since events are stored as JSON
+  // strings, this is equivalent to writing a garbage string.
+  bool AddInvalidEvent(base::Time time, EventType type) {
+    std::string key = database_->key_builder_->CreateEventKey(time, type);
+    leveldb::Status status =
+        database_->event_db_->Put(database_->write_options_, key, "fake_event");
+    return status.ok();
+  }
+
+  size_t GetNumberOfMetricEntries() {
+    return GetNumberOfEntries(database_->metric_db_.get());
+  }
+  size_t GetNumberOfEventEntries() {
+    return GetNumberOfEntries(database_->event_db_.get());
+  }
+
+ private:
+  // Returns the number of entries in a given database.
+  size_t GetNumberOfEntries(leveldb::DB* level_db) {
+    size_t number_of_entries = 0;
+    scoped_ptr<leveldb::Iterator> iter(
+        level_db->NewIterator(database_->read_options_));
+    for (iter->SeekToFirst(); iter->Valid(); iter->Next())
+      ++number_of_entries;
+    return number_of_entries;
+  }
+
+  Database* database_;
+};
+
 // A clock that increments every access. Great for testing.
 class TestingClock : public Database::Clock {
  public:
@@ -107,14 +164,18 @@ class PerformanceMonitorDatabaseMetricTest : public ::testing::Test {
   }
 
   void PopulateDB() {
-    db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE,
-                   std::string("50.5"));
-    db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("13.1"));
-
-    db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                   std::string("1000000"));
-    db_->AddMetric(activity_, METRIC_PRIVATE_MEMORY_USAGE,
-                   std::string("3000000"));
+    db_->AddMetric(kProcessChromeAggregate,
+                   Metric(METRIC_CPU_USAGE, clock_->GetTime(), 50.5));
+    db_->AddMetric(activity_,
+                   Metric(METRIC_CPU_USAGE, clock_->GetTime(), 13.1));
+    db_->AddMetric(kProcessChromeAggregate,
+                   Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                          clock_->GetTime(),
+                          1000000.0));
+    db_->AddMetric(activity_,
+                   Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                          clock_->GetTime(),
+                          3000000.0));
   }
 
   scoped_ptr<Database> db_;
@@ -129,7 +190,8 @@ TEST(PerformanceMonitorDatabaseSetupTest, OpenClose) {
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   scoped_ptr<Database> db = Database::Create(temp_dir.path());
   ASSERT_TRUE(db.get());
-  ASSERT_TRUE(db->Close());
+  DatabaseTestHelper helper(db.get());
+  ASSERT_TRUE(helper.Close());
 }
 
 TEST(PerformanceMonitorDatabaseSetupTest, ActiveInterval) {
@@ -141,17 +203,19 @@ TEST(PerformanceMonitorDatabaseSetupTest, ActiveInterval) {
 
   scoped_ptr<Database> db_1 = Database::Create(temp_dir.path());
   db_1->set_clock(scoped_ptr<Database::Clock>(clock_1));
+  DatabaseTestHelper helper1(db_1.get());
   db_1->AddStateValue("test", "test");
   ASSERT_TRUE(db_1.get());
-  ASSERT_TRUE(db_1->Close());
+  ASSERT_TRUE(helper1.Close());
 
   TestingClock* clock_2 = new TestingClock(*clock_1);
   base::Time mid_time = clock_2->GetTime();
   scoped_ptr<Database> db_2 = Database::Create(temp_dir.path());
   db_2->set_clock(scoped_ptr<Database::Clock>(clock_2));
+  DatabaseTestHelper helper2(db_2.get());
   db_2->AddStateValue("test", "test");
   ASSERT_TRUE(db_2.get());
-  ASSERT_TRUE(db_2->Close());
+  ASSERT_TRUE(helper2.Close());
 
   TestingClock* clock_3 = new TestingClock(*clock_2);
   base::Time end_time = clock_3->GetTime();
@@ -225,13 +289,15 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetMaxMetric) {
   EXPECT_EQ(1000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 
-  db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                 std::string("99"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_PRIVATE_MEMORY_USAGE, clock_->GetTime(), 99.0));
   EXPECT_EQ(1000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 
-  db_->AddMetric(kProcessChromeAggregate, METRIC_PRIVATE_MEMORY_USAGE,
-                 std::string("6000000"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                        clock_->GetTime(),
+                        6000000.0));
   EXPECT_EQ(6000000,
             db_->GetMaxStatsForActivityAndMetric(METRIC_PRIVATE_MEMORY_USAGE));
 }
@@ -260,6 +326,23 @@ TEST_F(PerformanceMonitorDatabaseEventTest, GetInstallEvents) {
   EXPECT_TRUE(events[1]->data()->Equals(install_event_2_->data()));
 }
 
+TEST_F(PerformanceMonitorDatabaseEventTest, InvalidEvents) {
+  DatabaseTestHelper helper(db_.get());
+
+  // Insert an invalid event into the database; verify it is inserted.
+  size_t original_number_of_entries = helper.GetNumberOfEventEntries();
+  ASSERT_TRUE(helper.AddInvalidEvent(
+      clock_->GetTime(), EVENT_EXTENSION_INSTALL));
+  ASSERT_EQ(original_number_of_entries + 1u, helper.GetNumberOfEventEntries());
+
+  // Should not retrieve the invalid event.
+  Database::EventVector events = db_->GetEvents();
+  ASSERT_EQ(original_number_of_entries, events.size());
+
+  // Invalid event should have been deleted.
+  ASSERT_EQ(original_number_of_entries, helper.GetNumberOfEventEntries());
+}
+
 TEST_F(PerformanceMonitorDatabaseEventTest, GetUnusedEventType) {
   Database::EventVector events = db_->GetEvents(EVENT_EXTENSION_DISABLE);
   ASSERT_TRUE(events.empty());
@@ -343,7 +426,8 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForActivityAndMetric) {
   ASSERT_EQ(1u, stats.size());
   EXPECT_EQ(13.1, stats[0].value);
   base::Time before = clock_->GetTime();
-  db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("18"));
+  db_->AddMetric(activity_,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 18.0));
   stats = db_->GetStatsForActivityAndMetric(activity_, METRIC_CPU_USAGE,
                                             before, clock_->GetTime());
   ASSERT_EQ(1u, stats.size());
@@ -375,7 +459,8 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForMetricByActivity) {
       METRIC_CPU_USAGE, clock_->GetTime(), clock_->GetTime());
   EXPECT_EQ(0u, stats_map.size());
   base::Time before = clock_->GetTime();
-  db_->AddMetric(activity_, METRIC_CPU_USAGE, std::string("18"));
+  db_->AddMetric(activity_,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 18.0));
   stats_map = db_->GetStatsForMetricByActivity(METRIC_CPU_USAGE, before,
                                                clock_->GetTime());
   ASSERT_EQ(1u, stats_map.size());
@@ -384,9 +469,43 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetStatsForMetricByActivity) {
   EXPECT_EQ(18, stats->at(0).value);
 }
 
+TEST_F(PerformanceMonitorDatabaseMetricTest, InvalidMetrics) {
+  DatabaseTestHelper helper(db_.get());
+  Metric invalid_metric(METRIC_CPU_USAGE, clock_->GetTime(), -5.0);
+  ASSERT_FALSE(invalid_metric.IsValid());
+
+  // Find the original number of entries in the database.
+  size_t original_number_of_entries = helper.GetNumberOfMetricEntries();
+  Database::MetricVector stats = db_->GetStatsForActivityAndMetric(
+      activity_, METRIC_CPU_USAGE, base::Time(), clock_->GetTime());
+  size_t original_number_of_cpu_entries = stats.size();
+
+  // Check that the database normally refuses to insert bad metrics.
+  EXPECT_FALSE(db_->AddMetric(invalid_metric));
+
+  // Verify that it was not inserted into the database.
+  stats = db_->GetStatsForActivityAndMetric(
+      activity_, METRIC_CPU_USAGE, base::Time(), clock_->GetTime());
+  ASSERT_EQ(original_number_of_cpu_entries, stats.size());
+
+  // Forcefully insert it into the database.
+  ASSERT_TRUE(helper.AddInvalidMetric(activity_, invalid_metric));
+  ASSERT_EQ(original_number_of_entries + 1u, helper.GetNumberOfMetricEntries());
+
+  // Try to retrieve it; should only get one result.
+  stats = db_->GetStatsForActivityAndMetric(
+      activity_, METRIC_CPU_USAGE, base::Time(), clock_->GetTime());
+  ASSERT_EQ(original_number_of_cpu_entries, stats.size());
+
+  // Entry should have been deleted in the database.
+  ASSERT_EQ(original_number_of_entries, helper.GetNumberOfMetricEntries());
+}
+
 TEST_F(PerformanceMonitorDatabaseMetricTest, GetFullRange) {
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("3.4"));
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("21"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 3.4));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 21.0));
   Database::MetricVector stats =
       db_->GetStatsForActivityAndMetric(METRIC_CPU_USAGE);
   ASSERT_EQ(3u, stats.size());
@@ -397,10 +516,13 @@ TEST_F(PerformanceMonitorDatabaseMetricTest, GetFullRange) {
 
 TEST_F(PerformanceMonitorDatabaseMetricTest, GetRange) {
   base::Time start = clock_->GetTime();
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("3"));
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("9"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 3.0));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 9.0));
   base::Time end = clock_->GetTime();
-  db_->AddMetric(kProcessChromeAggregate, METRIC_CPU_USAGE, std::string("21"));
+  db_->AddMetric(kProcessChromeAggregate,
+                 Metric(METRIC_CPU_USAGE, clock_->GetTime(), 21.0));
   Database::MetricVector stats =
       db_->GetStatsForActivityAndMetric(METRIC_CPU_USAGE, start, end);
   ASSERT_EQ(2u, stats.size());
diff --git a/chrome/browser/performance_monitor/metric.cc b/chrome/browser/performance_monitor/metric.cc
index 4d105f7..2f28b0f 100644
--- a/chrome/browser/performance_monitor/metric.cc
+++ b/chrome/browser/performance_monitor/metric.cc
@@ -4,21 +4,81 @@
 
 #include "chrome/browser/performance_monitor/metric.h"
 
+#include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
+#include "base/time.h"
+#include "chrome/browser/performance_monitor/constants.h"
 
 namespace performance_monitor {
 
+namespace {
+
+// For certain metrics (for instance, bytes read), it is possible that there is
+// no maximum value which we can safely assume.
+const double kNoMaximum = -1.0;
+
+// These constants are designed to keep metrics reasonable. However, due to the
+// variety of system configurations which can run chrome, these values may not
+// catch *all* erroneous values. For instance, on a one-CPU machine, any CPU
+// usage > 100 is erroneous, but on a 16-CPU machine, it's perfectly normal.
+// These are "best-guesses" in order to weed out obviously-false values. A
+// metric is valid if it is greater than or equal to the minimum and less than
+// the maximum, i.e. if it falls in the range [min, max).
+const double kMinUndefined = 0.0;
+const double kMaxUndefined = 0.0;  // No undefined metric is valid.
+const double kMinCpuUsage = 0.0;
+const double kMaxCpuUsage = 100000.0;  // 100% on a 1000-CPU machine.
+const double kMinPrivateMemoryUsage = 0.0;
+const double kMaxPrivateMemoryUsage = kBytesPerTerabyte;
+const double kMinSharedMemoryUsage = 0.0;
+const double kMaxSharedMemoryUsage = kBytesPerTerabyte;
+const double kMinStartupTime = 0.0;
+const double kMaxStartupTime = base::Time::kMicrosecondsPerMinute * 15.0;
+const double kMinTestStartupTime = 0.0;
+const double kMaxTestStartupTime = base::Time::kMicrosecondsPerMinute * 15.0;
+const double kMinSessionRestoreTime = 0.0;
+const double kMaxSessionRestoreTime = base::Time::kMicrosecondsPerMinute * 15.0;
+const double kMinPageLoadTime = 0.0;
+const double kMaxPageLoadTime = base::Time::kMicrosecondsPerMinute * 15.0;
+const double kMinNetworkBytesRead = 0.0;
+const double kMaxNetworkBytesRead = kNoMaximum;
+
+struct MetricBound {
+  double min;
+  double max;
+};
+
+const MetricBound kMetricBounds[] = {
+ { kMinUndefined, kMaxUndefined },
+ { kMinCpuUsage, kMaxCpuUsage },
+ { kMinPrivateMemoryUsage, kMaxPrivateMemoryUsage },
+ { kMinSharedMemoryUsage, kMaxSharedMemoryUsage },
+ { kMinStartupTime, kMaxStartupTime },
+ { kMinTestStartupTime, kMaxTestStartupTime },
+ { kMinSessionRestoreTime, kMaxSessionRestoreTime },
+ { kMinPageLoadTime, kMaxPageLoadTime },
+ { kMinNetworkBytesRead, kMaxNetworkBytesRead },
+};
+
+COMPILE_ASSERT(ARRAYSIZE_UNSAFE(kMetricBounds) == METRIC_NUMBER_OF_METRICS,
+               metric_bounds_size_doesnt_match_metric_count);
+
+}  // namespace
+
 Metric::Metric() {
   value = 0.0;
 }
 
-Metric::Metric(const base::Time& metric_time, const double metric_value)
-    : time(metric_time), value(metric_value) {
+Metric::Metric(MetricType metric_type,
+               const base::Time& metric_time,
+               const double metric_value)
+    : type(metric_type), time(metric_time), value(metric_value) {
 }
 
-Metric::Metric(const std::string& metric_time,
-               const std::string& metric_value) {
+Metric::Metric(MetricType metric_type,
+               const std::string& metric_time,
+               const std::string& metric_value) : type(metric_type) {
   int64 conversion = 0;
   base::StringToInt64(metric_time, &conversion);
   time = base::Time::FromInternalValue(conversion);
@@ -28,4 +88,15 @@ Metric::Metric(const std::string& metric_time,
 Metric::~Metric() {
 }
 
+bool Metric::IsValid() const {
+  return type < METRIC_NUMBER_OF_METRICS &&
+      (value < kMetricBounds[type].max ||
+          kMetricBounds[type].max == kNoMaximum) &&
+      value >= kMetricBounds[type].min;
+}
+
+std::string Metric::ValueAsString() const {
+  return base::DoubleToString(value);
+}
+
 }  // namespace performance_monitor
diff --git a/chrome/browser/performance_monitor/metric.h b/chrome/browser/performance_monitor/metric.h
index 76e8ded..79b9ed7 100644
--- a/chrome/browser/performance_monitor/metric.h
+++ b/chrome/browser/performance_monitor/metric.h
@@ -32,10 +32,25 @@ METRIC_NUMBER_OF_METRICS
 struct Metric {
  public:
   Metric();
-  Metric(const base::Time& metric_time, const double metric_value);
-  Metric(const std::string& metric_time, const std::string& metric_value);
+  Metric(MetricType metric_type,
+         const base::Time& metric_time,
+         const double metric_value);
+  Metric(MetricType metric_type,
+         const std::string& metric_time,
+         const std::string& metric_value);
   ~Metric();
 
+  // Check the value in the metric to make sure that it is reasonable. Since
+  // some metric-gathering methods will fail and return incorrect values, we
+  // need to try to weed these out as best we can.
+  bool IsValid() const;
+
+  // This converts the double stored in value to a string format. This will
+  // not perform any checking on the validity of the metric, and only makes
+  // sense if the metric IsValid().
+  std::string ValueAsString() const;
+
+  MetricType type;
   base::Time time;
   double value;
 };
diff --git a/chrome/browser/performance_monitor/performance_monitor.cc b/chrome/browser/performance_monitor/performance_monitor.cc
index effe470..d9fee3f 100644
--- a/chrome/browser/performance_monitor/performance_monitor.cc
+++ b/chrome/browser/performance_monitor/performance_monitor.cc
@@ -278,10 +278,9 @@ void PerformanceMonitor::AddEventOnBackgroundThread(scoped_ptr<Event> event) {
   database_->AddEvent(*event.get());
 }
 
-void PerformanceMonitor::AddMetricOnBackgroundThread(MetricType type,
-                                                     const std::string& value) {
+void PerformanceMonitor::AddMetricOnBackgroundThread(Metric metric) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  database_->AddMetric(type, value);
+  database_->AddMetric(metric);
 }
 
 void PerformanceMonitor::NotifyInitialized() {
@@ -312,7 +311,9 @@ void PerformanceMonitor::GatherCPUUsageOnBackgroundThread() {
       cpu_usage += iter->second->GetCPUUsage();
     }
 
-    database_->AddMetric(METRIC_CPU_USAGE, base::DoubleToString(cpu_usage));
+    database_->AddMetric(Metric(METRIC_CPU_USAGE,
+                                base::Time::Now(),
+                                cpu_usage));
   }
 }
 
@@ -331,10 +332,12 @@ void PerformanceMonitor::GatherMemoryUsageOnBackgroundThread() {
     }
   }
 
-  database_->AddMetric(METRIC_PRIVATE_MEMORY_USAGE,
-                       base::Uint64ToString(private_memory_sum));
-  database_->AddMetric(METRIC_SHARED_MEMORY_USAGE,
-                       base::Uint64ToString(shared_memory_sum));
+  database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                              base::Time::Now(),
+                              static_cast<double>(private_memory_sum)));
+  database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE,
+                              base::Time::Now(),
+                              static_cast<double>(shared_memory_sum)));
 }
 
 void PerformanceMonitor::UpdateMetricsMapOnBackgroundThread() {
@@ -432,8 +435,10 @@ void PerformanceMonitor::InsertIOData(
     PerformanceDataForIOThread performance_data_for_io_thread) {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
   database_->AddMetric(
-      METRIC_NETWORK_BYTES_READ,
-      base::Uint64ToString(performance_data_for_io_thread.network_bytes_read));
+      Metric(METRIC_NETWORK_BYTES_READ,
+             base::Time::Now(),
+             static_cast<double>(
+                 performance_data_for_io_thread.network_bytes_read)));
 }
 
 void PerformanceMonitor::BytesReadOnIOThread(const net::URLRequest& request,
@@ -521,8 +526,10 @@ void PerformanceMonitor::Observe(int type,
           base::Bind(
               &PerformanceMonitor::AddMetricOnBackgroundThread,
               base::Unretained(this),
-              METRIC_PAGE_LOAD_TIME,
-              base::Int64ToString(load_details->load_time.ToInternalValue())));
+              Metric(METRIC_PAGE_LOAD_TIME,
+                     base::Time::Now(),
+                     static_cast<double>(
+                         load_details->load_time.ToInternalValue()))));
       break;
     }
     default: {
diff --git a/chrome/browser/performance_monitor/performance_monitor.h b/chrome/browser/performance_monitor/performance_monitor.h
index a51253c..0dd1dda 100644
--- a/chrome/browser/performance_monitor/performance_monitor.h
+++ b/chrome/browser/performance_monitor/performance_monitor.h
@@ -131,8 +131,9 @@ class PerformanceMonitor : public content::NotificationObserver {
   void AddEventOnBackgroundThread(scoped_ptr<Event> event);
 
   // Since Database::AddMetric() is overloaded, base::Bind() does not work and
-  // we need a helper function.
-  void AddMetricOnBackgroundThread(MetricType type, const std::string& value);
+  // we need a helper function. Deliberately not const & so that we will
+  // construct a new metric on the background thread.
+  void AddMetricOnBackgroundThread(Metric metric);
 
   // Notify any listeners that PerformanceMonitor has finished the initializing.
   void NotifyInitialized();
diff --git a/chrome/browser/performance_monitor/startup_timer.cc b/chrome/browser/performance_monitor/startup_timer.cc
index 0af0a57..c6e5816 100644
--- a/chrome/browser/performance_monitor/startup_timer.cc
+++ b/chrome/browser/performance_monitor/startup_timer.cc
@@ -21,9 +21,8 @@ namespace performance_monitor {
 namespace {
 // Needed because Database::AddMetric is overloaded, so base::Bind doesn't work.
 void AddMetricToDatabaseOnBackgroundThread(Database* database,
-                                           MetricType metric,
-                                           std::string value) {
-  database->AddMetric(metric, value);
+                                           Metric metric) {
+  database->AddMetric(metric);
 }
 
 }  // namespace
@@ -114,9 +113,11 @@ void StartupTimer::InsertElapsedStartupTime() {
       base::Bind(
           &AddMetricToDatabaseOnBackgroundThread,
           base::Unretained(PerformanceMonitor::GetInstance()->database()),
-          startup_type_ == STARTUP_NORMAL ? METRIC_STARTUP_TIME
-                                          : METRIC_TEST_STARTUP_TIME,
-          base::Int64ToString(elapsed_startup_time_.ToInternalValue())));
+          Metric(startup_type_ == STARTUP_NORMAL ? METRIC_STARTUP_TIME
+                                                 : METRIC_TEST_STARTUP_TIME,
+                 base::Time::Now(),
+                 static_cast<double>(
+                     elapsed_startup_time_.ToInternalValue()))));
 }
 
 void StartupTimer::InsertElapsedSessionRestoreTime() {
@@ -129,8 +130,9 @@ void StartupTimer::InsertElapsedSessionRestoreTime() {
         base::Bind(
             &AddMetricToDatabaseOnBackgroundThread,
             base::Unretained(PerformanceMonitor::GetInstance()->database()),
-            METRIC_SESSION_RESTORE_TIME,
-            base::Int64ToString(iter->ToInternalValue())));
+            Metric(METRIC_SESSION_RESTORE_TIME,
+                   base::Time::Now(),
+                   static_cast<double>(iter->ToInternalValue()))));
   }
 }
 
diff --git a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_constants.cc b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_constants.cc
index 1fd6c3c..37c463c 100644
--- a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_constants.cc
+++ b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_constants.cc
@@ -5,25 +5,13 @@
 #include "chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_constants.h"
 
 #include "base/logging.h"
+#include "chrome/browser/performance_monitor/constants.h"
 #include "base/time.h"
 
 namespace performance_monitor {
 
 namespace {
 
-// Memory measurements
-const int64 kBytesPerKilobyte = 1 << 10;
-const int64 kBytesPerMegabyte = kBytesPerKilobyte * (1 << 10);
-const int64 kBytesPerGigabyte = kBytesPerMegabyte * (1 << 10);
-const int64 kBytesPerTerabyte = kBytesPerGigabyte * (1 << 10);
-
-// Time measurements - Most of these are imported from base/time.h
-// These units are used for display (and it's related calculations), not for
-// any mathematical analysis. Thus we can estimate for values without an exact
-// conversion.
-const int64 kMicrosecondsPerMonth = base::Time::kMicrosecondsPerDay * 30;
-const int64 kMicrosecondsPerYear = base::Time::kMicrosecondsPerDay * 365;
-
 // Keep this list synced with the enum declared in the .h file.
 const UnitDetails kUnitDetailsList[] = {
   { UNIT_BYTES, MEASUREMENT_TYPE_MEMORY, 1 },
diff --git a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.cc b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.cc
index bf3e8f0..b2bd17c 100644
--- a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.cc
+++ b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.cc
@@ -20,6 +20,7 @@ double GetConversionFactor(UnitDetails from, UnitDetails to) {
 }
 
 scoped_ptr<Database::MetricVector> AggregateMetric(
+    MetricType type,
     const Database::MetricVector* metrics,
     const base::Time& start,
     const base::TimeDelta& resolution) {
@@ -56,7 +57,7 @@ scoped_ptr<Database::MetricVector> AggregateMetric(
     // at the end of the window.
     integrated += metric_value * (window_end - last_sample_time).InSecondsF();
     double average = integrated / resolution.InSecondsF();
-    results->push_back(Metric(window_end, average));
+    results->push_back(Metric(type, window_end, average));
   }
   return results.Pass();
 }
diff --git a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.h b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.h
index 8064981..58691d9 100644
--- a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.h
+++ b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util.h
@@ -23,6 +23,7 @@ double GetConversionFactor(UnitDetails from, UnitDetails to);
 // |start| and data points are omitted if there are no points to resample.
 // Returns a pointer to a new MetricVector, or NULL if |metrics| is empty.
 scoped_ptr<Database::MetricVector> AggregateMetric(
+    MetricType type,
     const Database::MetricVector* metrics,
     const base::Time& start,
     const base::TimeDelta& resolution);
diff --git a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util_unittest.cc b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util_unittest.cc
index d2d7f17..851c929 100644
--- a/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util_unittest.cc
+++ b/chrome/browser/ui/webui/performance_monitor/performance_monitor_ui_util_unittest.cc
@@ -19,12 +19,12 @@ class PerformanceMonitorUtilTest : public ::testing::Test {
 TEST(PerformanceMonitorUtilTest, AggregateMetricEmptyTest) {
   Database::MetricVector metric;
   const base::Time data_time = base::Time::FromDoubleT(1);
-  metric.push_back(Metric(data_time, 1));
+  metric.push_back(Metric(METRIC_CPU_USAGE, data_time, 1));
 
   const base::Time results_time = base::Time::FromDoubleT(3);
   const base::TimeDelta resolution = base::TimeDelta::FromSeconds(1);
   scoped_ptr<Database::MetricVector> aggregated_metric =
-      AggregateMetric(&metric, results_time, resolution);
+      AggregateMetric(METRIC_CPU_USAGE, &metric, results_time, resolution);
   ASSERT_EQ(0u, aggregated_metric->size());
 }
 
@@ -35,9 +35,12 @@ TEST(PerformanceMonitorUtilTest, AggregateMetricSimpleTest) {
 
   const double value = 3.14;
   Database::MetricVector metric;
-  metric.push_back(Metric(data_time, value));
+  metric.push_back(Metric(METRIC_CPU_USAGE, data_time, value));
   scoped_ptr<Database::MetricVector> aggregated_metric =
-      AggregateMetric(&metric, results_time, results_resolution);
+      AggregateMetric(METRIC_CPU_USAGE,
+                      &metric,
+                      results_time,
+                      results_resolution);
 
   ASSERT_EQ(1u, aggregated_metric->size());
   ASSERT_EQ(results_time + results_resolution, aggregated_metric->at(0).time);
@@ -54,12 +57,17 @@ TEST(PerformanceMonitorUtilTest, AggregateMetricDenseTest) {
   Database::MetricVector metric;
 
   for (int i = 0; i < num_points; ++i) {
-    metric.push_back(Metric(current_data_time, current_value));
+    metric.push_back(Metric(METRIC_CPU_USAGE,
+                            current_data_time,
+                            current_value));
     current_value += 1;
     current_data_time += data_resolution;
   }
   scoped_ptr<Database::MetricVector> aggregated_metric =
-      AggregateMetric(&metric, results_time, results_resolution);
+      AggregateMetric(METRIC_CPU_USAGE,
+                      &metric,
+                      results_time,
+                      results_resolution);
   // The first 4 points get ignored because they are before the start time.
   // The remaining 8 points are aggregated into two data points.
   ASSERT_EQ(2u, aggregated_metric->size());
@@ -73,18 +81,21 @@ TEST(PerformanceMonitorUtilTest, AggregateMetricSparseTest) {
 
   const base::Time data_time1 = base::Time::FromDoubleT(20);
   const double value1 = 3.14;
-  metric.push_back(Metric(data_time1, value1));
+  metric.push_back(Metric(METRIC_CPU_USAGE, data_time1, value1));
   const base::Time data_time2 = base::Time::FromDoubleT(40);
   const double value2 = 6.28;
-  metric.push_back(Metric(data_time2, value2));
+  metric.push_back(Metric(METRIC_CPU_USAGE, data_time2, value2));
   const base::Time data_time3 = base::Time::FromDoubleT(60);
   const double value3 = 9.42;
-  metric.push_back(Metric(data_time3, value3));
+  metric.push_back(Metric(METRIC_CPU_USAGE, data_time3, value3));
 
   const base::Time results_time = base::Time::FromDoubleT(19);
   const base::TimeDelta results_resolution = base::TimeDelta::FromSeconds(2);
   scoped_ptr<Database::MetricVector> aggregated_metric =
-      AggregateMetric(&metric, results_time, results_resolution);
+      AggregateMetric(METRIC_CPU_USAGE,
+                      &metric,
+                      results_time,
+                      results_resolution);
 
   // The first aggregation point is split between the first value and the second
   // value. The second is split between the second and third. The third doesn't
diff --git a/chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc b/chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc
index 16c4085..fdc2776 100644
--- a/chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc
+++ b/chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc
@@ -325,7 +325,8 @@ void DoGetMetric(DictionaryValue* results,
       results,
       metric_type,
       db->GetMaxStatsForActivityAndMetric(metric_type),
-      AggregateMetric(metric_vector.get(),
+      AggregateMetric(metric_type,
+                      metric_vector.get(),
                       start,
                       resolution).get());
 }