diff --git a/components/metrics/metrics_log_manager.cc b/components/metrics/metrics_log_manager.cc index 6b0525d1d5cd..d2777347b7f3 100644 --- a/components/metrics/metrics_log_manager.cc +++ b/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_) diff --git a/components/metrics/metrics_log_manager.h b/components/metrics/metrics_log_manager.h index 0bdc886c3795..7fb28b38e1b4 100644 --- a/components/metrics/metrics_log_manager.h +++ b/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(); diff --git a/components/metrics/metrics_log_manager_unittest.cc b/components/metrics/metrics_log_manager_unittest.cc index b43a514de70d..b74d614bdf3a 100644 --- a/components/metrics/metrics_log_manager_unittest.cc +++ b/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)); } } diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc index 925421fea799..aa76bf45a9d9 100644 --- a/components/metrics/metrics_service.cc +++ b/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). diff --git a/components/metrics/persisted_logs.cc b/components/metrics/persisted_logs.cc index f3cc5fdf679c..fc669fbba14e 100644 --- a/components/metrics/persisted_logs.cc +++ b/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(-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(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(last_provisional_store_index_), list_.size()); - list_.erase(list_.begin() + last_provisional_store_index_); - last_provisional_store_index_ = -1; + DCHECK_LT(static_cast(staged_log_index_), list_.size()); + list_.erase(list_.begin() + staged_log_index_); + staged_log_index_ = -1; } void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const { diff --git a/components/metrics/persisted_logs.h b/components/metrics/persisted_logs.h index 5de19265395e..7aadfa7f26b2 100644 --- a/components/metrics/persisted_logs.h +++ b/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 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); }; diff --git a/components/metrics/persisted_logs_unittest.cc b/components/metrics/persisted_logs_unittest.cc index 27c230ce010e..3e9c7fcd65fc 100644 --- a/components/metrics/persisted_logs_unittest.cc +++ b/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);