Remove the provisional store and just keep the staged log in the queue while uploading it.

BUG=372911

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

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

components/metrics/metrics_log_manager.cc

@@ -115,22 +115,6 @@ void MetricsLogManager::StoreLog(const std::string& log_data,
   }
 }
 
-void MetricsLogManager::StoreStagedLogAsUnsent(
-    PersistedLogs::StoreType store_type) {
-  DCHECK(has_staged_log());
-  if (initial_log_queue_.has_staged_log())
-    initial_log_queue_.StoreStagedLogAsUnsent(store_type);
-  else
-    ongoing_log_queue_.StoreStagedLogAsUnsent(store_type);
-}
-
-void MetricsLogManager::DiscardLastProvisionalStore() {
-  // We have at most one provisional store, (since at most one log is being
-  // uploaded at a time), so at least one of these will be a no-op.
-  initial_log_queue_.DiscardLastProvisionalStore();
-  ongoing_log_queue_.DiscardLastProvisionalStore();
-}
-
 void MetricsLogManager::PersistUnsentLogs() {
   DCHECK(unsent_logs_loaded_);
   if (!unsent_logs_loaded_)

components/metrics/metrics_log_manager.h

@@ -84,20 +84,6 @@ class MetricsLogManager {
   // This should only be called if there is not a current log.
   void ResumePausedLog();
 
-  // Saves the staged log, then clears staged_log().
-  // If |store_type| is PROVISIONAL_STORE, it can be dropped from storage with
-  // a later call to DiscardLastProvisionalStore (if it hasn't already been
-  // staged again).
-  // 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(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
-  // the log is no longer present, this is a no-op.
-  void DiscardLastProvisionalStore();
-
   // Saves any unsent logs to persistent storage.
   void PersistUnsentLogs();
 

components/metrics/metrics_log_manager_unittest.cc

@@ -178,7 +178,6 @@ 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(PersistedLogs::NORMAL_STORE);
     log_manager.FinishCurrentLog();
 
     // Nothing should be written out until PersistUnsentLogs is called.
@@ -238,7 +237,6 @@ TEST(MetricsLogManagerTest, StoreStagedLogTypes) {
         "id", 0, MetricsLog::ONGOING_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.PersistUnsentLogs();
 
     EXPECT_EQ(0U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
@@ -254,7 +252,6 @@ TEST(MetricsLogManagerTest, StoreStagedLogTypes) {
         "id", 0, MetricsLog::INITIAL_STABILITY_LOG, &client, &pref_service)));
     log_manager.FinishCurrentLog();
     log_manager.StageNextLogForUpload();
-    log_manager.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
     log_manager.PersistUnsentLogs();
 
     EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
@@ -282,10 +279,10 @@ TEST(MetricsLogManagerTest, LargeLogDiscarding) {
   EXPECT_EQ(0U, pref_service.TypeCount(MetricsLog::ONGOING_LOG));
 }
 
-TEST(MetricsLogManagerTest, ProvisionalStoreStandardFlow) {
+TEST(MetricsLogManagerTest, DiscardOrder) {
+  // Ensure that the correct log is discarded if new logs are pushed while
+  // a log is staged.
   TestMetricsServiceClient client;
-
-  // Ensure that provisional store works, and discards the correct log.
   {
     TestLogPrefService pref_service;
     MetricsLogManager log_manager(&pref_service, 0);
@@ -297,64 +294,11 @@ 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(PersistedLogs::PROVISIONAL_STORE);
     log_manager.FinishCurrentLog();
-    log_manager.DiscardLastProvisionalStore();
-
-    log_manager.PersistUnsentLogs();
-    EXPECT_EQ(0U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
-    EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::ONGOING_LOG));
-  }
-}
-
-TEST(MetricsLogManagerTest, ProvisionalStoreNoop) {
-  TestMetricsServiceClient client;
-
-  // Ensure that trying to drop a sent log is a no-op, even if another log has
-  // since been staged.
-  {
-    TestLogPrefService pref_service;
-    MetricsLogManager log_manager(&pref_service, 0);
-    log_manager.LoadPersistedUnsentLogs();
-
-    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(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(PersistedLogs::NORMAL_STORE);
-    log_manager.DiscardLastProvisionalStore();
-
-    log_manager.PersistUnsentLogs();
-    EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::ONGOING_LOG));
-  }
-
-  // Ensure that trying to drop more than once is a no-op
-  {
-    TestLogPrefService pref_service;
-    MetricsLogManager log_manager(&pref_service, 0);
-    log_manager.LoadPersistedUnsentLogs();
-
-    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(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(PersistedLogs::PROVISIONAL_STORE);
-    log_manager.DiscardLastProvisionalStore();
-    log_manager.DiscardLastProvisionalStore();
 
     log_manager.PersistUnsentLogs();
+    EXPECT_EQ(0U, pref_service.TypeCount(MetricsLog::INITIAL_STABILITY_LOG));
     EXPECT_EQ(1U, pref_service.TypeCount(MetricsLog::ONGOING_LOG));
   }
 }

components/metrics/metrics_service.cc

@@ -773,16 +773,6 @@ void MetricsService::PushPendingLogsToPersistentStorage() {
   if (state_ < SENDING_INITIAL_STABILITY_LOG)
     return;  // We didn't and still don't have time to get plugin list etc.
 
-  if (log_manager_.has_staged_log()) {
-    // We may race here, and send second copy of the log later.
-    metrics::PersistedLogs::StoreType store_type;
-    if (log_upload_in_progress_)
-      store_type = metrics::PersistedLogs::PROVISIONAL_STORE;
-    else
-      store_type = metrics::PersistedLogs::NORMAL_STORE;
-    log_manager_.StoreStagedLogAsUnsent(store_type);
-  }
-  DCHECK(!log_manager_.has_staged_log());
   CloseCurrentLog();
   log_manager_.PersistUnsentLogs();
 
@@ -1035,10 +1025,6 @@ void MetricsService::OnLogUploadComplete(int response_code) {
                             ResponseCodeToStatus(response_code),
                             NUM_RESPONSE_STATUSES);
 
-  // If the upload was provisionally stored, drop it now that the upload is
-  // known to have gone through.
-  log_manager_.DiscardLastProvisionalStore();
-
   bool upload_succeeded = response_code == 200;
 
   // Provide boolean for error recovery (allow us to ignore response_code).

components/metrics/persisted_logs.cc

@@ -65,16 +65,6 @@ void PersistedLogs::LogHashPair::Init(const std::string& log_data) {
   hash = base::SHA1HashString(log_data);
 }
 
-void PersistedLogs::LogHashPair::Clear() {
-  compressed_log_data.clear();
-  hash.clear();
-}
-
-void PersistedLogs::LogHashPair::Swap(PersistedLogs::LogHashPair* input) {
-  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,
@@ -87,7 +77,7 @@ PersistedLogs::PersistedLogs(PrefService* local_state,
       min_log_count_(min_log_count),
       min_log_bytes_(min_log_bytes),
       max_log_size_(max_log_size != 0 ? max_log_size : static_cast<size_t>(-1)),
-      last_provisional_store_index_(-1) {
+      staged_log_index_(-1) {
   DCHECK(local_state_);
   // One of the limit arguments must be non-zero.
   DCHECK(min_log_count_ > 0 || min_log_bytes_ > 0);
@@ -125,33 +115,15 @@ void PersistedLogs::StageLog() {
   // hard-to-identify crashes much later.
   CHECK(!list_.empty());
   DCHECK(!has_staged_log());
-  staged_log_.Swap(&list_.back());
-  list_.pop_back();
-
-  // If the staged log was the last provisional store, clear that.
-  if (static_cast<size_t>(last_provisional_store_index_) == list_.size())
-    last_provisional_store_index_ = -1;
+  staged_log_index_ = list_.size() - 1;
   DCHECK(has_staged_log());
 }
 
 void PersistedLogs::DiscardStagedLog() {
   DCHECK(has_staged_log());
-  staged_log_.Clear();
-}
-
-void PersistedLogs::StoreStagedLogAsUnsent(StoreType store_type) {
-  list_.push_back(LogHashPair());
-  list_.back().Swap(&staged_log_);
-  if (store_type == PROVISIONAL_STORE)
-    last_provisional_store_index_ = list_.size() - 1;
-}
-
-void PersistedLogs::DiscardLastProvisionalStore() {
-  if (last_provisional_store_index_ == -1)
-    return;
-  DCHECK_LT(static_cast<size_t>(last_provisional_store_index_), list_.size());
-  list_.erase(list_.begin() + last_provisional_store_index_);
-  last_provisional_store_index_ = -1;
+  DCHECK_LT(static_cast<size_t>(staged_log_index_), list_.size());
+  list_.erase(list_.begin() + staged_log_index_);
+  staged_log_index_ = -1;
 }
 
 void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const {

components/metrics/persisted_logs.h

@@ -37,11 +37,6 @@ class PersistedLogs {
     END_RECALL_STATUS       // Number of bins to use to create the histogram.
   };
 
-  enum StoreType {
-    NORMAL_STORE,       // A standard store operation.
-    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| and also reads from legacy pref |old_pref_name|.
   // Calling code is responsible for ensuring that the lifetime of |local_state|
@@ -76,35 +71,19 @@ class PersistedLogs {
   // Remove the staged log.
   void DiscardStagedLog();
 
-  // Saves the staged log, then clears staged_log().
-  // If |store_type| is PROVISIONAL_STORE, it can be dropped from storage with
-  // a later call to DiscardLastProvisionalStore (if it hasn't already been
-  // staged again).
-  // 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(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
-  // the log is no longer present, this is a no-op.
-  void DiscardLastProvisionalStore();
-
   // True if a log has been staged.
-  bool has_staged_log() const {
-    return !staged_log_.compressed_log_data.empty();
-  }
+  bool has_staged_log() const { return staged_log_index_ != -1; };
 
   // Returns the element in the front of the list.
   const std::string& staged_log() const {
     DCHECK(has_staged_log());
-    return staged_log_.compressed_log_data;
+    return list_[staged_log_index_].compressed_log_data;
   }
 
   // Returns the element in the front of the list.
   const std::string& staged_log_hash() const {
     DCHECK(has_staged_log());
-    return staged_log_.hash;
+    return list_[staged_log_index_].hash;
   }
 
   // The number of elements currently stored.
@@ -149,12 +128,6 @@ class PersistedLogs {
     // 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;
 
@@ -165,16 +138,9 @@ class PersistedLogs {
   // corruption while they are stored in memory.
   std::vector<LogHashPair> list_;
 
-  // The log staged for upload.
-  LogHashPair staged_log_;
-
-  // The index and type of the last provisional store. If nothing has been
-  // provisionally stored, or the last provisional store has already been
-  // re-staged, the index will be -1;
-  // This is necessary because during an upload there are two logs (staged
-  // and current) and a client might store them in either order, so it's
-  // not necessarily the case that the provisional store is the last store.
-  int last_provisional_store_index_;
+  // The index and type of the log staged for upload. If nothing has been
+  // staged, the index will be -1.
+  int staged_log_index_;
 
   DISALLOW_COPY_AND_ASSIGN(PersistedLogs);
 };

components/metrics/persisted_logs_unittest.cc

@@ -240,7 +240,7 @@ TEST_F(PersistedLogsTest, Staging) {
   EXPECT_EQ(persisted_logs.staged_log(), Compress("two"));
   persisted_logs.StoreLog("three");
   EXPECT_EQ(persisted_logs.staged_log(), Compress("two"));
-  EXPECT_EQ(persisted_logs.size(), 2U);
+  EXPECT_EQ(persisted_logs.size(), 3U);
   persisted_logs.DiscardStagedLog();
   EXPECT_FALSE(persisted_logs.has_staged_log());
   EXPECT_EQ(persisted_logs.size(), 2U);
@@ -254,38 +254,15 @@ TEST_F(PersistedLogsTest, Staging) {
   EXPECT_EQ(persisted_logs.size(), 0U);
 }
 
-TEST_F(PersistedLogsTest, ProvisionalStoreStandardFlow) {
-  // Ensure that provisional store works, and discards the correct log.
+TEST_F(PersistedLogsTest, DiscardOrder) {
+  // Ensure that the correct log is discarded if new logs are pushed while
+  // a log is staged.
   TestPersistedLogs persisted_logs(&prefs_, kLogByteLimit);
 
   persisted_logs.StoreLog("one");
   persisted_logs.StageLog();
-  persisted_logs.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
   persisted_logs.StoreLog("two");
-  persisted_logs.DiscardLastProvisionalStore();
-  persisted_logs.SerializeLogs();
-
-  TestPersistedLogs result_persisted_logs(&prefs_, kLogByteLimit);
-  EXPECT_EQ(PersistedLogs::RECALL_SUCCESS,
-            result_persisted_logs.DeserializeLogs());
-  EXPECT_EQ(1U, result_persisted_logs.size());
-  result_persisted_logs.ExpectNextLog("two");
-}
-
-TEST_F(PersistedLogsTest, ProvisionalStoreNoop1) {
-  // Ensure that trying to drop a sent log is a no-op, even if another log has
-  // since been staged.
-  TestPersistedLogs persisted_logs(&prefs_, kLogByteLimit);
-  persisted_logs.DeserializeLogs();
-  persisted_logs.StoreLog("one");
-  persisted_logs.StageLog();
-  persisted_logs.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
-  persisted_logs.StageLog();
   persisted_logs.DiscardStagedLog();
-  persisted_logs.StoreLog("two");
-  persisted_logs.StageLog();
-  persisted_logs.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
-  persisted_logs.DiscardLastProvisionalStore();
   persisted_logs.SerializeLogs();
 
   TestPersistedLogs result_persisted_logs(&prefs_, kLogByteLimit);
@@ -295,28 +272,8 @@ TEST_F(PersistedLogsTest, ProvisionalStoreNoop1) {
   result_persisted_logs.ExpectNextLog("two");
 }
 
-TEST_F(PersistedLogsTest, ProvisionalStoreNoop2) {
-  // Ensure that trying to drop more than once is a no-op
-  TestPersistedLogs persisted_logs(&prefs_, kLogByteLimit);
-  persisted_logs.DeserializeLogs();
-  persisted_logs.StoreLog("one");
-  persisted_logs.StageLog();
-  persisted_logs.StoreStagedLogAsUnsent(PersistedLogs::NORMAL_STORE);
-  persisted_logs.StoreLog("two");
-  persisted_logs.StageLog();
-  persisted_logs.StoreStagedLogAsUnsent(PersistedLogs::PROVISIONAL_STORE);
-  persisted_logs.DiscardLastProvisionalStore();
-  persisted_logs.DiscardLastProvisionalStore();
-  persisted_logs.SerializeLogs();
-
-  TestPersistedLogs result_persisted_logs(&prefs_, kLogByteLimit);
-  EXPECT_EQ(PersistedLogs::RECALL_SUCCESS,
-            result_persisted_logs.DeserializeLogs());
-  EXPECT_EQ(1U, result_persisted_logs.size());
-  result_persisted_logs.ExpectNextLog("one");
-}
 
-TEST_F(PersistedLogsTest, Encoding) {
+TEST_F(PersistedLogsTest, Hashes) {
   const char kFooText[] = "foo";
   const std::string foo_hash = base::SHA1HashString(kFooText);