Don't scatter during OOBE or user-initiated update checks.
We need to add logic to disable scattering of downloads if we are in OOBE
or if we're doing a manual update check.
Scheduled checks are already disabled during OOBE, but this extra check
will ensure that any scattering policy (there's a pending work item to get
policy during OOBE) during OOBE will have no effect on the update.
Similarly manual (i.e user-initiated) update checks through
update_engine_client or through Chrome UI should not honor scattering.
That way, this can serve as a simple user-friendly workaround in case
there's any bug in scattering logic that bricks the system by any chance.
BUG=chromeos-31563: Don't scatter during OOBE or manual update checks.
TEST=Updated unit tests. Tested all code paths manually on ZGB and Kaen.
Change-Id: Ib631e560c1f620ca53db79ee59dc66efb27ea83c
Reviewed-on: https://gerrit.chromium.org/gerrit/24564
Commit-Ready: Jay Srinivasan <jaysri@chromium.org>
Reviewed-by: Jay Srinivasan <jaysri@chromium.org>
Tested-by: Jay Srinivasan <jaysri@chromium.org>
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index 0ba1179..2a0d5a6 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -19,8 +19,10 @@
#include "update_engine/http_fetcher_unittest.h"
#include "update_engine/libcurl_http_fetcher.h"
#include "update_engine/mock_http_fetcher.h"
+#include "update_engine/mock_system_state.h"
#include "update_engine/multi_range_http_fetcher.h"
#include "update_engine/proxy_resolver.h"
+#include "update_engine/utils.h"
using std::make_pair;
using std::pair;
@@ -203,7 +205,8 @@
CHECK(num_proxies > 0);
proxy_resolver_.set_num_proxies(num_proxies);
LibcurlHttpFetcher *ret = new
- LibcurlHttpFetcher(reinterpret_cast<ProxyResolver*>(&proxy_resolver_));
+ LibcurlHttpFetcher(reinterpret_cast<ProxyResolver*>(&proxy_resolver_),
+ &mock_system_state_);
// Speed up test execution.
ret->set_idle_seconds(1);
ret->set_retry_seconds(1);
@@ -240,6 +243,8 @@
virtual HttpServer *CreateServer() {
return new PythonHttpServer;
}
+
+ MockSystemState mock_system_state_;
};
class MultiRangeHttpFetcherTest : public LibcurlHttpFetcherTest {
@@ -251,8 +256,8 @@
proxy_resolver_.set_num_proxies(num_proxies);
ProxyResolver* resolver =
reinterpret_cast<ProxyResolver*>(&proxy_resolver_);
- MultiRangeHttpFetcher *ret =
- new MultiRangeHttpFetcher(new LibcurlHttpFetcher(resolver));
+ MultiRangeHttpFetcher *ret = new MultiRangeHttpFetcher(
+ new LibcurlHttpFetcher(resolver, &mock_system_state_));
ret->ClearRanges();
ret->AddRange(0);
// Speed up test execution.
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index e2bfc15..a0bf9a7 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -213,8 +213,9 @@
transfer_size_ = -1;
resume_offset_ = 0;
retry_count_ = 0;
- max_retry_count_ = (utils::IsOOBEComplete()) ? kMaxRetryCountOobeComplete
- : kMaxRetryCountOobeNotComplete;
+ max_retry_count_ = (system_state_->IsOOBEComplete() ?
+ kMaxRetryCountOobeComplete :
+ kMaxRetryCountOobeNotComplete);
no_network_retry_count_ = 0;
http_response_code_ = 0;
terminate_requested_ = false;
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index 7b9f12c..f2239b1 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -27,8 +27,10 @@
static const int kMaxRetryCountOobeComplete = 20;
static const int kMaxRetryCountOobeNotComplete = 3;
- explicit LibcurlHttpFetcher(ProxyResolver* proxy_resolver)
+ explicit LibcurlHttpFetcher(ProxyResolver* proxy_resolver,
+ SystemState* system_state)
: HttpFetcher(proxy_resolver),
+ system_state_(system_state),
curl_multi_handle_(NULL),
curl_handle_(NULL),
curl_http_headers_(NULL),
@@ -192,6 +194,9 @@
// Returns whether or not the current build is official.
bool IsOfficialBuild() const;
+ // External state of the system
+ SystemState* system_state_;
+
// Handles for the libcurl library
CURLM *curl_multi_handle_;
CURL *curl_handle_;
diff --git a/main.cc b/main.cc
index fc0319f..213bb7d 100644
--- a/main.cc
+++ b/main.cc
@@ -26,6 +26,7 @@
#include "update_engine/terminator.h"
#include "update_engine/update_attempter.h"
#include "update_engine/update_check_scheduler.h"
+#include "update_engine/utils.h"
extern "C" {
#include "update_engine/update_engine.dbusserver.h"
@@ -181,10 +182,12 @@
// Create the update attempter:
chromeos_update_engine::ConcreteDbusGlib dbus;
+ chromeos_update_engine::RealSystemState real_system_state;
chromeos_update_engine::UpdateAttempter update_attempter(&prefs,
&metrics_lib,
&dbus,
- NULL);
+ NULL,
+ &real_system_state);
// Create the dbus service object:
dbus_g_object_type_install_info(UPDATE_ENGINE_TYPE_SERVICE,
@@ -197,7 +200,8 @@
// Schedule periodic update checks.
chromeos_update_engine::UpdateCheckScheduler scheduler(&update_attempter,
- NULL);
+ NULL,
+ &real_system_state);
scheduler.Run();
// Update boot flags after 45 seconds.
diff --git a/mock_system_state.h b/mock_system_state.h
new file mode 100644
index 0000000..0d9f1fd
--- /dev/null
+++ b/mock_system_state.h
@@ -0,0 +1,23 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_MOCK_SYSTEM_STATE_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_MOCK_SYSTEM_STATE_H__
+
+#include <gmock/gmock.h>
+
+#include "update_engine/utils.h"
+
+namespace chromeos_update_engine {
+
+// Mock the SystemStateInterface so that we could lie that
+// OOBE is completed even when there's no such marker file, etc.
+class MockSystemState : public SystemState {
+ public:
+ MOCK_METHOD0(IsOOBEComplete, bool());
+};
+
+} // namespeace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_MOCK_SYSTEM_STATE_H__
diff --git a/prefs.cc b/prefs.cc
index cb6e4f7..1388817 100644
--- a/prefs.cc
+++ b/prefs.cc
@@ -45,6 +45,7 @@
FilePath filename;
TEST_AND_RETURN_FALSE(GetFileNameForKey(key, &filename));
if (!file_util::ReadFileToString(filename, value)) {
+ LOG(INFO) << key << " not present in " << prefs_dir_.value();
return false;
}
return true;
@@ -62,7 +63,8 @@
bool Prefs::GetInt64(const string& key, int64_t* value) {
string str_value;
- TEST_AND_RETURN_FALSE(GetString(key, &str_value));
+ if (!GetString(key, &str_value))
+ return false;
TrimWhitespaceASCII(str_value, TRIM_ALL, &str_value);
TEST_AND_RETURN_FALSE(base::StringToInt64(str_value, value));
return true;
diff --git a/update_attempter.cc b/update_attempter.cc
index da9e7cc..52d6e4c 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -107,7 +107,8 @@
UpdateAttempter::UpdateAttempter(PrefsInterface* prefs,
MetricsLibraryInterface* metrics_lib,
DbusGlibInterface* dbus_iface,
- GpioHandler* gpio_handler)
+ GpioHandler* gpio_handler,
+ SystemState* system_state)
: processor_(new ActionProcessor()),
dbus_service_(NULL),
prefs_(prefs),
@@ -133,7 +134,8 @@
is_using_test_url_(false),
is_test_update_attempted_(false),
gpio_handler_(gpio_handler),
- init_waiting_period_from_prefs_(true) {
+ init_waiting_period_from_prefs_(true),
+ system_state_(system_state) {
if (utils::FileExists(kUpdateCompletedMarker))
status_ = UPDATE_STATUS_UPDATED_NEED_REBOOT;
}
@@ -146,7 +148,8 @@
const string& omaha_url,
bool obey_proxies,
bool interactive,
- bool is_test) {
+ bool is_test,
+ bool is_user_initiated) {
chrome_proxy_resolver_.Init();
fake_update_success_ = false;
if (status_ == UPDATE_STATUS_UPDATED_NEED_REBOOT) {
@@ -166,7 +169,8 @@
omaha_url,
obey_proxies,
interactive,
- is_test)) {
+ is_test,
+ is_user_initiated)) {
return;
}
@@ -185,7 +189,8 @@
const string& omaha_url,
bool obey_proxies,
bool interactive,
- bool is_test) {
+ bool is_test,
+ bool is_user_initiated) {
http_response_code_ = 0;
// Lazy initialize the policy provider, or reload the latest policy data.
@@ -209,54 +214,62 @@
&omaha_request_params_.target_version_prefix);
int64 new_scatter_factor_in_secs = 0;
device_policy.GetScatterFactorInSeconds(&new_scatter_factor_in_secs);
+ if (new_scatter_factor_in_secs < 0) // sanitize input, just in case.
+ new_scatter_factor_in_secs = 0;
scatter_factor_ = TimeDelta::FromSeconds(new_scatter_factor_in_secs);
}
- if (scatter_factor_.InSeconds() != old_scatter_factor_in_secs) {
- int64 wait_period_in_secs = 0;
- if (init_waiting_period_from_prefs_ &&
- prefs_->GetInt64(kPrefsWallClockWaitPeriod, &wait_period_in_secs) &&
- wait_period_in_secs >= 0 &&
- wait_period_in_secs <= scatter_factor_.InSeconds()) {
- // This means:
- // 1. This is the first update check to come this far in this process.
- // 2. There's a persisted value for the waiting period available.
- // 3. And that persisted value is still valid.
- // So, in this case, we should reuse the persisted value instead of
- // generating a new random value to improve the chances of a good
- // distribution for scattering.
- omaha_request_params_.waiting_period =
- TimeDelta::FromSeconds(wait_period_in_secs);
- LOG(INFO) << "Using persisted value for wall clock based waiting period.";
- } else {
- // In this case, we should regenerate the waiting period to make sure
- // it's within the bounds of the new scatter factor value.
- omaha_request_params_.waiting_period = TimeDelta::FromSeconds(
- base::RandInt(0, scatter_factor_.InSeconds()));
-
- LOG(INFO) << "Generated new value of " << utils::FormatSecs(
- omaha_request_params_.waiting_period.InSeconds())
- << " for wall clock based waiting period.";
-
- // Do a best-effort to persist this. We'll work fine even if the
- // persistence fails.
- prefs_->SetInt64(kPrefsWallClockWaitPeriod,
- omaha_request_params_.waiting_period.InSeconds());
- }
+ bool is_scatter_enabled = false;
+ if (scatter_factor_.InSeconds() == 0) {
+ LOG(INFO) << "Scattering disabled since scatter factor is set to 0";
+ } else if (is_user_initiated) {
+ LOG(INFO) << "Scattering disabled as this update check is user-initiated";
+ } else if (!system_state_->IsOOBEComplete()) {
+ LOG(INFO) << "Scattering disabled since OOBE is not complete yet";
+ } else {
+ is_scatter_enabled = true;
+ LOG(INFO) << "Scattering is enabled";
}
- // We should reset this value since we're past the first initialization
- // of the waiting period for this process.
- init_waiting_period_from_prefs_ = false;
+ if (is_scatter_enabled) {
+ // This means the scattering policy is turned on.
+ if (scatter_factor_.InSeconds() != old_scatter_factor_in_secs) {
+ int64 wait_period_in_secs = 0;
+ if (init_waiting_period_from_prefs_ &&
+ prefs_->GetInt64(kPrefsWallClockWaitPeriod, &wait_period_in_secs) &&
+ wait_period_in_secs >= 0 &&
+ wait_period_in_secs <= scatter_factor_.InSeconds()) {
+ // This means:
+ // 1. This is the first update check to come this far in this process.
+ // 2. There's a persisted value for the waiting period available.
+ // 3. And that persisted value is still valid.
+ // So, in this case, we should reuse the persisted value instead of
+ // generating a new random value to improve the chances of a good
+ // distribution for scattering.
+ omaha_request_params_.waiting_period =
+ TimeDelta::FromSeconds(wait_period_in_secs);
+ LOG(INFO) << "Using persisted value for wall clock based waiting period.";
+ } else {
+ // In this case, we should regenerate the waiting period to make sure
+ // it's within the bounds of the new scatter factor value.
+ omaha_request_params_.waiting_period = TimeDelta::FromSeconds(
+ base::RandInt(0, scatter_factor_.InSeconds()));
- if (scatter_factor_.InSeconds() == 0) {
- // This means the scattering feature is turned off. Make sure to disable
- // all the knobs so that we don't invoke any scattering related code.
- omaha_request_params_.wall_clock_based_wait_enabled = false;
- omaha_request_params_.update_check_count_wait_enabled = false;
- prefs_->Delete(kPrefsWallClockWaitPeriod);
- prefs_->Delete(kPrefsUpdateCheckCount);
- } else {
+ LOG(INFO) << "Generated new value of " << utils::FormatSecs(
+ omaha_request_params_.waiting_period.InSeconds())
+ << " for wall clock based waiting period.";
+
+ // Do a best-effort to persist this. We'll work fine even if the
+ // persistence fails.
+ prefs_->SetInt64(kPrefsWallClockWaitPeriod,
+ omaha_request_params_.waiting_period.InSeconds());
+ }
+ }
+
+ // We should reset this value since we're past the first initialization
+ // of the waiting period for this process.
+ init_waiting_period_from_prefs_ = false;
+
// This means the scattering policy is turned on. We'll do wall-clock-
// based-wait by default. And if we don't have any issues in accessing
// the file system to do update the update check count value, we'll
@@ -264,6 +277,13 @@
omaha_request_params_.wall_clock_based_wait_enabled = true;
bool decrement_succeeded = DecrementUpdateCheckCount();
omaha_request_params_.update_check_count_wait_enabled = decrement_succeeded;
+ } else {
+ // This means the scattering feature is turned off. Make sure to disable
+ // all the knobs so that we don't invoke any scattering related code.
+ omaha_request_params_.wall_clock_based_wait_enabled = false;
+ omaha_request_params_.update_check_count_wait_enabled = false;
+ prefs_->Delete(kPrefsWallClockWaitPeriod);
+ prefs_->Delete(kPrefsUpdateCheckCount);
}
// Determine whether an alternative test address should be used.
@@ -322,7 +342,7 @@
// Actions:
LibcurlHttpFetcher* update_check_fetcher =
- new LibcurlHttpFetcher(GetProxyResolver());
+ new LibcurlHttpFetcher(GetProxyResolver(), system_state_);
// Try harder to connect to the network, esp when not interactive.
// See comment in libcurl_http_fetcher.cc.
update_check_fetcher->set_no_network_max_retries(interactive ? 1 : 3);
@@ -344,10 +364,11 @@
&omaha_request_params_,
new OmahaEvent(
OmahaEvent::kTypeUpdateDownloadStarted),
- new LibcurlHttpFetcher(GetProxyResolver()),
+ new LibcurlHttpFetcher(GetProxyResolver(),
+ system_state_),
false));
LibcurlHttpFetcher* download_fetcher =
- new LibcurlHttpFetcher(GetProxyResolver());
+ new LibcurlHttpFetcher(GetProxyResolver(), system_state_);
download_fetcher->set_check_certificate(CertificateChecker::kDownload);
shared_ptr<DownloadAction> download_action(
new DownloadAction(prefs_,
@@ -358,7 +379,8 @@
&omaha_request_params_,
new OmahaEvent(
OmahaEvent::kTypeUpdateDownloadFinished),
- new LibcurlHttpFetcher(GetProxyResolver()),
+ new LibcurlHttpFetcher(GetProxyResolver(),
+ system_state_),
false));
shared_ptr<FilesystemCopierAction> filesystem_verifier_action(
new FilesystemCopierAction(false, true));
@@ -370,7 +392,8 @@
new OmahaRequestAction(prefs_,
&omaha_request_params_,
new OmahaEvent(OmahaEvent::kTypeUpdateComplete),
- new LibcurlHttpFetcher(GetProxyResolver()),
+ new LibcurlHttpFetcher(GetProxyResolver(),
+ system_state_),
false));
download_action->set_delegate(this);
@@ -417,9 +440,11 @@
void UpdateAttempter::CheckForUpdate(const string& app_version,
const string& omaha_url) {
+ LOG(INFO) << "New update check requested";
+
if (status_ != UPDATE_STATUS_IDLE) {
- LOG(INFO) << "Check for update requested, but status is "
- << UpdateStatusToString(status_) << ", so not checking.";
+ LOG(INFO) << "Skipping update check because current status is "
+ << UpdateStatusToString(status_);
return;
}
@@ -433,7 +458,9 @@
is_test_update_attempted_ = true;
}
- Update(app_version, omaha_url, true, true, is_test);
+ // Passing true for is_user_initiated to indicate that this
+ // is not a scheduled update check.
+ Update(app_version, omaha_url, true, true, is_test, true);
}
bool UpdateAttempter::RebootIfNeeded() {
@@ -752,7 +779,8 @@
new OmahaRequestAction(prefs_,
&omaha_request_params_,
error_event_.release(), // Pass ownership.
- new LibcurlHttpFetcher(GetProxyResolver()),
+ new LibcurlHttpFetcher(GetProxyResolver(),
+ system_state_),
false));
actions_.push_back(shared_ptr<AbstractAction>(error_event_action));
processor_->EnqueueAction(error_event_action.get());
@@ -866,7 +894,8 @@
new OmahaRequestAction(prefs_,
&omaha_request_params_,
NULL,
- new LibcurlHttpFetcher(GetProxyResolver()),
+ new LibcurlHttpFetcher(GetProxyResolver(),
+ system_state_),
true));
actions_.push_back(shared_ptr<OmahaRequestAction>(ping_action));
processor_->set_delegate(NULL);
diff --git a/update_attempter.h b/update_attempter.h
index b38367d..deb0ff2 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -62,7 +62,8 @@
UpdateAttempter(PrefsInterface* prefs,
MetricsLibraryInterface* metrics_lib,
DbusGlibInterface* dbus_iface,
- GpioHandler* gpio_handler);
+ GpioHandler* gpio_handler,
+ SystemState* system_state);
virtual ~UpdateAttempter();
// Checks for update and, if a newer version is available, attempts to update
@@ -71,12 +72,15 @@
// update will likely respect Chrome's proxy setting. For security reasons, we
// may still not honor them. Interactive should be true if this was called
// from the user (ie dbus). |is_test| will lead to using an alternative test
- // server URL, if |omaha_url| is empty.
+ // server URL, if |omaha_url| is empty. |is_user_initiated| will be true
+ // only if the update is being kicked off through dbus and will be false for
+ // other types of kick off such as scheduled updates.
virtual void Update(const std::string& app_version,
const std::string& omaha_url,
bool obey_proxies,
bool interactive,
- bool is_test);
+ bool is_test,
+ bool is_user_initiated);
// ActionProcessorDelegate methods:
void ProcessingDone(const ActionProcessor* processor, ActionExitCode code);
@@ -232,7 +236,8 @@
const std::string& omaha_url,
bool obey_proxies,
bool interactive,
- bool is_test);
+ bool is_test,
+ bool is_user_initiated);
// Helper method of Update() to construct the sequence of actions to
// be performed for an update check. Please refer to
@@ -340,6 +345,10 @@
// False otherwise.
bool init_waiting_period_from_prefs_;
+ // External state of the system outside the update_engine process
+ // carved out separately to mock out easily in unit tests.
+ SystemState* system_state_;
+
DISALLOW_COPY_AND_ASSIGN(UpdateAttempter);
};
diff --git a/update_attempter_mock.h b/update_attempter_mock.h
index bdb83ff..5ea7f27 100644
--- a/update_attempter_mock.h
+++ b/update_attempter_mock.h
@@ -15,13 +15,14 @@
class UpdateAttempterMock : public UpdateAttempter {
public:
explicit UpdateAttempterMock(MockDbusGlib* dbus)
- : UpdateAttempter(NULL, NULL, dbus, NULL) {}
+ : UpdateAttempter(NULL, NULL, dbus, NULL, NULL) {}
- MOCK_METHOD5(Update, void(const std::string& app_version,
+ MOCK_METHOD6(Update, void(const std::string& app_version,
const std::string& omaha_url,
bool obey_proxies,
bool interactive,
- bool is_test));
+ bool is_test,
+ bool is_user_initiated));
};
} // namespace chromeos_update_engine
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 5a0bed6..424ca18 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -12,6 +12,7 @@
#include "update_engine/filesystem_copier_action.h"
#include "update_engine/mock_dbus_interface.h"
#include "update_engine/mock_http_fetcher.h"
+#include "update_engine/mock_system_state.h"
#include "update_engine/postinstall_runner_action.h"
#include "update_engine/prefs.h"
#include "update_engine/prefs_mock.h"
@@ -37,7 +38,7 @@
class UpdateAttempterUnderTest : public UpdateAttempter {
public:
explicit UpdateAttempterUnderTest(MockDbusGlib* dbus)
- : UpdateAttempter(NULL, NULL, dbus, NULL) {}
+ : UpdateAttempter(NULL, NULL, dbus, NULL, NULL) {}
};
class UpdateAttempterTest : public ::testing::Test {
@@ -46,6 +47,7 @@
virtual void SetUp() {
EXPECT_EQ(NULL, attempter_.dbus_service_);
EXPECT_EQ(NULL, attempter_.prefs_);
+ EXPECT_EQ(NULL, attempter_.system_state_);
EXPECT_EQ(NULL, attempter_.metrics_lib_);
EXPECT_EQ(NULL, attempter_.update_check_scheduler_);
EXPECT_EQ(0, attempter_.http_response_code_);
@@ -60,6 +62,7 @@
processor_ = new ActionProcessorMock();
attempter_.processor_.reset(processor_); // Transfers ownership.
attempter_.prefs_ = &prefs_;
+ attempter_.system_state_ = &mock_system_state_;
}
void QuitMainLoop();
@@ -91,10 +94,15 @@
static gboolean StaticDecrementUpdateCheckCountTestStart(
gpointer data);
+ void NoScatteringDoneDuringManualUpdateTestStart();
+ static gboolean StaticNoScatteringDoneDuringManualUpdateTestStart(
+ gpointer data);
+
MockDbusGlib dbus_;
UpdateAttempterUnderTest attempter_;
ActionProcessorMock* processor_;
NiceMock<PrefsMock> prefs_;
+ MockSystemState mock_system_state_;
GMainLoop* loop_;
};
@@ -129,7 +137,7 @@
OmahaResponse response;
response.poll_interval = 234;
action.SetOutputObject(response);
- UpdateCheckScheduler scheduler(&attempter_, NULL);
+ UpdateCheckScheduler scheduler(&attempter_, NULL, &mock_system_state_);
attempter_.set_update_check_scheduler(&scheduler);
EXPECT_CALL(prefs_, GetInt64(kPrefsDeltaUpdateFailures, _)).Times(0);
attempter_.ActionCompleted(NULL, &action, kActionCodeSuccess);
@@ -316,6 +324,13 @@
return FALSE;
}
+gboolean UpdateAttempterTest::StaticNoScatteringDoneDuringManualUpdateTestStart(
+ gpointer data) {
+ UpdateAttempterTest* ua_test = reinterpret_cast<UpdateAttempterTest*>(data);
+ ua_test->NoScatteringDoneDuringManualUpdateTestStart();
+ return FALSE;
+}
+
namespace {
const string kActionTypes[] = {
OmahaRequestAction::StaticType(),
@@ -342,7 +357,7 @@
}
EXPECT_CALL(*processor_, StartProcessing()).Times(1);
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
g_idle_add(&StaticUpdateTestVerify, this);
}
@@ -382,7 +397,7 @@
}
TEST_F(UpdateAttempterTest, PingOmahaTest) {
- UpdateCheckScheduler scheduler(&attempter_, NULL);
+ UpdateCheckScheduler scheduler(&attempter_, NULL, &mock_system_state_);
scheduler.enabled_ = true;
EXPECT_FALSE(scheduler.scheduled_);
attempter_.set_update_check_scheduler(&scheduler);
@@ -442,7 +457,7 @@
SetArgumentPointee<0>(std::string("canary-channel")),
Return(true)));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_EQ("canary-channel", attempter_.omaha_request_params_.app_track);
g_idle_add(&StaticQuitMainLoop, this);
@@ -470,7 +485,7 @@
SetArgumentPointee<0>(true),
Return(true)));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_TRUE(attempter_.omaha_request_params_.update_disabled);
g_idle_add(&StaticQuitMainLoop, this);
@@ -500,7 +515,7 @@
SetArgumentPointee<0>(target_version_prefix),
Return(true)));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_EQ(target_version_prefix.c_str(),
attempter_.omaha_request_params_.target_version_prefix);
@@ -531,7 +546,7 @@
SetArgumentPointee<0>(scatter_factor_in_seconds),
Return(true)));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_EQ(scatter_factor_in_seconds, attempter_.scatter_factor_.InSeconds());
g_idle_add(&StaticQuitMainLoop, this);
@@ -552,6 +567,9 @@
Prefs prefs;
attempter_.prefs_ = &prefs;
+ EXPECT_CALL(mock_system_state_,
+ IsOOBEComplete()).WillRepeatedly(Return(true));
+
string prefs_dir;
EXPECT_TRUE(utils::MakeTempDirectory("/tmp/ue_ut_prefs.XXXXXX",
&prefs_dir));
@@ -573,7 +591,7 @@
SetArgumentPointee<0>(scatter_factor_in_seconds),
Return(true)));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_EQ(scatter_factor_in_seconds, attempter_.scatter_factor_.InSeconds());
// Make sure the file still exists.
@@ -588,7 +606,7 @@
// However, if the count is already 0, it's not decremented. Test that.
initial_value = 0;
EXPECT_TRUE(prefs.SetInt64(kPrefsUpdateCheckCount, initial_value));
- attempter_.Update("", "", false, false, false);
+ attempter_.Update("", "", false, false, false, false);
EXPECT_TRUE(prefs.Exists(kPrefsUpdateCheckCount));
EXPECT_TRUE(prefs.GetInt64(kPrefsUpdateCheckCount, &new_value));
EXPECT_EQ(initial_value, new_value);
@@ -596,4 +614,61 @@
g_idle_add(&StaticQuitMainLoop, this);
}
+TEST_F(UpdateAttempterTest, NoScatteringDoneDuringManualUpdateTestStart) {
+ loop_ = g_main_loop_new(g_main_context_default(), FALSE);
+ g_idle_add(&StaticNoScatteringDoneDuringManualUpdateTestStart, this);
+ g_main_loop_run(loop_);
+ g_main_loop_unref(loop_);
+ loop_ = NULL;
+}
+
+void UpdateAttempterTest::NoScatteringDoneDuringManualUpdateTestStart() {
+ // Tests that no scattering logic is enabled if the update check
+ // is manually done (as opposed to a scheduled update check)
+ int64 initial_value = 8;
+ Prefs prefs;
+ attempter_.prefs_ = &prefs;
+
+ EXPECT_CALL(mock_system_state_,
+ IsOOBEComplete()).WillRepeatedly(Return(true));
+
+ string prefs_dir;
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/ue_ut_prefs.XXXXXX",
+ &prefs_dir));
+ ScopedDirRemover temp_dir_remover(prefs_dir);
+
+ LOG_IF(ERROR, !prefs.Init(FilePath(prefs_dir)))
+ << "Failed to initialize preferences.";
+ EXPECT_TRUE(prefs.SetInt64(kPrefsUpdateCheckCount, initial_value));
+
+ // make sure scatter_factor is non-zero as scattering is disabled
+ // otherwise.
+ int64 scatter_factor_in_seconds = 50;
+
+ policy::MockDevicePolicy* device_policy = new policy::MockDevicePolicy();
+ attempter_.policy_provider_.reset(new policy::PolicyProvider(device_policy));
+
+ EXPECT_CALL(*device_policy, LoadPolicy()).WillRepeatedly(Return(true));
+
+ EXPECT_CALL(*device_policy, GetScatterFactorInSeconds(_))
+ .WillRepeatedly(DoAll(
+ SetArgumentPointee<0>(scatter_factor_in_seconds),
+ Return(true)));
+
+ // pass true for is_user_initiated so we can test that the
+ // scattering is disabled.
+ attempter_.Update("", "", false, false, false, true);
+ EXPECT_EQ(scatter_factor_in_seconds, attempter_.scatter_factor_.InSeconds());
+
+ // Make sure scattering is disabled for manual (i.e. user initiated) update
+ // checks and none of the artifacts are present.
+ EXPECT_FALSE(attempter_.omaha_request_params_.wall_clock_based_wait_enabled);
+ EXPECT_FALSE(prefs.Exists(kPrefsWallClockWaitPeriod));
+ EXPECT_FALSE(attempter_.omaha_request_params_.
+ update_check_count_wait_enabled);
+ EXPECT_FALSE(prefs.Exists(kPrefsUpdateCheckCount));
+
+ g_idle_add(&StaticQuitMainLoop, this);
+}
+
} // namespace chromeos_update_engine
diff --git a/update_check_scheduler.cc b/update_check_scheduler.cc
index c3b0d17..148fd1b 100644
--- a/update_check_scheduler.cc
+++ b/update_check_scheduler.cc
@@ -19,14 +19,16 @@
const int UpdateCheckScheduler::kTimeoutRegularFuzz = 10 * 60;
UpdateCheckScheduler::UpdateCheckScheduler(UpdateAttempter* update_attempter,
- GpioHandler* gpio_handler)
+ GpioHandler* gpio_handler,
+ SystemState* system_state)
: update_attempter_(update_attempter),
enabled_(false),
scheduled_(false),
last_interval_(0),
poll_interval_(0),
is_test_update_attempted_(false),
- gpio_handler_(gpio_handler) {}
+ gpio_handler_(gpio_handler),
+ system_state_(system_state) {}
UpdateCheckScheduler::~UpdateCheckScheduler() {}
@@ -59,10 +61,6 @@
return utils::IsRemovableDevice(utils::RootDevice(utils::BootDevice()));
}
-bool UpdateCheckScheduler::IsOOBEComplete() {
- return utils::IsOOBEComplete();
-}
-
bool UpdateCheckScheduler::IsOfficialBuild() {
return utils::IsOfficialBuild();
}
@@ -92,7 +90,7 @@
me->scheduled_ = false;
bool is_test = false;
- if (me->IsOOBEComplete() ||
+ if (me->system_state_->IsOOBEComplete() ||
(is_test = (!me->is_test_update_attempted_ &&
me->gpio_handler_ &&
me->gpio_handler_->IsTestModeSignaled()))) {
@@ -104,7 +102,7 @@
// Before updating, we flush any previously generated UMA reports.
CertificateChecker::FlushReport();
- me->update_attempter_->Update("", "", false, false, is_test);
+ me->update_attempter_->Update("", "", false, false, is_test, false);
} else {
// Skips all automatic update checks if the OOBE process is not complete and
// schedules a new check as if it is the first one.
diff --git a/update_check_scheduler.h b/update_check_scheduler.h
index cd3ad87..16f831e 100644
--- a/update_check_scheduler.h
+++ b/update_check_scheduler.h
@@ -42,7 +42,8 @@
static const int kTimeoutMaxBackoffInterval;
UpdateCheckScheduler(UpdateAttempter* update_attempter,
- GpioHandler* gpio_handler);
+ GpioHandler* gpio_handler,
+ SystemState* system_state);
virtual ~UpdateCheckScheduler();
// Initiates the periodic update checks, if necessary.
@@ -65,7 +66,6 @@
FRIEND_TEST(UpdateCheckSchedulerTest, ComputeNextIntervalAndFuzzTest);
FRIEND_TEST(UpdateCheckSchedulerTest, GTimeoutAddSecondsTest);
FRIEND_TEST(UpdateCheckSchedulerTest, IsBootDeviceRemovableTest);
- FRIEND_TEST(UpdateCheckSchedulerTest, IsOOBECompleteTest);
FRIEND_TEST(UpdateCheckSchedulerTest, IsOfficialBuildTest);
FRIEND_TEST(UpdateCheckSchedulerTest, RunBootDeviceRemovableTest);
FRIEND_TEST(UpdateCheckSchedulerTest, RunNonOfficialBuildTest);
@@ -87,7 +87,6 @@
// Wrappers for utils functions so that they can be mocked in tests.
virtual bool IsBootDeviceRemovable();
- virtual bool IsOOBEComplete();
virtual bool IsOfficialBuild();
// Returns true if an update check can be scheduled. An update check should
@@ -135,6 +134,9 @@
// GPIO handler object.
GpioHandler* gpio_handler_;
+ // The external state of the system outside the update_engine process.
+ SystemState* system_state_;
+
DISALLOW_COPY_AND_ASSIGN(UpdateCheckScheduler);
};
diff --git a/update_check_scheduler_unittest.cc b/update_check_scheduler_unittest.cc
index 965ce9d..2e7e62d 100644
--- a/update_check_scheduler_unittest.cc
+++ b/update_check_scheduler_unittest.cc
@@ -4,6 +4,7 @@
#include <gtest/gtest.h>
+#include "update_engine/mock_system_state.h"
#include "update_engine/update_attempter_mock.h"
#include "update_engine/update_check_scheduler.h"
@@ -30,12 +31,13 @@
class UpdateCheckSchedulerUnderTest : public UpdateCheckScheduler {
public:
UpdateCheckSchedulerUnderTest(UpdateAttempter* update_attempter)
- : UpdateCheckScheduler(update_attempter, NULL) {}
+ : UpdateCheckScheduler(update_attempter, NULL, &mock_system_state_) {}
MOCK_METHOD2(GTimeoutAddSeconds, guint(guint seconds, GSourceFunc function));
MOCK_METHOD0(IsBootDeviceRemovable, bool());
MOCK_METHOD0(IsOfficialBuild, bool());
- MOCK_METHOD0(IsOOBEComplete, bool());
+
+ MockSystemState mock_system_state_;
};
class UpdateCheckSchedulerTest : public ::testing::Test {
@@ -155,11 +157,6 @@
EXPECT_FALSE(scheduler_.UpdateCheckScheduler::IsBootDeviceRemovable());
}
-TEST_F(UpdateCheckSchedulerTest, IsOOBECompleteTest) {
- // Invokes the actual utils wrapper method rather than the subclass mock.
- EXPECT_FALSE(scheduler_.UpdateCheckScheduler::IsOOBEComplete());
-}
-
TEST_F(UpdateCheckSchedulerTest, IsOfficialBuildTest) {
// Invokes the actual utils wrapper method rather than the subclass mock.
EXPECT_TRUE(scheduler_.UpdateCheckScheduler::IsOfficialBuild());
@@ -276,8 +273,9 @@
TEST_F(UpdateCheckSchedulerTest, StaticCheckOOBECompleteTest) {
scheduler_.scheduled_ = true;
- EXPECT_CALL(scheduler_, IsOOBEComplete()).Times(1).WillOnce(Return(true));
- EXPECT_CALL(attempter_, Update("", "", false, false, false))
+ EXPECT_CALL(scheduler_.mock_system_state_,
+ IsOOBEComplete()).Times(1).WillOnce(Return(true));
+ EXPECT_CALL(attempter_, Update("", "", false, false, false, false))
.Times(1)
.WillOnce(Assign(&scheduler_.scheduled_, true));
scheduler_.enabled_ = true;
@@ -287,8 +285,9 @@
TEST_F(UpdateCheckSchedulerTest, StaticCheckOOBENotCompleteTest) {
scheduler_.scheduled_ = true;
- EXPECT_CALL(scheduler_, IsOOBEComplete()).Times(1).WillOnce(Return(false));
- EXPECT_CALL(attempter_, Update("", "", _, _, _)).Times(0);
+ EXPECT_CALL(scheduler_.mock_system_state_,
+ IsOOBEComplete()).Times(1).WillOnce(Return(false));
+ EXPECT_CALL(attempter_, Update("", "", _, _, _,_)).Times(0);
int interval_min, interval_max;
FuzzRange(UpdateCheckScheduler::kTimeoutInitialInterval,
UpdateCheckScheduler::kTimeoutRegularFuzz,
diff --git a/utils.cc b/utils.cc
index bf9267c..769d7fe 100644
--- a/utils.cc
+++ b/utils.cc
@@ -41,9 +41,14 @@
namespace chromeos_update_engine {
+static const char kOOBECompletedMarker[] = "/home/chronos/.oobe_completed";
+
+bool RealSystemState::IsOOBEComplete() {
+ return file_util::PathExists(FilePath(kOOBECompletedMarker));
+}
+
namespace utils {
-static const char kOOBECompletedMarker[] = "/home/chronos/.oobe_completed";
static const char kDevImageMarker[] = "/root/.dev_mode";
const char* const kStatefulPartition = "/mnt/stateful_partition";
@@ -51,10 +56,6 @@
return !file_util::PathExists(FilePath(kDevImageMarker));
}
-bool IsOOBEComplete() {
- return file_util::PathExists(FilePath(kOOBECompletedMarker));
-}
-
bool IsNormalBootMode() {
// TODO(petkov): Convert to a library call once a crossystem library is
// available (crosbug.com/13291).
diff --git a/utils.h b/utils.h
index cce30ca..fd9782a 100644
--- a/utils.h
+++ b/utils.h
@@ -27,10 +27,6 @@
// Returns true if this is an official Chrome OS build, false otherwise.
bool IsOfficialBuild();
-// Returns true if the OOBE process has been completed and EULA accepted, false
-// otherwise.
-bool IsOOBEComplete();
-
// Returns true if the boot mode is normal or if it's unable to determine the
// boot mode. Returns false if the boot mode is developer.
bool IsNormalBootMode();
@@ -272,6 +268,25 @@
} // namespace utils
+
+// An interface to get the state of the current system.
+// Carved out separately so it can be mocked for unit tests.
+// Currently it has only one method, but we should start migrating other
+// methods to use this as and when needed to unit test them.
+class SystemState {
+ public:
+ // Returns true if the OOBE process has been completed and EULA accepted.
+ // False otherwise.
+ virtual bool IsOOBEComplete() = 0;
+};
+
+// A real implementation of the SystemStateInterface which is
+// used by the actual product code.
+class RealSystemState : public SystemState {
+ public:
+ virtual bool IsOOBEComplete();
+};
+
// Class to unmount FS when object goes out of scope
class ScopedFilesystemUnmounter {
public: