update_engine: do not send platform updatecheck for install
Currently we send updatecheck for platform for install. Based on what
our conclusion earlier, we should not send updatecheck for platform in
case of an install.
In such case where updatecheck is not sent in request, updatecheck
tag is also missing in response. We address this change accordingly in
the response parser. Unit test is also updated accordingly.
BUG=chromium:879313
TEST=unittest
Change-Id: I1d7b0ac13062b268bbde9f45e713a039eeeb924e
Reviewed-on: https://chromium-review.googlesource.com/1277717
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index de02b5e..6adacc3 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -149,6 +149,7 @@
OmahaRequestParams* params,
bool ping_only,
bool include_ping,
+ bool skip_updatecheck,
int ping_active_days,
int ping_roll_call_days,
PrefsInterface* prefs) {
@@ -157,17 +158,20 @@
if (include_ping)
app_body = GetPingXml(ping_active_days, ping_roll_call_days);
if (!ping_only) {
- app_body += " <updatecheck";
- if (!params->target_version_prefix().empty()) {
- app_body += base::StringPrintf(
- " targetversionprefix=\"%s\"",
- XmlEncodeWithDefault(params->target_version_prefix(), "").c_str());
- // Rollback requires target_version_prefix set.
- if (params->rollback_allowed()) {
- app_body += " rollback_allowed=\"true\"";
+ if (!skip_updatecheck) {
+ app_body += " <updatecheck";
+ if (!params->target_version_prefix().empty()) {
+ app_body += base::StringPrintf(
+ " targetversionprefix=\"%s\"",
+ XmlEncodeWithDefault(params->target_version_prefix(), "")
+ .c_str());
+ // Rollback requires target_version_prefix set.
+ if (params->rollback_allowed()) {
+ app_body += " rollback_allowed=\"true\"";
+ }
}
+ app_body += "></updatecheck>\n";
}
- app_body += "></updatecheck>\n";
// If this is the first update check after a reboot following a previous
// update, generate an event containing the previous version number. If
@@ -266,12 +270,18 @@
const OmahaAppData& app_data,
bool ping_only,
bool include_ping,
+ bool skip_updatecheck,
int ping_active_days,
int ping_roll_call_days,
int install_date_in_days,
SystemState* system_state) {
- string app_body = GetAppBody(event, params, ping_only, include_ping,
- ping_active_days, ping_roll_call_days,
+ string app_body = GetAppBody(event,
+ params,
+ ping_only,
+ include_ping,
+ skip_updatecheck,
+ ping_active_days,
+ ping_roll_call_days,
system_state->prefs());
string app_versions;
@@ -402,12 +412,13 @@
.id = params->GetAppId(),
.version = params->app_version(),
.product_components = params->product_components()};
- // TODO(xiaochu): send update check flag only when operation is update.
+ // Skips updatecheck for platform app in case of an install operation.
string app_xml = GetAppXml(event,
params,
product_app,
ping_only,
include_ping,
+ params->is_install(), /* skip_updatecheck */
ping_active_days,
ping_roll_call_days,
install_date_in_days,
@@ -420,6 +431,7 @@
system_app,
ping_only,
include_ping,
+ false, /* skip_updatecheck */
ping_active_days,
ping_roll_call_days,
install_date_in_days,
@@ -435,6 +447,7 @@
dlc_app,
ping_only,
include_ping,
+ false, /* skip_updatecheck */
ping_active_days,
ping_roll_call_days,
install_date_in_days,
@@ -892,7 +905,8 @@
bool ParsePackage(OmahaParserData::App* app,
OmahaResponse* output_object,
ScopedActionCompleter* completer) {
- if (app->updatecheck_status == kValNoUpdate) {
+ if (app->updatecheck_status.empty() ||
+ app->updatecheck_status == kValNoUpdate) {
if (!app->packages.empty()) {
LOG(ERROR) << "No update in this <app> but <package> is not empty.";
completer->set_code(ErrorCode::kOmahaResponseInvalid);
@@ -1085,23 +1099,29 @@
OmahaResponse* output_object,
ScopedActionCompleter* completer) {
output_object->update_exists = false;
- for (size_t i = 0; i < parser_data->apps.size(); i++) {
- const string& status = parser_data->apps[i].updatecheck_status;
+ for (const auto& app : parser_data->apps) {
+ const string& status = app.updatecheck_status;
if (status == kValNoUpdate) {
// Don't update if any app has status="noupdate".
- LOG(INFO) << "No update for <app> " << i;
+ LOG(INFO) << "No update for <app> " << app.id;
output_object->update_exists = false;
break;
} else if (status == "ok") {
- if (parser_data->apps[i].action_postinstall_attrs[kAttrNoUpdate] ==
- "true") {
+ auto const& attr_no_update =
+ app.action_postinstall_attrs.find(kAttrNoUpdate);
+ if (attr_no_update != app.action_postinstall_attrs.end() &&
+ attr_no_update->second == "true") {
// noupdate="true" in postinstall attributes means it's an update to
// self, only update if there's at least one app really have update.
- LOG(INFO) << "Update to self for <app> " << i;
+ LOG(INFO) << "Update to self for <app> " << app.id;
} else {
- LOG(INFO) << "Update for <app> " << i;
+ LOG(INFO) << "Update for <app> " << app.id;
output_object->update_exists = true;
}
+ } else if (status.empty() && params_->is_install() &&
+ params_->GetAppId() == app.id) {
+ // Skips the platform app for install operation.
+ LOG(INFO) << "No payload (and ignore) for <app> " << app.id;
} else {
LOG(ERROR) << "Unknown Omaha response status: " << status;
completer->set_code(ErrorCode::kOmahaResponseInvalid);
@@ -1129,11 +1149,20 @@
// this is the system app (this check is intentionally skipped if there is
// no system_app_id set)
output_object->system_version = app.manifest_version;
+ } else if (params_->is_install() &&
+ app.manifest_version != params_->app_version()) {
+ LOG(WARNING) << "An app has a different version (" << app.manifest_version
+ << ") that is different than platform app version ("
+ << params_->app_version() << ")";
}
if (!app.action_postinstall_attrs.empty() && attrs.empty()) {
attrs = app.action_postinstall_attrs;
}
}
+ if (params_->is_install()) {
+ LOG(INFO) << "Use request version for Install operation.";
+ output_object->version = params_->app_version();
+ }
if (output_object->version.empty()) {
LOG(ERROR) << "Omaha Response does not have version in manifest!";
completer->set_code(ErrorCode::kOmahaResponseInvalid);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 759eeeb..06d937d 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -78,8 +78,10 @@
"Don't change the value of kRollforward infinity unless its "
"size has been changed in firmware.");
+const char kCurrentVersion[] = "0.1.0.0";
const char kTestAppId[] = "test-app-id";
const char kTestAppId2[] = "test-app2-id";
+const char kTestAppIdSkipUpdatecheck[] = "test-app-id-skip-updatecheck";
// This is a helper struct to allow unit tests build an update response with the
// values they care about.
@@ -180,6 +182,9 @@
(multi_app_no_update
? "<app><updatecheck status=\"noupdate\"/></app>"
: "") +
+ (multi_app_skip_updatecheck
+ ? "<app appid=\"" + app_id_skip_updatecheck + "\"></app>"
+ : "") +
"</response>";
}
@@ -190,6 +195,8 @@
string app_id = kTestAppId;
string app_id2 = kTestAppId2;
+ string app_id_skip_updatecheck = kTestAppIdSkipUpdatecheck;
+ string current_version = kCurrentVersion;
string version = "1.2.3.4";
string version2 = "2.3.4.5";
string more_info_url = "http://more/info";
@@ -224,6 +231,8 @@
bool multi_app_self_update = false;
// Whether to include an additional app with status="noupdate".
bool multi_app_no_update = false;
+ // Whether to include an additional app with no updatecheck tag.
+ bool multi_app_skip_updatecheck = false;
// Whether to include more than one package in an app.
bool multi_package = false;
@@ -294,7 +303,7 @@
request_params_.set_os_sp("service_pack");
request_params_.set_os_board("x86-generic");
request_params_.set_app_id(kTestAppId);
- request_params_.set_app_version("0.1.0.0");
+ request_params_.set_app_version(kCurrentVersion);
request_params_.set_app_lang("en-US");
request_params_.set_current_channel("unittest");
request_params_.set_target_channel("unittest");
@@ -307,6 +316,8 @@
request_params_.set_target_version_prefix("");
request_params_.set_rollback_allowed(false);
request_params_.set_is_powerwash_allowed(false);
+ request_params_.set_is_install(false);
+ request_params_.set_dlc_ids({});
fake_system_state_.set_request_params(&request_params_);
fake_system_state_.set_prefs(&fake_prefs_);
@@ -3091,6 +3102,7 @@
TEST_F(OmahaRequestActionTest, InstallTest) {
OmahaResponse response;
+ request_params_.set_is_install(true);
request_params_.set_dlc_ids({"dlc_no_0", "dlc_no_1"});
brillo::Blob post_data;
ASSERT_TRUE(TestUpdateCheck(fake_update_response_.GetUpdateResponse(),
@@ -3109,8 +3121,40 @@
string post_str(post_data.begin(), post_data.end());
for (const auto& dlc_id : request_params_.dlc_ids()) {
EXPECT_NE(string::npos,
- post_str.find("appid=\"test-app-id_" + dlc_id + "\""));
+ post_str.find("appid=\"" + fake_update_response_.app_id + "_" +
+ dlc_id + "\""));
}
+ EXPECT_NE(string::npos,
+ post_str.find("appid=\"" + fake_update_response_.app_id + "\""));
+
+ // Count number of updatecheck tag in response.
+ int updatecheck_count = 0;
+ size_t pos = 0;
+ while ((pos = post_str.find("<updatecheck", pos)) != string::npos) {
+ updatecheck_count++;
+ pos++;
+ }
+ EXPECT_EQ(request_params_.dlc_ids().size(), updatecheck_count);
+}
+
+TEST_F(OmahaRequestActionTest, InstallMissingPlatformVersionTest) {
+ fake_update_response_.multi_app_skip_updatecheck = true;
+ fake_update_response_.multi_app_no_update = false;
+ request_params_.set_is_install(true);
+ request_params_.set_dlc_ids({"dlc_no_0", "dlc_no_1"});
+ request_params_.set_app_id(fake_update_response_.app_id_skip_updatecheck);
+ OmahaResponse response;
+ ASSERT_TRUE(TestUpdateCheck(fake_update_response_.GetUpdateResponse(),
+ -1,
+ false, // ping_only
+ ErrorCode::kSuccess,
+ metrics::CheckResult::kUpdateAvailable,
+ metrics::CheckReaction::kUpdating,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ nullptr));
+ EXPECT_TRUE(response.update_exists);
+ EXPECT_EQ(fake_update_response_.current_version, response.version);
}
} // namespace chromeos_update_engine