Implement download progress for app install.

This CL hooks up download progress notifications from the WinHTTP network
client up to AppInstall.

BUG: 1014590.

Change-Id: I18cc3276284a221a93f2d7ca05005eb1bf3dc71d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216035Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773284}

chrome/updater/win/BUILD.gn

@@ -149,6 +149,7 @@ source_set("install_app") {
   ]
 
   deps = [
+    ":base",
     ":install_progress_observer",
     ":lib",
     "//base",
@@ -165,7 +166,6 @@ source_set("install_app") {
   cflags_cc = [ "-Wno-missing-braces" ]
 }
 
-# Tests built into Chrome's unit_tests.exe.
 source_set("updater_tests") {
   testonly = true
 

chrome/updater/win/install_app.cc

@@ -45,6 +45,7 @@
 #include "chrome/updater/win/ui/resources/resources.grh"
 #include "chrome/updater/win/ui/splash_screen.h"
 #include "chrome/updater/win/ui/util.h"
+#include "chrome/updater/win/util.h"
 #include "components/prefs/pref_service.h"
 #include "components/update_client/configurator.h"
 #include "components/update_client/crx_update_item.h"
@@ -569,11 +570,13 @@ void InstallAppController::StateChange(
           base::ASCIIToUTF16(crx_update_item.next_version.GetString()));
       break;
 
-    case update_client::ComponentState::kDownloading:
-      // TODO(sorin): handle progress and time remaining.
-      // https://crbug.com/1014590
-      install_progress_observer_ipc_->OnDownloading(app_id, app_name_, -1, 0);
-      break;
+    case update_client::ComponentState::kDownloading: {
+      // TODO(sorin): handle time remaining https://crbug.com/1014590.
+      const auto pos = GetDownloadProgress(crx_update_item.downloaded_bytes,
+                                           crx_update_item.total_bytes);
+      install_progress_observer_ipc_->OnDownloading(app_id, app_name_, -1,
+                                                    pos != -1 ? pos : 0);
+    } break;
 
     case update_client::ComponentState::kUpdating: {
       // TODO(sorin): handle the install cancellation.

chrome/updater/win/net/network_winhttp.cc

@@ -61,32 +61,32 @@ void NetworkFetcherWinHTTP::Close() {
 }
 
 std::string NetworkFetcherWinHTTP::GetResponseBody() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return post_response_body_;
 }
 
 HRESULT NetworkFetcherWinHTTP::GetNetError() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return net_error_;
 }
 
 std::string NetworkFetcherWinHTTP::GetHeaderETag() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return etag_;
 }
 
 int64_t NetworkFetcherWinHTTP::GetXHeaderRetryAfterSec() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return xheader_retry_after_sec_;
 }
 
 base::FilePath NetworkFetcherWinHTTP::GetFilePath() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return file_path_;
 }
 
 int64_t NetworkFetcherWinHTTP::GetContentSize() const {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return content_size_;
 }
 
@@ -97,7 +97,7 @@ void NetworkFetcherWinHTTP::PostRequest(
     FetchStartedCallback fetch_started_callback,
     FetchProgressCallback fetch_progress_callback,
     FetchCompleteCallback fetch_complete_callback) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   url_ = url;
   fetch_started_callback_ = std::move(fetch_started_callback);
@@ -109,8 +109,8 @@ void NetworkFetcherWinHTTP::PostRequest(
 
   verb_ = L"POST";
   content_type_ = L"Content-Type: application/json\r\n";
-  write_data_callback_ = base::BindRepeating(
-      &NetworkFetcherWinHTTP::WriteDataToMemory, base::Unretained(this));
+  write_data_callback_ =
+      base::BindRepeating(&NetworkFetcherWinHTTP::WriteDataToMemory, this);
 
   net_error_ = BeginFetch(post_data, post_additional_headers);
   if (FAILED(net_error_))
@@ -123,7 +123,7 @@ void NetworkFetcherWinHTTP::DownloadToFile(
     FetchStartedCallback fetch_started_callback,
     FetchProgressCallback fetch_progress_callback,
     FetchCompleteCallback fetch_complete_callback) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   url_ = url;
   file_path_ = file_path;
@@ -135,8 +135,8 @@ void NetworkFetcherWinHTTP::DownloadToFile(
   CrackUrl(url, &is_https_, &host_, &port_, &path_for_request_);
 
   verb_ = L"GET";
-  write_data_callback_ = base::BindRepeating(
-      &NetworkFetcherWinHTTP::WriteDataToFile, base::Unretained(this));
+  write_data_callback_ =
+      base::BindRepeating(&NetworkFetcherWinHTTP::WriteDataToFile, this);
 
   net_error_ = BeginFetch({}, {});
   if (FAILED(net_error_))
@@ -146,7 +146,7 @@ void NetworkFetcherWinHTTP::DownloadToFile(
 HRESULT NetworkFetcherWinHTTP::BeginFetch(
     const std::string& data,
     const base::flat_map<std::string, std::string>& additional_headers) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   connect_handle_ = Connect();
   if (!connect_handle_.get())
     return HRESULTFromLastError();
@@ -195,13 +195,13 @@ HRESULT NetworkFetcherWinHTTP::BeginFetch(
 }
 
 scoped_hinternet NetworkFetcherWinHTTP::Connect() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return scoped_hinternet(::WinHttpConnect(
       session_handle_, base::SysUTF8ToWide(host_).c_str(), port_, 0));
 }
 
 scoped_hinternet NetworkFetcherWinHTTP::OpenRequest() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   uint32_t flags = WINHTTP_FLAG_REFRESH;
   if (is_https_)
     flags |= WINHTTP_FLAG_SECURE;
@@ -212,7 +212,7 @@ scoped_hinternet NetworkFetcherWinHTTP::OpenRequest() {
 }
 
 HRESULT NetworkFetcherWinHTTP::SendRequest(const std::string& data) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   VLOG(2) << data;
 
@@ -229,7 +229,7 @@ HRESULT NetworkFetcherWinHTTP::SendRequest(const std::string& data) {
 }
 
 void NetworkFetcherWinHTTP::SendRequestComplete() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   base::string16 all;
   QueryHeadersString(
@@ -244,14 +244,14 @@ void NetworkFetcherWinHTTP::SendRequestComplete() {
 }
 
 HRESULT NetworkFetcherWinHTTP::ReceiveResponse() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!::WinHttpReceiveResponse(request_handle_.get(), nullptr))
     return HRESULTFromLastError();
   return S_OK;
 }
 
 void NetworkFetcherWinHTTP::ReceiveResponseComplete() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   base::string16 all;
   QueryHeadersString(request_handle_.get(), WINHTTP_QUERY_RAW_HEADERS_CRLF,
@@ -298,7 +298,7 @@ void NetworkFetcherWinHTTP::ReceiveResponseComplete() {
 }
 
 HRESULT NetworkFetcherWinHTTP::QueryDataAvailable() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!::WinHttpQueryDataAvailable(request_handle_.get(), nullptr))
     return HRESULTFromLastError();
   return S_OK;
@@ -306,17 +306,20 @@ HRESULT NetworkFetcherWinHTTP::QueryDataAvailable() {
 
 void NetworkFetcherWinHTTP::QueryDataAvailableComplete(
     size_t num_bytes_available) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   net_error_ = ReadData(num_bytes_available);
   if (FAILED(net_error_))
     std::move(fetch_complete_callback_).Run();
 }
 
-HRESULT NetworkFetcherWinHTTP::ReadData(size_t num_bytes_available) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+// TODO(crbug.com/1087944): ignore |num_bytes_available| for now since it
+// may contain invalid data. Use a fixed buffer size, larger than the internal
+// WinHTTP buffer size (8K), according to the documentation for WinHttpReadData.
+HRESULT NetworkFetcherWinHTTP::ReadData(size_t /*num_bytes_available*/) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  const int num_bytes_to_read = base::saturated_cast<int>(num_bytes_available);
-  read_buffer_.resize(num_bytes_to_read);
+  constexpr size_t kNumBytesToRead = 0x4000;  // 16KiB.
+  read_buffer_.resize(kNumBytesToRead);
 
   if (!::WinHttpReadData(request_handle_.get(), &read_buffer_.front(),
                          read_buffer_.size(), nullptr)) {
@@ -326,33 +329,29 @@ HRESULT NetworkFetcherWinHTTP::ReadData(size_t num_bytes_available) {
 }
 
 void NetworkFetcherWinHTTP::ReadDataComplete(size_t num_bytes_read) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-
-  fetch_progress_callback_.Run(base::saturated_cast<int64_t>(num_bytes_read));
-
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   read_buffer_.resize(num_bytes_read);
   write_data_callback_.Run();
 }
 
 void NetworkFetcherWinHTTP::RequestError(const WINHTTP_ASYNC_RESULT* result) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   net_error_ = HRESULTFromUpdaterError(result->dwError);
   std::move(fetch_complete_callback_).Run();
 }
 
 void NetworkFetcherWinHTTP::WriteDataToFile() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   constexpr base::TaskTraits kTaskTraits = {
       base::MayBlock(), base::TaskPriority::BEST_EFFORT,
       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};
-
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, kTaskTraits,
-      base::BindOnce(&NetworkFetcherWinHTTP::WriteDataToFileBlocking,
-                     base::Unretained(this)),
-      base::BindOnce(&NetworkFetcherWinHTTP::WriteDataToFileComplete,
-                     base::Unretained(this)));
+      base::BindOnce(&NetworkFetcherWinHTTP::WriteDataToFileBlocking, this),
+      base::BindOnce(&NetworkFetcherWinHTTP::WriteDataToFileComplete, this));
 }
 
+// Returns true if EOF is reached.
 bool NetworkFetcherWinHTTP::WriteDataToFileBlocking() {
   if (read_buffer_.empty()) {
     file_.Close();
@@ -384,7 +383,9 @@ bool NetworkFetcherWinHTTP::WriteDataToFileBlocking() {
 }
 
 void NetworkFetcherWinHTTP::WriteDataToFileComplete(bool is_eof) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  fetch_progress_callback_.Run(base::saturated_cast<int64_t>(content_size_));
 
   if (is_eof || FAILED(net_error_)) {
     std::move(fetch_complete_callback_).Run();
@@ -397,7 +398,7 @@ void NetworkFetcherWinHTTP::WriteDataToFileComplete(bool is_eof) {
 }
 
 void NetworkFetcherWinHTTP::WriteDataToMemory() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (read_buffer_.empty()) {
     VLOG(2) << post_response_body_;
@@ -408,6 +409,7 @@ void NetworkFetcherWinHTTP::WriteDataToMemory() {
 
   post_response_body_.append(read_buffer_.begin(), read_buffer_.end());
   content_size_ += read_buffer_.size();
+  fetch_progress_callback_.Run(base::saturated_cast<int64_t>(content_size_));
 
   net_error_ = QueryDataAvailable();
   if (FAILED(net_error_))
@@ -433,7 +435,7 @@ void NetworkFetcherWinHTTP::StatusCallback(HINTERNET handle,
                                            uint32_t status,
                                            void* info,
                                            uint32_t info_len) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   base::StringPiece status_string;
   base::string16 info_string;
   switch (status) {

chrome/updater/win/net/network_winhttp.h

@@ -17,8 +17,8 @@
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/sequence_checker.h"
 #include "base/strings/string_piece_forward.h"
-#include "base/threading/thread_checker.h"
 #include "chrome/updater/win/net/scoped_hinternet.h"
 #include "components/update_client/network.h"
 #include "url/gurl.h"
@@ -30,8 +30,7 @@ class SingleThreadTaskRunner;
 namespace updater {
 
 // Implements a network fetcher in terms of WinHTTP. The class is ref-counted
-// as it is accessed from both the main thread and the worker threads in
-// WinHTTP.
+// as it is accessed from the main thread and the worker threads in WinHTTP.
 class NetworkFetcherWinHTTP
     : public base::RefCountedThreadSafe<NetworkFetcherWinHTTP> {
  public:
@@ -53,6 +52,10 @@ class NetworkFetcherWinHTTP
       FetchStartedCallback fetch_started_callback,
       FetchProgressCallback fetch_progress_callback,
       FetchCompleteCallback fetch_complete_callback);
+
+  // Downloads the content of the |url| to a file identified by |file_path|.
+  // The content is written to the file as it is being retrieved from the
+  // network.
   void DownloadToFile(const GURL& url,
                       const base::FilePath& file_path,
                       FetchStartedCallback fetch_started_callback,
@@ -64,6 +67,9 @@ class NetworkFetcherWinHTTP
   std::string GetHeaderETag() const;
   int64_t GetXHeaderRetryAfterSec() const;
   base::FilePath GetFilePath() const;
+
+  // Returns the number of bytes retrieved from the network. This may be
+  // different than the content length if an error occurred.
   int64_t GetContentSize() const;
 
  private:
@@ -105,7 +111,7 @@ class NetworkFetcherWinHTTP
   bool WriteDataToFileBlocking();
   void WriteDataToFileComplete(bool is_eof);
 
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
 
   const HINTERNET& session_handle_;  // Owned by NetworkFetcherWinHTTPFactory.

chrome/updater/win/util.cc

@@ -12,6 +12,7 @@
 #include "base/files/file_path.h"
 #include "base/guid.h"
 #include "base/logging.h"
+#include "base/numerics/ranges.h"
 #include "base/process/process_iterator.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string16.h"
@@ -312,4 +313,12 @@ base::string16 GetRegistryKeyClientStateUpdater() {
   return base::ASCIIToUTF16(base::StrCat({CLIENT_STATE_KEY, kUpdaterAppId}));
 }
 
+int GetDownloadProgress(int64_t downloaded_bytes, int64_t total_bytes) {
+  if (downloaded_bytes == -1 || total_bytes == -1 || total_bytes == 0)
+    return -1;
+  DCHECK_LE(downloaded_bytes, total_bytes);
+  return 100 *
+         base::ClampToRange(double{downloaded_bytes} / total_bytes, 0.0, 1.0);
+}
+
 }  // namespace updater

chrome/updater/win/util.h

@@ -7,6 +7,8 @@
 
 #include <winerror.h>
 
+#include <stdint.h>
+
 #include <string>
 
 #include "base/strings/string16.h"
@@ -98,6 +100,10 @@ base::string16 GetRegistryKeyClientsUpdater();
 // subkey. The path does not include the registry root hive prefix.
 base::string16 GetRegistryKeyClientStateUpdater();
 
+// Returns a value in the [0, 100] range or -1 if the progress could not
+// be computed.
+int GetDownloadProgress(int64_t downloaded_bytes, int64_t total_bytes);
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_WIN_UTIL_H_

chrome/updater/win/util_unittest.cc

@@ -17,4 +17,14 @@ TEST(UpdaterTestUtil, HRESULTFromLastError) {
   EXPECT_EQ(E_FAIL, HRESULTFromLastError());
 }
 
+TEST(UpdaterTestUtil, GetDownloadProgress) {
+  EXPECT_EQ(GetDownloadProgress(0, 50), 0);
+  EXPECT_EQ(GetDownloadProgress(12, 50), 24);
+  EXPECT_EQ(GetDownloadProgress(25, 50), 50);
+  EXPECT_EQ(GetDownloadProgress(50, 50), 100);
+  EXPECT_EQ(GetDownloadProgress(50, 50), 100);
+  EXPECT_EQ(GetDownloadProgress(0, -1), -1);
+  EXPECT_EQ(GetDownloadProgress(-1, -1), -1);
+  EXPECT_EQ(GetDownloadProgress(50, 0), -1);
+}
 }  // namespace updater