Make MetricsService save compressed logs to local state.

Previously, it would persist logs in uncompressed form. 
This CL changes the code to compress logs before they're 
saved. A new pair of prefs is introduced for storing 
these logs, while reading from the old pref is still 
maintained to not lose old logs. 

Additionally, this makes the metrics log discard limit 
(currently 50k) be checked against the compressed size 
rather than the uncompressed size. 

Simplifies the format used to store logs, now simply 
storing the compressed log bytes and corresponding hash 
for each log. The size and checksum fields are removed. 
(Size was redundant while the checksum is not no longer 
necessary now that we store the log hash.) 

Deletes some tests that inspected the actual pref 
values are removed, in favor of tests that check that 
serializing and de-serializing works as expected. 

Finally, also introduces GzipUncompress() that is 
needed for tests that inspect log manager logs. 

BUG=382076
TBR=agl@chromium.org

Review URL: https://codereview.chromium.org/318203004

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

chrome/browser/metrics/metrics_service_unittest.cc

@@ -13,6 +13,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "components/metrics/compression_utils.h"
 #include "components/metrics/metrics_log.h"
 #include "components/metrics/metrics_service_observer.h"
 #include "components/metrics/metrics_state_manager.h"
@@ -209,8 +210,12 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
   log_manager->StageNextLogForUpload();
   EXPECT_TRUE(log_manager->has_staged_log());
 
+  std::string uncompressed_log;
+  EXPECT_TRUE(metrics::GzipUncompress(log_manager->staged_log(),
+                                      &uncompressed_log));
+
   metrics::ChromeUserMetricsExtension uma_log;
-  EXPECT_TRUE(uma_log.ParseFromString(log_manager->staged_log()));
+  EXPECT_TRUE(uma_log.ParseFromString(uncompressed_log));
 
   EXPECT_TRUE(uma_log.has_client_id());
   EXPECT_TRUE(uma_log.has_session_id());

components/components_tests.gyp

@@ -111,13 +111,13 @@
             'keyed_service/content/browser_context_dependency_manager_unittest.cc',
             'keyed_service/core/dependency_graph_unittest.cc',
             'language_usage_metrics/language_usage_metrics_unittest.cc',
+            'metrics/compression_utils_unittest.cc',
             'metrics/machine_id_provider_win_unittest.cc',
             'metrics/metrics_hashes_unittest.cc',
             'metrics/metrics_log_manager_unittest.cc',
             'metrics/metrics_log_unittest.cc',
             'metrics/metrics_reporting_scheduler_unittest.cc',
             'metrics/metrics_state_manager_unittest.cc',
-            'metrics/net/compression_utils_unittest.cc',
             'metrics/persisted_logs_unittest.cc',
             'navigation_interception/intercept_navigation_resource_throttle_unittest.cc',
             'network_time/network_time_tracker_unittest.cc',

components/metrics.gypi

@@ -12,11 +12,13 @@
       ],
       'dependencies': [
         '../base/base.gyp:base',
+        '../third_party/zlib/zlib.gyp:zlib',
         'component_metrics_proto',
         'variations',
       ],
       'sources': [
-        'metrics/metrics_provider.h',
+        'metrics/compression_utils.cc',
+        'metrics/compression_utils.h',
         'metrics/cloned_install_detector.cc',
         'metrics/cloned_install_detector.h',
         'metrics/machine_id_provider.h',
@@ -32,6 +34,7 @@
         'metrics/metrics_log_manager.h',
         'metrics/metrics_pref_names.cc',
         'metrics/metrics_pref_names.h',
+        'metrics/metrics_provider.h',
         'metrics/metrics_reporting_scheduler.cc',
         'metrics/metrics_reporting_scheduler.h',
         'metrics/metrics_service.cc',
@@ -67,12 +70,9 @@
       ],
       'dependencies': [
         '../net/net.gyp:net',
-        '../third_party/zlib/zlib.gyp:zlib',
         'metrics',
       ],
       'sources': [
-        'metrics/net/compression_utils.cc',
-        'metrics/net/compression_utils.h',
         'metrics/net/net_metrics_log_uploader.cc',
         'metrics/net/net_metrics_log_uploader.h',
       ],

components/metrics/DEPS

 include_rules = [ 
   "+components/variations",
   "-net",
+  "+third_party/zlib",
 ]

components/metrics/net/compression_utils.cc → components/metrics/compression_utils.cc

@@ -2,11 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/metrics/net/compression_utils.h"
+#include "components/metrics/compression_utils.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/logging.h"
+#include "base/sys_byteorder.h"
 #include "third_party/zlib/zlib.h"
 
 namespace {
@@ -15,7 +17,7 @@ namespace {
 const size_t kGzipZlibHeaderDifferenceBytes = 16;
 
 // Pass an integer greater than the following get a gzip header instead of a
-// zlib header when calling deflateInit2_.
+// zlib header when calling deflateInit2() and inflateInit2().
 const int kWindowBitsToGetGzipHeader = 16;
 
 // This describes the amount of memory zlib uses to compress data. It can go
@@ -24,51 +26,99 @@ const int kWindowBitsToGetGzipHeader = 16;
 const int kZlibMemoryLevel = 8;
 
 // This code is taken almost verbatim from third_party/zlib/compress.c. The only
-// difference is deflateInit2_ is called which sets the window bits to be > 16.
+// difference is deflateInit2() is called which sets the window bits to be > 16.
 // That causes a gzip header to be emitted rather than a zlib header.
 int GzipCompressHelper(Bytef* dest,
                        uLongf* dest_length,
                        const Bytef* source,
                        uLong source_length) {
-    z_stream stream;
-
-    stream.next_in = bit_cast<Bytef*>(source);
-    stream.avail_in = static_cast<uInt>(source_length);
-    stream.next_out = dest;
-    stream.avail_out = static_cast<uInt>(*dest_length);
-    if (static_cast<uLong>(stream.avail_out) != *dest_length)
-      return Z_BUF_ERROR;
-
-    stream.zalloc = static_cast<alloc_func>(0);
-    stream.zfree = static_cast<free_func>(0);
-    stream.opaque = static_cast<voidpf>(0);
-
-    gz_header gzip_header;
-    memset(&gzip_header, 0, sizeof(gzip_header));
-    int err = deflateInit2_(&stream,
-                            Z_DEFAULT_COMPRESSION,
-                            Z_DEFLATED,
-                            MAX_WBITS + kWindowBitsToGetGzipHeader,
-                            kZlibMemoryLevel,
-                            Z_DEFAULT_STRATEGY,
-                            ZLIB_VERSION,
-                            sizeof(z_stream));
-    if (err != Z_OK)
-      return err;
-
-    err = deflateSetHeader(&stream, &gzip_header);
-    if (err != Z_OK)
-      return err;
-
-    err = deflate(&stream, Z_FINISH);
-    if (err != Z_STREAM_END) {
-      deflateEnd(&stream);
-      return err == Z_OK ? Z_BUF_ERROR : err;
-    }
-    *dest_length = stream.total_out;
-
-    err = deflateEnd(&stream);
+  z_stream stream;
+
+  stream.next_in = bit_cast<Bytef*>(source);
+  stream.avail_in = static_cast<uInt>(source_length);
+  stream.next_out = dest;
+  stream.avail_out = static_cast<uInt>(*dest_length);
+  if (static_cast<uLong>(stream.avail_out) != *dest_length)
+    return Z_BUF_ERROR;
+
+  stream.zalloc = static_cast<alloc_func>(0);
+  stream.zfree = static_cast<free_func>(0);
+  stream.opaque = static_cast<voidpf>(0);
+
+  gz_header gzip_header;
+  memset(&gzip_header, 0, sizeof(gzip_header));
+  int err = deflateInit2(&stream,
+                         Z_DEFAULT_COMPRESSION,
+                         Z_DEFLATED,
+                         MAX_WBITS + kWindowBitsToGetGzipHeader,
+                         kZlibMemoryLevel,
+                         Z_DEFAULT_STRATEGY);
+  if (err != Z_OK)
     return err;
+
+  err = deflateSetHeader(&stream, &gzip_header);
+  if (err != Z_OK)
+    return err;
+
+  err = deflate(&stream, Z_FINISH);
+  if (err != Z_STREAM_END) {
+    deflateEnd(&stream);
+    return err == Z_OK ? Z_BUF_ERROR : err;
+  }
+  *dest_length = stream.total_out;
+
+  err = deflateEnd(&stream);
+  return err;
+}
+
+// This code is taken almost verbatim from third_party/zlib/uncompr.c. The only
+// difference is inflateInit2() is called which sets the window bits to be > 16.
+// That causes a gzip header to be parsed rather than a zlib header.
+int GzipUncompressHelper(Bytef* dest,
+                         uLongf* dest_length,
+                         const Bytef* source,
+                         uLong source_length) {
+  z_stream stream;
+
+  stream.next_in = bit_cast<Bytef*>(source);
+  stream.avail_in = static_cast<uInt>(source_length);
+  if (static_cast<uLong>(stream.avail_in) != source_length)
+    return Z_BUF_ERROR;
+
+  stream.next_out = dest;
+  stream.avail_out = static_cast<uInt>(*dest_length);
+  if (static_cast<uLong>(stream.avail_out) != *dest_length)
+    return Z_BUF_ERROR;
+
+  stream.zalloc = static_cast<alloc_func>(0);
+  stream.zfree = static_cast<free_func>(0);
+
+  int err = inflateInit2(&stream, MAX_WBITS + kWindowBitsToGetGzipHeader);
+  if (err != Z_OK)
+    return err;
+
+  err = inflate(&stream, Z_FINISH);
+  if (err != Z_STREAM_END) {
+    inflateEnd(&stream);
+    if (err == Z_NEED_DICT || (err == Z_BUF_ERROR && stream.avail_in == 0))
+      return Z_DATA_ERROR;
+    return err;
+  }
+  *dest_length = stream.total_out;
+
+  err = inflateEnd(&stream);
+  return err;
+}
+
+// Returns the uncompressed size from GZIP-compressed |compressed_data|.
+uint32 GetUncompressedSize(const std::string& compressed_data) {
+  // The uncompressed size is stored in the last 4 bytes of |input| in LE.
+  uint32 size;
+  if (compressed_data.length() < sizeof(size))
+    return 0;
+  memcpy(&size, &compressed_data[compressed_data.length() - sizeof(size)],
+         sizeof(size));
+  return base::ByteSwapToLE32(size);
 }
 
 }  // namespace
@@ -90,7 +140,17 @@ bool GzipCompress(const std::string& input, std::string* output) {
 
   compressed_data.resize(compressed_size);
   output->assign(compressed_data.begin(), compressed_data.end());
+  DCHECK_EQ(input.size(), GetUncompressedSize(*output));
   return true;
 }
 
+bool GzipUncompress(const std::string& input, std::string* output) {
+  output->resize(GetUncompressedSize(input));
+  uLongf uncompressed_size = static_cast<uLongf>(output->length());
+  return GzipUncompressHelper(bit_cast<Bytef*>(output->data()),
+                              &uncompressed_size,
+                              bit_cast<const Bytef*>(input.data()),
+                              static_cast<uLongf>(input.length())) == Z_OK;
+}
+
 }  // namespace metrics

components/metrics/net/compression_utils.h → components/metrics/compression_utils.h

@@ -2,16 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_METRICS_NET_COMPRESSION_UTILS_H_
-#define COMPONENTS_METRICS_NET_COMPRESSION_UTILS_H_
+#ifndef COMPONENTS_METRICS_COMPRESSION_UTILS_H_
+#define COMPONENTS_METRICS_COMPRESSION_UTILS_H_
 
 #include <string>
 
 namespace metrics {
 
-// Compresses the text in |input| using gzip, storing the result in |output|.
+// Compresses the data in |input| using gzip, storing the result in |output|.
 bool GzipCompress(const std::string& input, std::string* output);
 
+// Uncompresses the data in |input| using gzip, storing the result in |output|.
+bool GzipUncompress(const std::string& input, std::string* output);
+
 }  // namespace metrics
 
-#endif  // COMPONENTS_METRICS_NET_COMPRESSION_UTILS_H_
+#endif  // COMPONENTS_METRICS_COMPRESSION_UTILS_H_

components/metrics/net/compression_utils_unittest.cc → components/metrics/compression_utils_unittest.cc

@@ -2,14 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/metrics/net/compression_utils.h"
+#include "components/metrics/compression_utils.h"
 
 #include <string>
 
-#include "base/base_paths.h"
 #include "base/basictypes.h"
-#include "base/file_util.h"
-#include "base/path_service.h"
+#include "base/logging.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace metrics {
@@ -32,11 +30,6 @@ const uint8 kCompressedData[] =
      0x2f, 0xca, 0x49, 0x01, 0x00, 0x85, 0x11, 0x4a, 0x0d,
      0x0b, 0x00, 0x00, 0x00};
 
-// Re-enable C4309.
-#if defined(OS_WIN)
-#pragma warning( default: 4309 )
-#endif
-
 }  // namespace
 
 TEST(CompressionUtilsTest, GzipCompression) {
@@ -49,4 +42,35 @@ TEST(CompressionUtilsTest, GzipCompression) {
   EXPECT_EQ(golden_compressed_data, compressed_data);
 }
 
+TEST(CompressionUtilsTest, GzipUncompression) {
+  std::string compressed_data(reinterpret_cast<const char*>(kCompressedData),
+                              arraysize(kCompressedData));
+
+  std::string uncompressed_data;
+  EXPECT_TRUE(GzipUncompress(compressed_data, &uncompressed_data));
+
+  std::string golden_data(reinterpret_cast<const char*>(kData),
+                          arraysize(kData));
+  EXPECT_EQ(golden_data, uncompressed_data);
+}
+
+// Checks that compressing/decompressing input > 256 bytes works as expected.
+TEST(CompressionUtilsTest, LargeInput) {
+  const size_t kSize = 32 * 1024;
+
+  // Generate a data string of |kSize| for testing.
+  std::string data;
+  data.resize(kSize);
+  for (size_t i = 0; i < kSize; ++i)
+    data[i] = static_cast<char>(i & 0xFF);
+
+  std::string compressed_data;
+  EXPECT_TRUE(GzipCompress(data, &compressed_data));
+
+  std::string uncompressed_data;
+  EXPECT_TRUE(GzipUncompress(compressed_data, &uncompressed_data));
+
+  EXPECT_EQ(data, uncompressed_data);
+}
+
 }  // namespace metrics

components/metrics/metrics_log_manager.cc

@@ -43,11 +43,13 @@ MetricsLogManager::MetricsLogManager(PrefService* local_state,
     : unsent_logs_loaded_(false),
       initial_log_queue_(local_state,
                          prefs::kMetricsInitialLogs,
+                         prefs::kMetricsInitialLogsOld,
                          kInitialLogsPersistLimit,
                          kStorageByteLimitPerLogType,
                          0),
       ongoing_log_queue_(local_state,
                          prefs::kMetricsOngoingLogs,
+                         prefs::kMetricsOngoingLogsOld,
                          kOngoingLogsPersistLimit,
                          kStorageByteLimitPerLogType,
                          max_ongoing_log_size) {}
@@ -62,10 +64,10 @@ void MetricsLogManager::BeginLoggingWithLog(scoped_ptr<MetricsLog> log) {
 void MetricsLogManager::FinishCurrentLog() {
   DCHECK(current_log_.get());
   current_log_->CloseLog();
-  std::string log_text;
-  current_log_->GetEncodedLog(&log_text);
-  if (!log_text.empty())
-    StoreLog(&log_text, current_log_->log_type());
+  std::string log_data;
+  current_log_->GetEncodedLog(&log_data);
+  if (!log_data.empty())
+    StoreLog(log_data, current_log_->log_type());
   current_log_.reset();
 }
 
@@ -101,20 +103,20 @@ void MetricsLogManager::ResumePausedLog() {
   current_log_.reset(paused_log_.release());
 }
 
-void MetricsLogManager::StoreLog(std::string* log,
+void MetricsLogManager::StoreLog(const std::string& log_data,
                                  MetricsLog::LogType log_type) {
   switch (log_type) {
     case MetricsLog::INITIAL_STABILITY_LOG:
-      initial_log_queue_.StoreLog(log);
+      initial_log_queue_.StoreLog(log_data);
       break;
     case MetricsLog::ONGOING_LOG:
-      ongoing_log_queue_.StoreLog(log);
+      ongoing_log_queue_.StoreLog(log_data);
       break;
   }
 }
 
 void MetricsLogManager::StoreStagedLogAsUnsent(
-    metrics::PersistedLogs::StoreType store_type) {
+    PersistedLogs::StoreType store_type) {
   DCHECK(has_staged_log());
   if (initial_log_queue_.has_staged_log())
     initial_log_queue_.StoreStagedLogAsUnsent(store_type);

components/metrics/metrics_log_manager.h

@@ -91,7 +91,7 @@ class MetricsLogManager {
   // This is intended to be used when logs are being saved while an upload is in
   // progress, in case the upload later succeeds.
   // This can only be called if has_staged_log() is true.
-  void StoreStagedLogAsUnsent(metrics::PersistedLogs::StoreType store_type);
+  void StoreStagedLogAsUnsent(PersistedLogs::StoreType store_type);
 
   // Discards the last log stored with StoreStagedLogAsUnsent with |store_type|
   // set to PROVISIONAL_STORE, as long as it hasn't already been re-staged. If
@@ -105,10 +105,8 @@ class MetricsLogManager {
   void LoadPersistedUnsentLogs();
 
  private:
-  // Saves |log| as the given type.
-  // NOTE: This clears the contents of |log| (to avoid an expensive copy),
-  // so the log should be discarded after this call.
-  void StoreLog(std::string* log, MetricsLog::LogType log_type);
+  // Saves |log_data| as the given type.
+  void StoreLog(const std::string& log_data, MetricsLog::LogType log_type);
 
   // Tracks whether unsent logs (if any) have been loaded from the serializer.
   bool unsent_logs_loaded_;
@@ -120,8 +118,8 @@ class MetricsLogManager {
   scoped_ptr<MetricsLog> paused_log_;
 
   // Logs that have not yet been sent.
-  metrics::PersistedLogs initial_log_queue_;
-  metrics::PersistedLogs ongoing_log_queue_;
+  PersistedLogs initial_log_queue_;
+  PersistedLogs ongoing_log_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsLogManager);
 };

components/metrics/metrics_log_manager_unittest.cc

@@ -24,7 +24,9 @@ class TestLogPrefService : public TestingPrefServiceSimple {
  public:
   TestLogPrefService() {
     registry()->RegisterListPref(prefs::kMetricsInitialLogs);
+    registry()->RegisterListPref(prefs::kMetricsInitialLogsOld);
     registry()->RegisterListPref(prefs::kMetricsOngoingLogs);
+    registry()->RegisterListPref(prefs::kMetricsOngoingLogsOld);
   }
 
   // Returns the number of logs of the given type.
@@ -34,7 +36,7 @@ class TestLogPrefService : public TestingPrefServiceSimple {
       list_length = GetList(prefs::kMetricsInitialLogs)->GetSize();
     else
       list_length = GetList(prefs::kMetricsOngoingLogs)->GetSize();
-    return list_length ? list_length - 2 : 0;
+    return list_length / 2;
   }
 };
 
@@ -160,9 +162,9 @@ TEST(MetricsLogManagerTest, StoreAndLoad) {
     // Simulate a log having already been unsent from a previous session.
     {
       std::string log("proto");
-      metrics::PersistedLogs ongoing_logs(
-          &pref_service, prefs::kMetricsOngoingLogs, 1, 1, 0);
-      ongoing_logs.StoreLog(&log);
+      PersistedLogs ongoing_logs(&pref_service, prefs::kMetricsOngoingLogs,
+                                 prefs::kMetricsOngoingLogsOld, 1, 1, 0);
+      ongoing_logs.StoreLog(log);
       ongoing_logs.SerializeLogs();
     }
     EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::ONGOING_LOG));
@@ -176,7 +178,7 @@ TEST(MetricsLogManagerTest, StoreAndLoad) {
     log_manager.BeginLoggingWithLog(make_scoped_ptr(new MetricsLog(
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(metrics::PersistedLogs::NORMAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.FinishCurrentLog();
 
     // Nothing should be written out until PersistUnsentLogs is called.
@@ -236,7 +238,7 @@ TEST(MetricsLogManagerTest, StoreStagedLogTypes) {
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(metrics::PersistedLogs::NORMAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.PersistUnsentLogs();
 
     EXPECT_EQ(0U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
@@ -252,7 +254,7 @@ TEST(MetricsLogManagerTest, StoreStagedLogTypes) {
         "id", 0, MetricsLog::INITIAL_STABILITY_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(metrics::PersistedLogs::NORMAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.PersistUnsentLogs();
 
     EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
@@ -295,8 +297,7 @@ TEST(MetricsLogManagerTest, ProvisionalStoreStandardFlow) {
     log_manager.BeginLoggingWithLog(make_scoped_ptr(new MetricsLog(
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(
-        metrics::PersistedLogs::PROVISIONAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
     log_manager.FinishCurrentLog();
     log_manager.DiscardLastProvisionalStore();
 
@@ -320,15 +321,14 @@ TEST(MetricsLogManagerTest, ProvisionalStoreNoop) {
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(
-        metrics::PersistedLogs::PROVISIONAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
     log_manager.StageNextLogForUpload();
     log_manager.DiscardStagedLog();
     log_manager.BeginLoggingWithLog(make_scoped_ptr(new MetricsLog(
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(metrics::PersistedLogs::NORMAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.DiscardLastProvisionalStore();
 
     log_manager.PersistUnsentLogs();
@@ -345,13 +345,12 @@ TEST(MetricsLogManagerTest, ProvisionalStoreNoop) {
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(metrics::PersistedLogs::NORMAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.BeginLoggingWithLog(make_scoped_ptr(new MetricsLog(
         "id", 0, MetricsLog::ONGOING_LOG,  &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(
-        metrics::PersistedLogs::PROVISIONAL_STORE);
+    log_manager.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
     log_manager.DiscardLastProvisionalStore();
     log_manager.DiscardLastProvisionalStore();
 

components/metrics/metrics_log_uploader.h

@@ -25,9 +25,10 @@ class MetricsLogUploader {
                      const base::Callback<void(int)>& on_upload_complete);
   virtual ~MetricsLogUploader();
 
-  // Uploads a log with the specified |log_data| and |log_hash|. |log_hash| is
-  // expected to be the hex-encoded SHA1 hash of |log_data|.
-  virtual bool UploadLog(const std::string& log_data,
+  // Uploads a log with the specified |compressed_log_data| and |log_hash|.
+  // |log_hash| is expected to be the hex-encoded SHA1 hash of the log data
+  // before compression.
+  virtual bool UploadLog(const std::string& compressed_log_data,
                          const std::string& log_hash) = 0;
 
  protected:

components/metrics/metrics_pref_names.cc

@@ -16,6 +16,8 @@ const char kMetricsClientID[] = "user_experience_metrics.client_id2";
 // first minute of a browser session. These logs include things like crash count
 // info, etc.
 const char kMetricsInitialLogs[] =
+    "user_experience_metrics.initial_logs_list";
+const char kMetricsInitialLogsOld[] =
     "user_experience_metrics.initial_logs_as_protobufs";
 
 // The metrics entropy source.
@@ -41,6 +43,8 @@ const char kMetricsOldLowEntropySource[] =
 // logs typically include histograms and memory reports, as well as ongoing
 // user activities.
 const char kMetricsOngoingLogs[] =
+    "user_experience_metrics.ongoing_logs_list";
+const char kMetricsOngoingLogsOld[] =
     "user_experience_metrics.ongoing_logs_as_protobufs";
 
 // Boolean that indicates a cloned install has been detected and the metrics

components/metrics/metrics_pref_names.h

@@ -12,11 +12,13 @@ namespace prefs {
 // component. Document each in the .cc file.
 extern const char kMetricsClientID[];
 extern const char kMetricsInitialLogs[];
+extern const char kMetricsInitialLogsOld[];
 extern const char kMetricsLowEntropySource[];
 extern const char kMetricsMachineId[];
 extern const char kMetricsOldClientID[];
 extern const char kMetricsOldLowEntropySource[];
 extern const char kMetricsOngoingLogs[];
+extern const char kMetricsOngoingLogsOld[];
 extern const char kMetricsResetIds[];
 extern const char kMetricsReportingEnabledTimestamp[];
 extern const char kMetricsSessionID[];

components/metrics/metrics_service.cc

@@ -300,6 +300,8 @@ void MetricsService::RegisterPrefs(PrefRegistrySimple* registry) {
 
   registry->RegisterListPref(metrics::prefs::kMetricsInitialLogs);
   registry->RegisterListPref(metrics::prefs::kMetricsOngoingLogs);
+  registry->RegisterListPref(metrics::prefs::kMetricsInitialLogsOld);
+  registry->RegisterListPref(metrics::prefs::kMetricsOngoingLogsOld);
 
   registry->RegisterInt64Pref(metrics::prefs::kUninstallLaunchCount, 0);
   registry->RegisterInt64Pref(metrics::prefs::kUninstallMetricsUptimeSec, 0);

components/metrics/net/DEPS

 include_rules = [ 
   "+net",
-  "+third_party/zlib",
 ]

components/metrics/net/net_metrics_log_uploader.cc

@@ -5,7 +5,6 @@
 #include "components/metrics/net/net_metrics_log_uploader.h"
 
 #include "base/metrics/histogram.h"
-#include "components/metrics/net/compression_utils.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
 #include "url/gurl.h"
@@ -24,22 +23,8 @@ NetMetricsLogUploader::NetMetricsLogUploader(
 NetMetricsLogUploader::~NetMetricsLogUploader() {
 }
 
-bool NetMetricsLogUploader::UploadLog(const std::string& log_data,
+bool NetMetricsLogUploader::UploadLog(const std::string& compressed_log_data,
                                       const std::string& log_hash) {
-  std::string compressed_log_data;
-  if (!GzipCompress(log_data, &compressed_log_data)) {
-    NOTREACHED();
-    return false;
-  }
-
-  UMA_HISTOGRAM_PERCENTAGE(
-      "UMA.ProtoCompressionRatio",
-      static_cast<int>(100 * compressed_log_data.size() / log_data.size()));
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "UMA.ProtoGzippedKBSaved",
-      static_cast<int>((log_data.size() - compressed_log_data.size()) / 1024),
-      1, 2000, 50);
-
   current_fetch_.reset(
       net::URLFetcher::Create(GURL(server_url_), net::URLFetcher::POST, this));
   current_fetch_->SetRequestContext(request_context_getter_);

components/metrics/net/net_metrics_log_uploader.h

@@ -33,7 +33,7 @@ class NetMetricsLogUploader : public MetricsLogUploader,
   virtual ~NetMetricsLogUploader();
 
   // MetricsLogUploader:
-  virtual bool UploadLog(const std::string& log_data,
+  virtual bool UploadLog(const std::string& compressed_log_data,
                          const std::string& log_hash) OVERRIDE;
 
  private:

components/metrics/persisted_logs.cc

@@ -13,15 +13,12 @@
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/sha1.h"
 #include "base/timer/elapsed_timer.h"
+#include "components/metrics/compression_utils.h"
 
 namespace metrics {
 
 namespace {
 
-// We append (2) more elements to persisted lists: the size of the list and a
-// checksum of the elements.
-const size_t kChecksumEntryCount = 2;
-
 PersistedLogs::LogReadStatus MakeRecallStatusHistogram(
     PersistedLogs::LogReadStatus status) {
   UMA_HISTOGRAM_ENUMERATION("PrefService.PersistentLogRecallProtobufs",
@@ -29,28 +26,64 @@ PersistedLogs::LogReadStatus MakeRecallStatusHistogram(
   return status;
 }
 
+// Reads the value at |index| from |list_value| as a string and Base64-decodes
+// it into |result|. Returns true on success.
+bool ReadBase64String(const base::ListValue& list_value,
+                      size_t index,
+                      std::string* result) {
+  std::string base64_result;
+  if (!list_value.GetString(index, &base64_result))
+    return false;
+  return base::Base64Decode(base64_result, result);
+}
+
+// Base64-encodes |str| and appends the result to |list_value|.
+void AppendBase64String(const std::string& str, base::ListValue* list_value) {
+  std::string base64_str;
+  base::Base64Encode(str, &base64_str);
+  list_value->Append(base::Value::CreateStringValue(base64_str));
+}
+
 }  // namespace
 
-void PersistedLogs::LogHashPair::SwapLog(std::string* input) {
-  log.swap(*input);
-  if (!log.empty())
-    hash = base::SHA1HashString(log);
-  else
-    hash.clear();
+void PersistedLogs::LogHashPair::Init(const std::string& log_data) {
+  DCHECK(!log_data.empty());
+
+  if (!GzipCompress(log_data, &compressed_log_data)) {
+    NOTREACHED();
+    return;
+  }
+
+  UMA_HISTOGRAM_PERCENTAGE(
+      "UMA.ProtoCompressionRatio",
+      static_cast<int>(100 * compressed_log_data.size() / log_data.size()));
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      "UMA.ProtoGzippedKBSaved",
+      static_cast<int>((log_data.size() - compressed_log_data.size()) / 1024),
+      1, 2000, 50);
+
+  hash = base::SHA1HashString(log_data);
+}
+
+void PersistedLogs::LogHashPair::Clear() {
+  compressed_log_data.clear();
+  hash.clear();
 }
 
 void PersistedLogs::LogHashPair::Swap(PersistedLogs::LogHashPair* input) {
-  log.swap(input->log);
+  compressed_log_data.swap(input->compressed_log_data);
   hash.swap(input->hash);
 }
 
 PersistedLogs::PersistedLogs(PrefService* local_state,
                              const char* pref_name,
+                             const char* old_pref_name,
                              size_t min_log_count,
                              size_t min_log_bytes,
                              size_t max_log_size)
     : local_state_(local_state),
       pref_name_(pref_name),
+      old_pref_name_(old_pref_name),
       min_log_count_(min_log_count),
       min_log_bytes_(min_log_bytes),
       max_log_size_(max_log_size),
@@ -67,7 +100,7 @@ void PersistedLogs::SerializeLogs() {
   if (max_log_size_) {
     for (std::vector<LogHashPair>::iterator it = list_.begin();
          it != list_.end();) {
-      size_t log_size = it->log.length();
+      size_t log_size = it->compressed_log_data.length();
       if (log_size > max_log_size_) {
         UMA_HISTOGRAM_COUNTS("UMA.Large Accumulated Log Not Persisted",
                              static_cast<int>(log_size));
@@ -77,18 +110,29 @@ void PersistedLogs::SerializeLogs() {
       }
     }
   }
+
   ListPrefUpdate update(local_state_, pref_name_);
   WriteLogsToPrefList(update.Get());
+
+  // Clear the old pref now that we've written to the new one.
+  // TODO(asvitkine): Remove the old pref in M39.
+  local_state_->ClearPref(old_pref_name_);
 }
 
 PersistedLogs::LogReadStatus PersistedLogs::DeserializeLogs() {
-  const base::ListValue* unsent_logs = local_state_->GetList(pref_name_);
+  // First, try reading from old pref. If it's empty, read from the new one.
+  // TODO(asvitkine): Remove the old pref in M39.
+  const base::ListValue* unsent_logs = local_state_->GetList(old_pref_name_);
+  if (!unsent_logs->empty())
+    return ReadLogsFromOldPrefList(*unsent_logs);
+
+  unsent_logs = local_state_->GetList(pref_name_);
   return ReadLogsFromPrefList(*unsent_logs);
 }
 
-void PersistedLogs::StoreLog(std::string* input) {
+void PersistedLogs::StoreLog(const std::string& log_data) {
   list_.push_back(LogHashPair());
-  list_.back().SwapLog(input);
+  list_.back().Init(log_data);
 }
 
 void PersistedLogs::StageLog() {
@@ -107,7 +151,7 @@ void PersistedLogs::StageLog() {
 
 void PersistedLogs::DiscardStagedLog() {
   DCHECK(has_staged_log());
-  staged_log_.log.clear();
+  staged_log_.Clear();
 }
 
 void PersistedLogs::StoreStagedLogAsUnsent(StoreType store_type) {
@@ -127,7 +171,6 @@ void PersistedLogs::DiscardLastProvisionalStore() {
 
 void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) {
   list_value->Clear();
-
   // Leave the list completely empty if there are no storable values.
   if (list_.empty())
     return;
@@ -141,7 +184,7 @@ void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) {
     std::vector<LogHashPair>::const_reverse_iterator end = list_.rend();
     for (std::vector<LogHashPair>::const_reverse_iterator it = list_.rbegin();
          it != end; ++it) {
-      size_t log_size = it->log.length();
+      const size_t log_size = it->compressed_log_data.length();
       if (bytes_used >= min_log_bytes_ &&
           (list_.size() - start) >= min_log_count_) {
         break;
@@ -151,35 +194,44 @@ void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) {
     }
   }
   DCHECK_LT(start, list_.size());
-  if (start >= list_.size())
-    return;
 
-  // Store size at the beginning of the list_value.
-  list_value->Append(base::Value::CreateIntegerValue(list_.size() - start));
+  for (size_t i = start; i < list_.size(); ++i) {
+    AppendBase64String(list_[i].compressed_log_data, list_value);
+    AppendBase64String(list_[i].hash, list_value);
+  }
+}
+
+PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
+    const base::ListValue& list_value) {
+  if (list_value.empty())
+    return MakeRecallStatusHistogram(LIST_EMPTY);
 
-  base::MD5Context ctx;
-  base::MD5Init(&ctx);
-  std::string encoded_log;
-  for (std::vector<LogHashPair>::const_iterator it = list_.begin() + start;
-       it != list_.end(); ++it) {
-    // We encode the compressed log as Value::CreateStringValue() expects to
-    // take a valid UTF8 string.
-    base::Base64Encode(it->log, &encoded_log);
-    base::MD5Update(&ctx, encoded_log);
-    list_value->Append(base::Value::CreateStringValue(encoded_log));
+  // For each log, there's two entries in the list (the data and the hash).
+  DCHECK_EQ(0U, list_value.GetSize() % 2);
+  const size_t log_count = list_value.GetSize() / 2;
+
+  // Resize |list_| ahead of time, so that values can be decoded directly into
+  // the elements of the list.
+  DCHECK(list_.empty());
+  list_.resize(log_count);
+
+  for (size_t i = 0; i < log_count; ++i) {
+    if (!ReadBase64String(list_value, i * 2, &list_[i].compressed_log_data) ||
+        !ReadBase64String(list_value, i * 2 + 1, &list_[i].hash)) {
+      list_.clear();
+      return MakeRecallStatusHistogram(LOG_STRING_CORRUPTION);
+    }
   }
 
-  // Append hash to the end of the list_value.
-  base::MD5Digest digest;
-  base::MD5Final(&digest, &ctx);
-  list_value->Append(base::Value::CreateStringValue(
-      base::MD5DigestToBase16(digest)));
-  // Minimum of 3 elements (size, data, hash).
-  DCHECK_GE(list_value->GetSize(), 3U);
+  return MakeRecallStatusHistogram(RECALL_SUCCESS);
 }
 
-PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
+PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromOldPrefList(
     const base::ListValue& list_value) {
+  // We append (2) more elements to persisted lists: the size of the list and a
+  // checksum of the elements.
+  const size_t kChecksumEntryCount = 2;
+
   if (list_value.GetSize() == 0)
     return MakeRecallStatusHistogram(LIST_EMPTY);
   if (list_value.GetSize() <= kChecksumEntryCount)
@@ -223,7 +275,7 @@ PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
     }
 
     DCHECK_LT(local_index, list_.size());
-    list_[local_index].SwapLog(&log_text);
+    list_[local_index].Init(log_text);
   }
 
   // Verify checksum.

components/metrics/persisted_logs.h

@@ -42,15 +42,19 @@ class PersistedLogs {
     PROVISIONAL_STORE,  // A store operation that can be easily reverted later.
   };
 
-  // Constructs a PersistedLogs that stores data in |local_state| under
-  // the preference |pref_name|.  Calling code is responsible for ensuring that
-  // the lifetime of |local_state| is longer than the lifetime of PersistedLogs.
-  // When saving logs to disk, we will store either the first |min_log_count|
-  // logs, or at least |min_log_bytes| bytes of logs, whichever is more.
-  // If the optional max_log_size parameter is non-zero, all logs larger than
+  // Constructs a PersistedLogs that stores data in |local_state| under the
+  // preference |pref_name| and also reads from legacy pref |old_pref_name|.
+  // Calling code is responsible for ensuring that the lifetime of |local_state|
+  // is longer than the lifetime of PersistedLogs.
+  //
+  // When saving logs to disk, stores either the first |min_log_count| logs, or
+  // at least |min_log_bytes| bytes of logs, whichever is greater.
+  //
+  // If the optional |max_log_size| parameter is non-zero, all logs larger than
   // that limit will be dropped before logs are written to disk.
   PersistedLogs(PrefService* local_state,
                 const char* pref_name,
+                const char* old_pref_name,
                 size_t min_log_count,
                 size_t min_log_bytes,
                 size_t max_log_size);
@@ -62,8 +66,8 @@ class PersistedLogs {
   // Reads the list from the preference.
   LogReadStatus DeserializeLogs();
 
-  // Adds a log to the list.  |input| will be swapped with an empty string.
-  void StoreLog(std::string* input);
+  // Adds a log to the list.
+  void StoreLog(const std::string& log_data);
 
   // Stages the most recent log.  The staged_log will remain the same even if
   // additional logs are added.
@@ -87,12 +91,14 @@ class PersistedLogs {
   void DiscardLastProvisionalStore();
 
   // True if a log has been staged.
-  bool has_staged_log() const { return !staged_log_.log.empty(); };
+  bool has_staged_log() const {
+    return !staged_log_.compressed_log_data.empty();
+  }
 
   // Returns the element in the front of the list.
   const std::string& staged_log() const {
     DCHECK(has_staged_log());
-    return staged_log_.log;
+    return staged_log_.compressed_log_data;
   }
 
   // Returns the element in the front of the list.
@@ -114,14 +120,22 @@ class PersistedLogs {
   // Reads the list from the ListValue.
   LogReadStatus ReadLogsFromPrefList(const base::ListValue& list);
 
+  // Reads the list from the old pref's ListValue.
+  // TODO(asvitkine): Remove the old pref in M39.
+  LogReadStatus ReadLogsFromOldPrefList(const base::ListValue& list);
+
   // A weak pointer to the PrefService object to read and write the preference
   // from.  Calling code should ensure this object continues to exist for the
   // lifetime of the PersistedLogs object.
   PrefService* local_state_;
 
-  // The name of the preference this object stores logs in.
+  // The name of the preference to serialize logs to/from.
   const char* pref_name_;
 
+  // The name of the preference to serialize logs from.
+  // TODO(asvitkine): Remove the old pref in M39.
+  const char* old_pref_name_;
+
   // We will keep at least this |min_log_count_| logs or |min_log_bytes_| bytes
   // of logs, whichever is greater, when writing to disk.  These apply after
   // skipping logs greater than |max_log_size_|.
@@ -132,14 +146,20 @@ class PersistedLogs {
   const size_t max_log_size_;
 
   struct LogHashPair {
-    // Raw log text, typically a serialized protobuf.
-    std::string log;
-    // The SHA1 hash of log, stored to catch errors from memory corruption.
-    std::string hash;
-    // Swap the content of input into log and update the hash.
-    void SwapLog(std::string* input);
+    // Initializes the members based on uncompressed |log_data|.
+    void Init(const std::string& log_data);
+
+    // Clears the struct members.
+    void Clear();
+
     // Swap both log and hash from another LogHashPair.
     void Swap(LogHashPair* input);
+
+    // Compressed log data - a serialized protobuf that's been gzipped.
+    std::string compressed_log_data;
+
+    // The SHA1 hash of log, stored to catch errors from memory corruption.
+    std::string hash;
   };
   // A list of all of the stored logs, stored with SHA1 hashes to check for
   // corruption while they are stored in memory.