Track bytes received across multiple update files

When downloading the packages that comprise a multi-package or multi-app
update, the UpdateAttempter receives BytesReceived() callbacks with
bytes_received resetting to 0 for each file.  This causes the progress
calculations to be incorrect.

This change tracks the total of the previously downloaded packages
within the DownloadAction, so that it properly tracks.  Resumed
downloads will jump ahead over skipped data, when the payload is
incremented.

Bug:  65451460
Tests: Added unit tests to directly test the accumulation and the the
transition from the previous state to UpdateStatus::DOWNLOADING when the
first bytes are received.

Change-Id: I3b540df16b9a664b09f53ee3ec962e2cbc8adf1b
(cherry picked from commit d6f869dbd9952be8a926e80c4f1e172845ab8d5f)

7 files changed, 224 insertions, 10 deletions

diff --git a/mock_file_writer.h b/mock_file_writer.h
index 72d6a863..26cd45d0 100644
--- a/mock_file_writer.h
+++ b/mock_file_writer.h
@@ -24,7 +24,8 @@ namespace chromeos_update_engine {
 
 class MockFileWriter : public FileWriter {
  public:
-  MOCK_METHOD2(Write, ssize_t(const void* bytes, size_t count));
+  MOCK_METHOD2(Write, bool(const void* bytes, size_t count));
+  MOCK_METHOD3(Write, bool(const void* bytes, size_t count, ErrorCode* error));
   MOCK_METHOD0(Close, int());
 };
 
diff --git a/mock_service_observer.h b/mock_service_observer.h
new file mode 100644
index 00000000..032f55a0
--- /dev/null
+++ b/mock_service_observer.h
@@ -0,0 +1,38 @@
+//
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#ifndef UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
+#define UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
+
+#include <gmock/gmock.h>
+#include "update_engine/service_observer_interface.h"
+
+namespace chromeos_update_engine {
+
+class MockServiceObserver : public ServiceObserverInterface {
+ public:
+  MOCK_METHOD5(SendStatusUpdate,
+               void(int64_t last_checked_time,
+                    double progress,
+                    update_engine::UpdateStatus status,
+                    const std::string& new_version,
+                    int64_t new_size));
+  MOCK_METHOD1(SendPayloadApplicationComplete, void(ErrorCode error_code));
+};
+
+}  // namespace chromeos_update_engine
+
+#endif  // UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index c3a50161..cd25962b 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -173,6 +173,7 @@ void DownloadAction::PerformAction() {
   install_plan_.Dump();
 
   bytes_received_ = 0;
+  bytes_received_previous_payloads_ = 0;
   bytes_total_ = 0;
   for (const auto& payload : install_plan_.payloads)
     bytes_total_ += payload.size;
@@ -317,8 +318,10 @@ void DownloadAction::ReceivedBytes(HttpFetcher* fetcher,
   }
 
   bytes_received_ += length;
+  uint64_t bytes_downloaded_total =
+      bytes_received_previous_payloads_ + bytes_received_;
   if (delegate_ && download_active_) {
-    delegate_->BytesReceived(length, bytes_received_, bytes_total_);
+    delegate_->BytesReceived(length, bytes_downloaded_total, bytes_total_);
   }
   if (writer_ && !writer_->Write(bytes, length, &code_)) {
     if (code_ != ErrorCode::kSuccess) {
@@ -349,13 +352,16 @@ void DownloadAction::ReceivedBytes(HttpFetcher* fetcher,
 void DownloadAction::TransferComplete(HttpFetcher* fetcher, bool successful) {
   if (writer_) {
     LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
-    writer_ = nullptr;
+    if (delta_performer_.get() == writer_) {
+      // no delta_performer_ in tests, so leave the test writer in place
+      writer_ = nullptr;
+    }
   }
   download_active_ = false;
   ErrorCode code =
       successful ? ErrorCode::kSuccess : ErrorCode::kDownloadTransferError;
-  if (code == ErrorCode::kSuccess && delta_performer_.get()) {
-    if (!payload_->already_applied)
+  if (code == ErrorCode::kSuccess) {
+    if (delta_performer_ && !payload_->already_applied)
       code = delta_performer_->VerifyPayload(payload_->hash, payload_->size);
     if (code != ErrorCode::kSuccess) {
       LOG(ERROR) << "Download of " << install_plan_.download_url
@@ -365,10 +371,12 @@ void DownloadAction::TransferComplete(HttpFetcher* fetcher, bool successful) {
         CloseP2PSharingFd(true);
     } else if (payload_ < &install_plan_.payloads.back() &&
                system_state_->payload_state()->NextPayload()) {
+      LOG(INFO) << "Incrementing to next payload";
       // No need to reset if this payload was already applied.
-      if (!payload_->already_applied)
+      if (delta_performer_ && !payload_->already_applied)
         DeltaPerformer::ResetUpdateProgress(prefs_, false);
       // Start downloading next payload.
+      bytes_received_previous_payloads_ += payload_->size;
       payload_++;
       install_plan_.download_url =
           system_state_->payload_state()->GetCurrentUrl();
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index d0e60008..786db203 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -166,7 +166,8 @@ class DownloadAction : public InstallPlanAction,
 
   // For reporting status to outsiders
   DownloadActionDelegate* delegate_;
-  uint64_t bytes_received_{0};
+  uint64_t bytes_received_{0};  // per file/range
+  uint64_t bytes_received_previous_payloads_{0};
   uint64_t bytes_total_{0};
   bool download_active_{false};
 
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 7d3ac6c3..f42b1d8e 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -41,6 +41,7 @@
 #include "update_engine/common/utils.h"
 #include "update_engine/fake_p2p_manager_configuration.h"
 #include "update_engine/fake_system_state.h"
+#include "update_engine/mock_file_writer.h"
 #include "update_engine/payload_consumer/mock_download_action.h"
 #include "update_engine/update_manager/fake_update_manager.h"
 
@@ -56,6 +57,7 @@ using test_utils::ScopedTempFile;
 using testing::AtLeast;
 using testing::InSequence;
 using testing::Return;
+using testing::SetArgPointee;
 using testing::_;
 
 class DownloadActionTest : public ::testing::Test { };
@@ -242,6 +244,92 @@ TEST(DownloadActionTest, NoDownloadDelegateTest) {
                false);  // use_download_delegate
 }
 
+TEST(DownloadActionTest, MultiPayloadProgressTest) {
+  std::vector<brillo::Blob> payload_datas;
+  // the first payload must be the largest, as it's the actual payload used by
+  // the MockHttpFetcher for all downloaded data.
+  payload_datas.emplace_back(4 * kMockHttpFetcherChunkSize + 256);
+  payload_datas.emplace_back(2 * kMockHttpFetcherChunkSize);
+  brillo::FakeMessageLoop loop(nullptr);
+  loop.SetAsCurrent();
+  FakeSystemState fake_system_state;
+  EXPECT_CALL(*fake_system_state.mock_payload_state(), NextPayload())
+      .WillOnce(Return(true));
+
+  MockFileWriter mock_file_writer;
+  EXPECT_CALL(mock_file_writer, Close()).WillRepeatedly(Return(0));
+  EXPECT_CALL(mock_file_writer, Write(_, _, _))
+      .WillRepeatedly(
+          DoAll(SetArgPointee<2>(ErrorCode::kSuccess), Return(true)));
+
+  InstallPlan install_plan;
+  uint64_t total_expected_download_size{0};
+  for (const auto& data : payload_datas) {
+    uint64_t size = data.size();
+    install_plan.payloads.push_back(
+        {.size = size, .type = InstallPayloadType::kFull});
+    total_expected_download_size += size;
+  }
+  ObjectFeederAction<InstallPlan> feeder_action;
+  feeder_action.set_obj(install_plan);
+  MockPrefs prefs;
+  MockHttpFetcher* http_fetcher = new MockHttpFetcher(
+      payload_datas[0].data(), payload_datas[0].size(), nullptr);
+  // takes ownership of passed in HttpFetcher
+  DownloadAction download_action(&prefs,
+                                 fake_system_state.boot_control(),
+                                 fake_system_state.hardware(),
+                                 &fake_system_state,
+                                 http_fetcher);
+  download_action.SetTestFileWriter(&mock_file_writer);
+  BondActions(&feeder_action, &download_action);
+  MockDownloadActionDelegate download_delegate;
+  {
+    InSequence s;
+    download_action.set_delegate(&download_delegate);
+    // these are hand-computed based on the payloads specified above
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              kMockHttpFetcherChunkSize,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              kMockHttpFetcherChunkSize * 2,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              kMockHttpFetcherChunkSize * 3,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              kMockHttpFetcherChunkSize * 4,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(256,
+                              kMockHttpFetcherChunkSize * 4 + 256,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              kMockHttpFetcherChunkSize * 5 + 256,
+                              total_expected_download_size));
+    EXPECT_CALL(download_delegate,
+                BytesReceived(kMockHttpFetcherChunkSize,
+                              total_expected_download_size,
+                              total_expected_download_size));
+  }
+  ActionProcessor processor;
+  processor.EnqueueAction(&feeder_action);
+  processor.EnqueueAction(&download_action);
+
+  loop.PostTask(
+      FROM_HERE,
+      base::Bind(
+          [](ActionProcessor* processor) { processor->StartProcessing(); },
+          base::Unretained(&processor)));
+  loop.Run();
+  EXPECT_FALSE(loop.PendingTasks());
+}
+
 namespace {
 class TerminateEarlyTestProcessorDelegate : public ActionProcessorDelegate {
  public:
diff --git a/update_attempter.h b/update_attempter.h
index b4e2f60b..9dd844dd 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -252,17 +252,20 @@ class UpdateAttempter : public ActionProcessorDelegate,
   FRIEND_TEST(UpdateAttempterTest, ActionCompletedDownloadTest);
   FRIEND_TEST(UpdateAttempterTest, ActionCompletedErrorTest);
   FRIEND_TEST(UpdateAttempterTest, ActionCompletedOmahaRequestTest);
+  FRIEND_TEST(UpdateAttempterTest, BootTimeInUpdateMarkerFile);
+  FRIEND_TEST(UpdateAttempterTest, BroadcastCompleteDownloadTest);
+  FRIEND_TEST(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest);
   FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventTest);
   FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventResumedTest);
   FRIEND_TEST(UpdateAttempterTest, DisableDeltaUpdateIfNeededTest);
+  FRIEND_TEST(UpdateAttempterTest, DownloadProgressAccumulationTest);
   FRIEND_TEST(UpdateAttempterTest, MarkDeltaUpdateFailureTest);
   FRIEND_TEST(UpdateAttempterTest, PingOmahaTest);
+  FRIEND_TEST(UpdateAttempterTest, ReportDailyMetrics);
   FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionNoEventTest);
   FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionTest);
-  FRIEND_TEST(UpdateAttempterTest, UpdateTest);
-  FRIEND_TEST(UpdateAttempterTest, ReportDailyMetrics);
-  FRIEND_TEST(UpdateAttempterTest, BootTimeInUpdateMarkerFile);
   FRIEND_TEST(UpdateAttempterTest, TargetVersionPrefixSetAndReset);
+  FRIEND_TEST(UpdateAttempterTest, UpdateTest);
 
   // CertificateChecker::Observer method.
   // Report metrics about the certificate being checked.
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 4ebc85c8..911f6640 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -48,6 +48,7 @@
 #include "update_engine/fake_system_state.h"
 #include "update_engine/mock_p2p_manager.h"
 #include "update_engine/mock_payload_state.h"
+#include "update_engine/mock_service_observer.h"
 #include "update_engine/payload_consumer/filesystem_verifier_action.h"
 #include "update_engine/payload_consumer/install_plan.h"
 #include "update_engine/payload_consumer/payload_constants.h"
@@ -219,6 +220,7 @@ TEST_F(UpdateAttempterTest, ActionCompletedDownloadTest) {
   EXPECT_CALL(*prefs_, GetInt64(kPrefsDeltaUpdateFailures, _)).Times(0);
   attempter_.ActionCompleted(nullptr, &action, ErrorCode::kSuccess);
   EXPECT_EQ(UpdateStatus::FINALIZING, attempter_.status());
+  EXPECT_EQ(0.0, attempter_.download_progress_);
   ASSERT_EQ(nullptr, attempter_.error_event_.get());
 }
 
@@ -232,6 +234,79 @@ TEST_F(UpdateAttempterTest, ActionCompletedErrorTest) {
   ASSERT_NE(nullptr, attempter_.error_event_.get());
 }
 
+TEST_F(UpdateAttempterTest, DownloadProgressAccumulationTest) {
+  // Simple test case, where all the values match (nothing was skipped)
+  uint64_t bytes_progressed_1 = 1024 * 1024;  // 1MB
+  uint64_t bytes_progressed_2 = 1024 * 1024;  // 1MB
+  uint64_t bytes_received_1 = bytes_progressed_1;
+  uint64_t bytes_received_2 = bytes_received_1 + bytes_progressed_2;
+  uint64_t bytes_total = 20 * 1024 * 1024;  // 20MB
+
+  double progress_1 =
+      static_cast<double>(bytes_received_1) / static_cast<double>(bytes_total);
+  double progress_2 =
+      static_cast<double>(bytes_received_2) / static_cast<double>(bytes_total);
+
+  EXPECT_EQ(0.0, attempter_.download_progress_);
+  // This is set via inspecting the InstallPlan payloads when the
+  // OmahaResponseAction is completed
+  attempter_.new_payload_size_ = bytes_total;
+  NiceMock<MockServiceObserver> observer;
+  EXPECT_CALL(observer,
+              SendStatusUpdate(
+                  _, progress_1, UpdateStatus::DOWNLOADING, _, bytes_total));
+  EXPECT_CALL(observer,
+              SendStatusUpdate(
+                  _, progress_2, UpdateStatus::DOWNLOADING, _, bytes_total));
+  attempter_.AddObserver(&observer);
+  attempter_.BytesReceived(bytes_progressed_1, bytes_received_1, bytes_total);
+  EXPECT_EQ(progress_1, attempter_.download_progress_);
+  // This iteration validates that a later set of updates to the variables are
+  // properly handled (so that |getStatus()| will return the same progress info
+  // as the callback is receiving.
+  attempter_.BytesReceived(bytes_progressed_2, bytes_received_2, bytes_total);
+  EXPECT_EQ(progress_2, attempter_.download_progress_);
+}
+
+TEST_F(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest) {
+  // The transition into UpdateStatus::DOWNLOADING happens when the
+  // first bytes are received.
+  uint64_t bytes_progressed = 1024 * 1024;    // 1MB
+  uint64_t bytes_received = 2 * 1024 * 1024;  // 2MB
+  uint64_t bytes_total = 20 * 1024 * 1024;    // 300MB
+  attempter_.status_ = UpdateStatus::CHECKING_FOR_UPDATE;
+  // This is set via inspecting the InstallPlan payloads when the
+  // OmahaResponseAction is completed
+  attempter_.new_payload_size_ = bytes_total;
+  EXPECT_EQ(0.0, attempter_.download_progress_);
+  NiceMock<MockServiceObserver> observer;
+  EXPECT_CALL(
+      observer,
+      SendStatusUpdate(_, _, UpdateStatus::DOWNLOADING, _, bytes_total));
+  attempter_.AddObserver(&observer);
+  attempter_.BytesReceived(bytes_progressed, bytes_received, bytes_total);
+  EXPECT_EQ(UpdateStatus::DOWNLOADING, attempter_.status_);
+}
+
+TEST_F(UpdateAttempterTest, BroadcastCompleteDownloadTest) {
+  // There is a special case to ensure that at 100% downloaded,
+  // download_progress_ is updated and that value broadcast. This test confirms
+  // that.
+  uint64_t bytes_progressed = 0;              // ignored
+  uint64_t bytes_received = 5 * 1024 * 1024;  // ignored
+  uint64_t bytes_total = 5 * 1024 * 1024;     // 300MB
+  attempter_.status_ = UpdateStatus::DOWNLOADING;
+  attempter_.new_payload_size_ = bytes_total;
+  EXPECT_EQ(0.0, attempter_.download_progress_);
+  NiceMock<MockServiceObserver> observer;
+  EXPECT_CALL(
+      observer,
+      SendStatusUpdate(_, 1.0, UpdateStatus::DOWNLOADING, _, bytes_total));
+  attempter_.AddObserver(&observer);
+  attempter_.BytesReceived(bytes_progressed, bytes_received, bytes_total);
+  EXPECT_EQ(1.0, attempter_.download_progress_);
+}
+
 TEST_F(UpdateAttempterTest, ActionCompletedOmahaRequestTest) {
   unique_ptr<MockHttpFetcher> fetcher(new MockHttpFetcher("", 0, nullptr));
   fetcher->FailTransfer(500);  // Sets the HTTP response code.