Load the and store rollback version into the correct variable.
Somehow missed this and added unittest to help never have this issue again.
In this CL I also got rid of the powerwash_prefs_ logic in GetPersistedValue
because it was just useless noise in my prev CL that isn't used anymore.
BUG=chromium:285381
TEST=unittests that actually catch error.
Change-Id: I4ed680c5b5c2e37549d58aa0a7c0d35a62623fe7
Reviewed-on: https://chromium-review.googlesource.com/168073
Tested-by: Chris Sosa <sosa@chromium.org>
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: Chris Sosa <sosa@chromium.org>
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 8e77afa..3cf78d3 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -50,9 +50,12 @@
// Note: policy decision to not update to a version we rolled back from.
string rollback_version =
system_state_->payload_state()->GetRollbackVersion();
- if (!rollback_version.empty() && rollback_version == response.version) {
- LOG(INFO) << "Received version that we rolled back from. Aborting.";
- return;
+ if(!rollback_version.empty()) {
+ LOG(INFO) << "Detected previous rollback from version " << rollback_version;
+ if(rollback_version == response.version) {
+ LOG(INFO) << "Received version that we rolled back from. Aborting.";
+ return;
+ }
}
// All decisions as to which URL should be used have already been done. So,
diff --git a/payload_state.cc b/payload_state.cc
index 1463bc0..90dc3d3 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -625,18 +625,13 @@
}
}
-int64_t PayloadState::GetPersistedValue(const string& key,
- bool across_powerwash) {
- PrefsInterface* prefs = prefs_;
- if (across_powerwash)
- prefs = powerwash_safe_prefs_;
-
+int64_t PayloadState::GetPersistedValue(const string& key) {
CHECK(prefs_);
- if (!prefs->Exists(key))
+ if (!prefs_->Exists(key))
return 0;
int64_t stored_value;
- if (!prefs->GetInt64(key, &stored_value))
+ if (!prefs_->GetInt64(key, &stored_value))
return 0;
if (stored_value < 0) {
@@ -690,13 +685,12 @@
}
void PayloadState::LoadPayloadAttemptNumber() {
- SetPayloadAttemptNumber(GetPersistedValue(kPrefsPayloadAttemptNumber,
- false));
+ SetPayloadAttemptNumber(GetPersistedValue(kPrefsPayloadAttemptNumber));
}
void PayloadState::LoadFullPayloadAttemptNumber() {
- SetFullPayloadAttemptNumber(GetPersistedValue(kPrefsFullPayloadAttemptNumber,
- false));
+ SetFullPayloadAttemptNumber(GetPersistedValue(
+ kPrefsFullPayloadAttemptNumber));
}
void PayloadState::SetPayloadAttemptNumber(int payload_attempt_number) {
@@ -716,7 +710,7 @@
}
void PayloadState::LoadUrlIndex() {
- SetUrlIndex(GetPersistedValue(kPrefsCurrentUrlIndex, false));
+ SetUrlIndex(GetPersistedValue(kPrefsCurrentUrlIndex));
}
void PayloadState::SetUrlIndex(uint32_t url_index) {
@@ -731,7 +725,7 @@
}
void PayloadState::LoadUrlSwitchCount() {
- SetUrlSwitchCount(GetPersistedValue(kPrefsUrlSwitchCount, false));
+ SetUrlSwitchCount(GetPersistedValue(kPrefsUrlSwitchCount));
}
void PayloadState::SetUrlSwitchCount(uint32_t url_switch_count) {
@@ -742,8 +736,7 @@
}
void PayloadState::LoadUrlFailureCount() {
- SetUrlFailureCount(GetPersistedValue(kPrefsCurrentUrlFailureCount,
- false));
+ SetUrlFailureCount(GetPersistedValue(kPrefsCurrentUrlFailureCount));
}
void PayloadState::SetUrlFailureCount(uint32_t url_failure_count) {
@@ -877,11 +870,16 @@
}
void PayloadState::LoadNumReboots() {
- SetNumReboots(GetPersistedValue(kPrefsNumReboots, false));
+ SetNumReboots(GetPersistedValue(kPrefsNumReboots));
}
void PayloadState::LoadRollbackVersion() {
- SetNumReboots(GetPersistedValue(kPrefsRollbackVersion, true));
+ CHECK(powerwash_safe_prefs_);
+ string rollback_version;
+ if (powerwash_safe_prefs_->GetString(kPrefsRollbackVersion,
+ &rollback_version)) {
+ SetRollbackVersion(rollback_version);
+ }
}
void PayloadState::SetRollbackVersion(const string& rollback_version) {
@@ -990,7 +988,7 @@
void PayloadState::LoadCurrentBytesDownloaded(DownloadSource source) {
string key = GetPrefsKey(kPrefsCurrentBytesDownloaded, source);
- SetCurrentBytesDownloaded(source, GetPersistedValue(key, false), true);
+ SetCurrentBytesDownloaded(source, GetPersistedValue(key), true);
}
void PayloadState::SetCurrentBytesDownloaded(
@@ -1014,7 +1012,7 @@
void PayloadState::LoadTotalBytesDownloaded(DownloadSource source) {
string key = GetPrefsKey(kPrefsTotalBytesDownloaded, source);
- SetTotalBytesDownloaded(source, GetPersistedValue(key, false), true);
+ SetTotalBytesDownloaded(source, GetPersistedValue(key), true);
}
void PayloadState::SetTotalBytesDownloaded(
@@ -1038,7 +1036,7 @@
}
void PayloadState::LoadNumResponsesSeen() {
- SetNumResponsesSeen(GetPersistedValue(kPrefsNumResponsesSeen, false));
+ SetNumResponsesSeen(GetPersistedValue(kPrefsNumResponsesSeen));
}
void PayloadState::SetNumResponsesSeen(int num_responses_seen) {
@@ -1241,7 +1239,7 @@
}
void PayloadState::LoadP2PNumAttempts() {
- SetP2PNumAttempts(GetPersistedValue(kPrefsP2PNumAttempts, false));
+ SetP2PNumAttempts(GetPersistedValue(kPrefsP2PNumAttempts));
}
Time PayloadState::GetP2PFirstAttemptTimestamp() {
@@ -1258,8 +1256,7 @@
}
void PayloadState::LoadP2PFirstAttemptTimestamp() {
- int64_t stored_value = GetPersistedValue(kPrefsP2PFirstAttemptTimestamp,
- false);
+ int64_t stored_value = GetPersistedValue(kPrefsP2PFirstAttemptTimestamp);
Time stored_time = Time::FromInternalValue(stored_value);
SetP2PFirstAttemptTimestamp(stored_time);
}
diff --git a/payload_state.h b/payload_state.h
index f358bda..ad0f339 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -114,6 +114,7 @@
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateFailedMetric);
FRIEND_TEST(PayloadStateTest, RebootAfterUpdateSucceed);
FRIEND_TEST(PayloadStateTest, RebootAfterCanceledUpdate);
+ FRIEND_TEST(PayloadStateTest, RollbackVersion);
FRIEND_TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs);
// Increments the payload attempt number used for metrics.
@@ -181,10 +182,9 @@
// reset on a new update.
void ResetDownloadSourcesOnNewUpdate();
- // Returns the persisted value for the given key. It also validates that
- // the value returned is non-negative. If |across_powerwash| is True,
- // get the value that will persist across a powerwash.
- int64_t GetPersistedValue(const std::string& key, bool across_powerwash);
+ // Returns the persisted value from prefs_ for the given key. It also
+ // validates that the value returned is non-negative.
+ int64_t GetPersistedValue(const std::string& key);
// Calculates the response "signature", which is basically a string composed
// of the subset of the fields in the current response that affect the
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 613940e..d8032fe 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -988,6 +988,17 @@
payload_state.Rollback();
EXPECT_EQ(rollback_version, payload_state.GetRollbackVersion());
+
+ // Change it up a little and verify we load it correctly.
+ rollback_version = "2345.0.1";
+ // Let's verify we can reload it correctly.
+ EXPECT_CALL(*mock_powerwash_safe_prefs, GetString(
+ kPrefsRollbackVersion, _)).WillOnce(DoAll(
+ SetArgumentPointee<1>(rollback_version), Return(true)));
+ EXPECT_CALL(*mock_powerwash_safe_prefs, SetString(kPrefsRollbackVersion,
+ rollback_version));
+ payload_state.LoadRollbackVersion();
+ EXPECT_EQ(rollback_version, payload_state.GetRollbackVersion());
}
TEST(PayloadStateTest, DurationsAreCorrect) {