[Extensions] Partition UpdateService update check requests.

This would help reduce the size of XML POST update check requests generated
by the update client. Without this change, the update client may generate
requests exceeding the size limit of the update server.

Bug: 722942
Change-Id: I1453e615a2271b8fd376b0706574b22b1d36e26d
Reviewed-on: https://chromium-review.googlesource.com/1033539
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555238}

extensions/browser/updater/update_service.cc

@@ -30,6 +30,10 @@ namespace extensions {
 
 namespace {
 
+// 98% of update checks have 20 or less extensions
+// (see Extensions.UpdateCheckExtension histogram).
+constexpr size_t kMaxExtensionsPerUpdate = 20;
+
 void SendUninstallPingCompleteCallback(update_client::Error error) {}
 
 }  // namespace
@@ -189,10 +193,9 @@ void UpdateService::StartUpdateCheck(
   in_progress_updates_.push_back(InProgressUpdate(std::move(callback)));
   InProgressUpdate& update = in_progress_updates_.back();
 
+  // |update_data| only store update info of extensions that are not being
+  // updated at the moment.
   ExtensionUpdateDataMap update_data;
-  // |extension_ids_to_update| stores the extension IDs from the new update
-  // check request that are not being updated at the moment.
-  std::vector<std::string> extension_ids_to_update;
   for (const auto& update_info : update_params.update_info) {
     const std::string& extension_id = update_info.first;
 
@@ -201,7 +204,6 @@ void UpdateService::StartUpdateCheck(
       continue;
 
     updating_extension_ids_.insert(extension_id);
-    extension_ids_to_update.push_back(extension_id);
 
     ExtensionUpdateData data = update_info.second;
     if (data.is_corrupt_reinstall) {
@@ -215,20 +217,31 @@ void UpdateService::StartUpdateCheck(
   }
 
   UMA_HISTOGRAM_COUNTS_100("Extensions.ExtensionUpdaterUpdateCalls",
-                           extension_ids_to_update.size());
-
-  // If all extension IDs are being updated by previous update check requests,
-  // there's no need to update these extensions again.
-  if (extension_ids_to_update.empty())
-    return;
+                           update_data.size());
+
+  // Divide extensions into batches to reduce the size of update check
+  // requests generated by the update client.
+  for (auto it = update_data.begin(); it != update_data.end();) {
+    ExtensionUpdateDataMap batch_data;
+    size_t batch_size =
+        std::min(kMaxExtensionsPerUpdate,
+                 static_cast<size_t>(std::distance(it, update_data.end())));
+
+    std::vector<std::string> batch_ids;
+    batch_ids.reserve(batch_size);
+    for (size_t i = 0; i < batch_size; ++i, ++it) {
+      batch_ids.push_back(it->first);
+      batch_data.emplace(it->first, std::move(it->second));
+    }
 
-  update_client_->Update(
-      extension_ids_to_update,
-      base::BindOnce(&UpdateDataProvider::GetData, update_data_provider_,
-                     std::move(update_data)),
-      update_params.priority == ExtensionUpdateCheckParams::FOREGROUND,
-      base::BindOnce(&UpdateService::UpdateCheckComplete,
-                     weak_ptr_factory_.GetWeakPtr()));
+    update_client_->Update(
+        batch_ids,
+        base::BindOnce(&UpdateDataProvider::GetData, update_data_provider_,
+                       std::move(batch_data)),
+        update_params.priority == ExtensionUpdateCheckParams::FOREGROUND,
+        base::BindOnce(&UpdateService::UpdateCheckComplete,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void UpdateService::UpdateCheckComplete(update_client::Error error) {
@@ -238,8 +251,11 @@ void UpdateService::UpdateCheckComplete(update_client::Error error) {
   // There must be at least one in-progress update (the one that just
   // finished).
   DCHECK(!in_progress_updates_.empty());
-  // And that the first update should have all of its pendings finished.
-  DCHECK(in_progress_updates_[0].pending_extension_ids.empty());
+
+  if (!in_progress_updates_[0].pending_extension_ids.empty()) {
+    // This can happen when the update check request is batched.
+    return;
+  }
 
   // Find all updates that have finished and remove them from the list.
   in_progress_updates_.erase(

extensions/browser/updater/update_service_unittest.cc

@@ -482,7 +482,7 @@ TEST_F(UpdateServiceTest, InProgressUpdate_Successful) {
       base::BindOnce([](bool* executed) { *executed = true; }, &executed));
   EXPECT_FALSE(executed);
 
-  auto& request = update_client()->update_request(0);
+  const auto& request = update_client()->update_request(0);
   EXPECT_THAT(request.extension_ids,
               testing::ElementsAre("A", "B", "C", "D", "E"));
 
@@ -519,7 +519,7 @@ TEST_F(UpdateServiceTest, InProgressUpdate_Duplicate) {
 
   ASSERT_EQ(1, update_client()->num_update_requests());
 
-  auto& request = update_client()->update_request(0);
+  const auto& request = update_client()->update_request(0);
   EXPECT_THAT(request.extension_ids,
               testing::ElementsAre("A", "B", "C", "D", "E"));
 
@@ -553,8 +553,8 @@ TEST_F(UpdateServiceTest, InProgressUpdate_NonOverlapped) {
   EXPECT_FALSE(executed2);
 
   ASSERT_EQ(2, update_client()->num_update_requests());
-  auto& request1 = update_client()->update_request(0);
-  auto& request2 = update_client()->update_request(1);
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
 
   EXPECT_THAT(request1.extension_ids, testing::ElementsAre("A", "B", "C"));
   EXPECT_THAT(request2.extension_ids, testing::ElementsAre("D", "E"));
@@ -590,8 +590,8 @@ TEST_F(UpdateServiceTest, InProgressUpdate_Overlapped) {
       base::BindOnce([](bool* executed) { *executed = true; }, &executed2));
   EXPECT_FALSE(executed2);
 
-  auto& request1 = update_client()->update_request(0);
-  auto& request2 = update_client()->update_request(1);
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
 
   EXPECT_THAT(request1.extension_ids, testing::ElementsAre("A", "B", "C"));
   EXPECT_THAT(request2.extension_ids, testing::ElementsAre("D"));
@@ -643,8 +643,8 @@ TEST_F(UpdateServiceTest, InProgressUpdate_3Overlapped) {
   EXPECT_FALSE(executed3);
 
   ASSERT_EQ(2, update_client()->num_update_requests());
-  auto& request1 = update_client()->update_request(0);
-  auto& request2 = update_client()->update_request(1);
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
 
   EXPECT_THAT(request1.extension_ids, testing::ElementsAre("A", "B", "C"));
   EXPECT_THAT(request2.extension_ids, testing::ElementsAre("D", "E"));
@@ -709,9 +709,9 @@ TEST_F(UpdateServiceTest, InProgressUpdate_4Overlapped) {
   EXPECT_FALSE(executed4);
 
   ASSERT_EQ(3, update_client()->num_update_requests());
-  auto& request1 = update_client()->update_request(0);
-  auto& request2 = update_client()->update_request(1);
-  auto& request3 = update_client()->update_request(2);
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
+  const auto& request3 = update_client()->update_request(2);
 
   EXPECT_THAT(request1.extension_ids, testing::ElementsAre("A", "B", "C"));
   EXPECT_THAT(request2.extension_ids, testing::ElementsAre("D", "E"));
@@ -732,6 +732,113 @@ TEST_F(UpdateServiceTest, InProgressUpdate_4Overlapped) {
   ASSERT_TRUE(executed4);
 }
 
+TEST_F(UpdateServiceTest, InProgressUpdate_Batch) {
+  // Verify that extensions are batched when the number of extensions exceeds
+  // 20.
+  update_client()->set_delay_update();
+  ExtensionUpdateCheckParams uc;
+
+  for (int i = 0; i < 50; ++i)
+    uc.update_info[base::StringPrintf("A%02d", i)] = ExtensionUpdateData();
+
+  bool executed = false;
+  update_service()->StartUpdateCheck(
+      uc, base::BindOnce([](bool* executed) { *executed = true; }, &executed));
+  EXPECT_FALSE(executed);
+
+  ASSERT_EQ(3, update_client()->num_update_requests());
+
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
+  const auto& request3 = update_client()->update_request(2);
+
+  EXPECT_THAT(
+      request1.extension_ids,
+      testing::ElementsAre("A00", "A01", "A02", "A03", "A04", "A05", "A06",
+                           "A07", "A08", "A09", "A10", "A11", "A12", "A13",
+                           "A14", "A15", "A16", "A17", "A18", "A19"));
+  EXPECT_THAT(
+      request2.extension_ids,
+      testing::ElementsAre("A20", "A21", "A22", "A23", "A24", "A25", "A26",
+                           "A27", "A28", "A29", "A30", "A31", "A32", "A33",
+                           "A34", "A35", "A36", "A37", "A38", "A39"));
+  EXPECT_THAT(request3.extension_ids,
+              testing::ElementsAre("A40", "A41", "A42", "A43", "A44", "A45",
+                                   "A46", "A47", "A48", "A49"));
+
+  update_client()->RunDelayedUpdate(0);
+  EXPECT_FALSE(executed);
+
+  update_client()->RunDelayedUpdate(1);
+  EXPECT_FALSE(executed);
+
+  update_client()->RunDelayedUpdate(2);
+  EXPECT_TRUE(executed);
+}
+
+TEST_F(UpdateServiceTest, InProgressUpdate_NoBatchAndBatch) {
+  update_client()->set_delay_update();
+  ExtensionUpdateCheckParams uc1;
+  ExtensionUpdateCheckParams uc2;
+
+  uc1.update_info["AA"] = ExtensionUpdateData();
+  uc1.update_info["BB"] = ExtensionUpdateData();
+  uc1.update_info["CC"] = ExtensionUpdateData();
+  uc1.update_info["DD"] = ExtensionUpdateData();
+
+  for (int i = 0; i < 50; ++i)
+    uc2.update_info[base::StringPrintf("A%02d", i)] = ExtensionUpdateData();
+
+  bool executed1 = false;
+  update_service()->StartUpdateCheck(
+      uc1,
+      base::BindOnce([](bool* executed) { *executed = true; }, &executed1));
+  EXPECT_FALSE(executed1);
+
+  bool executed2 = false;
+  update_service()->StartUpdateCheck(
+      uc2,
+      base::BindOnce([](bool* executed) { *executed = true; }, &executed2));
+  EXPECT_FALSE(executed2);
+
+  ASSERT_EQ(4, update_client()->num_update_requests());
+
+  const auto& request1 = update_client()->update_request(0);
+  const auto& request2 = update_client()->update_request(1);
+  const auto& request3 = update_client()->update_request(2);
+  const auto& request4 = update_client()->update_request(3);
+
+  EXPECT_THAT(request1.extension_ids,
+              testing::ElementsAre("AA", "BB", "CC", "DD"));
+
+  EXPECT_THAT(
+      request2.extension_ids,
+      testing::ElementsAre("A00", "A01", "A02", "A03", "A04", "A05", "A06",
+                           "A07", "A08", "A09", "A10", "A11", "A12", "A13",
+                           "A14", "A15", "A16", "A17", "A18", "A19"));
+  EXPECT_THAT(
+      request3.extension_ids,
+      testing::ElementsAre("A20", "A21", "A22", "A23", "A24", "A25", "A26",
+                           "A27", "A28", "A29", "A30", "A31", "A32", "A33",
+                           "A34", "A35", "A36", "A37", "A38", "A39"));
+  EXPECT_THAT(request4.extension_ids,
+              testing::ElementsAre("A40", "A41", "A42", "A43", "A44", "A45",
+                                   "A46", "A47", "A48", "A49"));
+
+  update_client()->RunDelayedUpdate(0);
+  EXPECT_TRUE(executed1);
+  EXPECT_FALSE(executed2);
+
+  update_client()->RunDelayedUpdate(1);
+  EXPECT_FALSE(executed2);
+
+  update_client()->RunDelayedUpdate(2);
+  EXPECT_FALSE(executed2);
+
+  update_client()->RunDelayedUpdate(3);
+  EXPECT_TRUE(executed2);
+}
+
 class UpdateServiceCanUpdateTest : public UpdateServiceTest,
                                    public ::testing::WithParamInterface<bool> {
  public: