Include details of a failed on-demand update check in feedback reports.

This change adds the following four fields to feedback reports on
Windows following an on-demand update check (and to chrome://system):

- update_error_code: An int value in the GoogleUpdateErrorCode enum.
- install_location: One of ("per-machine", "per-user", "unknown")
  indicating whether the browser is installed somewhere in the ordinary
  "all users" system location (e.g., "C:\Program Files (x86)\..."), the
  ordinary user-specific location (e.g.,
  "C:\Users\USER\AppData\Local\..."), or some other unexpected location.
- update_hresult: A Windows HRESULT (hex), provided by Google Update or
  COM, in case of error.
- install_result_code: An int process exit code from the installer in
  case of error if the installer was executed.

This is made possible by the introduction of UpdateState, which holds
all state relating to an update check, and GetLastUpdateState(), which
returns said state pertaining to the most recently completed update
check.

In support of this change, the Google Update update checker now uses
base::Opional<int> (rather than a magic value of -1) for the installer
process exit code to differentiate "none provided" from any specific
value.

Bug: 985209

Change-Id: Id49c65e40c0359aed767e0404afbf07030c20e07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1798345
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699243}

chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc

@@ -6,13 +6,19 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/json/json_string_value_serializer.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/system/sys_info.h"
 #include "base/task/post_task.h"
+#include "build/branding_buildflags.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings.h"
@@ -45,6 +51,9 @@
 
 #if defined(OS_WIN)
 #include "base/win/win_util.h"
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+#include "chrome/browser/google/google_update_win.h"
+#endif
 #include "ui/base/win/hidden_window.h"
 #endif
 
@@ -74,6 +83,12 @@ constexpr char kOsVersionTag[] = "OS VERSION";
 constexpr char kUsbKeyboardDetected[] = "usb_keyboard_detected";
 constexpr char kIsEnrolledToDomain[] = "enrolled_to_domain";
 constexpr char kInstallerBrandCode[] = "installer_brand_code";
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+constexpr char kUpdateErrorCode[] = "update_error_code";
+constexpr char kUpdateHresult[] = "update_hresult";
+constexpr char kInstallResultCode[] = "install_result_code";
+constexpr char kInstallLocation[] = "install_location";
+#endif
 #endif
 
 #if defined(OS_CHROMEOS)
@@ -192,6 +207,38 @@ void PopulateEntriesAsync(SystemLogsResponse* response) {
 }
 #endif  // defined(OS_CHROMEOS)
 
+#if defined(OS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+// Returns true if the path identified by |key| with the PathService is a parent
+// or ancestor of |child|.
+bool IsParentOf(int key, const base::FilePath& child) {
+  base::FilePath path;
+  return base::PathService::Get(key, &path) && path.IsParent(child);
+}
+
+// Returns a string representing the overall install location of the browser.
+// "Program Files" and "Program Files (x86)" are both considered "per-machine"
+// locations (for all users), whereas anything in a user's local app data dir is
+// considered a "per-user" location. This function returns an answer that gives,
+// in essence, the broad category of location without checking that the browser
+// is operating out of the exact expected install directory. It is interesting
+// to know via feedback reports if updates are failing with
+// CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY, which checks the exact directory,
+// yet the reported install_location is not "unknown".
+std::string DetermineInstallLocation() {
+  base::FilePath exe_path;
+
+  if (base::PathService::Get(base::FILE_EXE, &exe_path)) {
+    if (IsParentOf(base::DIR_PROGRAM_FILESX86, exe_path) ||
+        IsParentOf(base::DIR_PROGRAM_FILES, exe_path)) {
+      return "per-machine";
+    }
+    if (IsParentOf(base::DIR_LOCAL_APP_DATA, exe_path))
+      return "per-user";
+  }
+  return "unknown";
+}
+#endif  // defined(OS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+
 }  // namespace
 
 ChromeInternalLogSource::ChromeInternalLogSource()
@@ -230,6 +277,7 @@ void ChromeInternalLogSource::Fetch(SysLogsSourceCallback callback) {
   PopulateUsbKeyboardDetected(response.get());
   PopulateEnrolledToDomain(response.get());
   PopulateInstallerBrandCode(response.get());
+  PopulateLastUpdateState(response.get());
 #endif
 
   if (ProfileManager::GetLastUsedProfile()->IsChild())
@@ -410,6 +458,29 @@ void ChromeInternalLogSource::PopulateInstallerBrandCode(
   response->emplace(kInstallerBrandCode,
                     brand.empty() ? "Unknown brand code" : brand);
 }
-#endif
+
+void ChromeInternalLogSource::PopulateLastUpdateState(
+    SystemLogsResponse* response) {
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  const base::Optional<UpdateState> update_state = GetLastUpdateState();
+  if (!update_state)
+    return;  // There is nothing to include if no update check has completed.
+
+  response->emplace(kUpdateErrorCode,
+                    base::NumberToString(update_state->error_code));
+  response->emplace(kInstallLocation, DetermineInstallLocation());
+
+  if (update_state->error_code == GOOGLE_UPDATE_NO_ERROR)
+    return;  // There is nothing more to include if the last check succeeded.
+
+  response->emplace(kUpdateHresult,
+                    base::StringPrintf("0x%08lX", update_state->hresult));
+  if (update_state->installer_exit_code) {
+    response->emplace(kInstallResultCode,
+                      base::NumberToString(*update_state->installer_exit_code));
+  }
+#endif  // BUILDFLAG(GOOGLE_CHROME_BRANDING)
+}
+#endif  // defined(OS_WIN)
 
 }  // namespace system_logs

chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h

@@ -38,6 +38,7 @@ class ChromeInternalLogSource : public SystemLogsSource {
   void PopulateUsbKeyboardDetected(SystemLogsResponse* response);
   void PopulateEnrolledToDomain(SystemLogsResponse* response);
   void PopulateInstallerBrandCode(SystemLogsResponse* response);
+  void PopulateLastUpdateState(SystemLogsResponse* response);
 #endif
 
 #if defined(OS_CHROMEOS)

chrome/browser/google/google_update_win.h

@@ -7,9 +7,12 @@
 
 #include <wrl/client.h>
 
+#include <string>
+
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "google_update/google_update_idl.h"
 #include "ui/gfx/native_widget_types.h"
@@ -100,6 +103,34 @@ void BeginUpdateCheck(
     gfx::AcceleratedWidget elevation_window,
     const base::WeakPtr<UpdateCheckDelegate>& delegate);
 
+// The state from a completed update check.
+struct UpdateState {
+  UpdateState();
+  UpdateState(const UpdateState&);
+  UpdateState(UpdateState&&);
+  UpdateState& operator=(UpdateState&&);
+  ~UpdateState();
+
+  // GOOGLE_UPDATE_NO_ERROR if the last check or update succeeded; otherwise,
+  // the nature of the failure.
+  GoogleUpdateErrorCode error_code = GOOGLE_UPDATE_NO_ERROR;
+
+  // The next version available or an empty string if either no update is
+  // available or an error occurred before the new version was discovered.
+  base::string16 new_version;
+
+  // S_OK if the last check or update succeeded; otherwise, the failing error
+  // from Google Update or COM.
+  HRESULT hresult = S_OK;
+
+  // If present, the process exit code from the failed run of the installer.
+  base::Optional<int> installer_exit_code;
+};
+
+// Returns the state from the most recent completed update check or no value if
+// no such check has taken place.
+base::Optional<UpdateState> GetLastUpdateState();
+
 // A type of callback supplied by tests to provide a custom IGoogleUpdate3Web
 // implementation (see src/google_update/google_update_idl.idl).
 typedef base::Callback<HRESULT(Microsoft::WRL::ComPtr<IGoogleUpdate3Web>*)>

chrome/browser/google/google_update_win_unittest.cc

@@ -9,6 +9,7 @@
 #include <wrl/client.h>
 
 #include <memory>
+#include <string>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
@@ -684,6 +685,9 @@ TEST_P(GoogleUpdateWinTest, InvalidInstallDirectory) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code,
+            CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY);
 }
 
 // Test the case where the GoogleUpdate class can't be created for an update
@@ -698,6 +702,9 @@ TEST_P(GoogleUpdateWinTest, NoGoogleUpdateForCheck) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code,
+            GOOGLE_UPDATE_ONDEMAND_CLASS_NOT_FOUND);
 }
 
 // Test the case where the GoogleUpdate class can't be created for an upgrade.
@@ -711,6 +718,9 @@ TEST_P(GoogleUpdateWinTest, NoGoogleUpdateForUpgrade) {
   BeginUpdateCheck(std::string(), true, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code,
+            GOOGLE_UPDATE_ONDEMAND_CLASS_NOT_FOUND);
 }
 
 // Test the case where the GoogleUpdate class returns an error when an update
@@ -728,6 +738,10 @@ TEST_P(GoogleUpdateWinTest, FailUpdateCheck) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code,
+            GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->hresult, E_FAIL);
 }
 
 // Test the case where the GoogleUpdate class reports that updates are disabled
@@ -753,6 +767,8 @@ TEST_P(GoogleUpdateWinTest, UpdatesDisabledByPolicy) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_DISABLED_BY_POLICY);
 }
 
 // Test the case where the GoogleUpdate class reports that manual updates are
@@ -779,6 +795,9 @@ TEST_P(GoogleUpdateWinTest, ManualUpdatesDisabledByPolicy) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code,
+            GOOGLE_UPDATE_DISABLED_BY_POLICY_AUTO_ONLY);
 }
 
 // Test an update check where no update is available.
@@ -800,6 +819,9 @@ TEST_P(GoogleUpdateWinTest, UpdateCheckNoUpdate) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_NO_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->new_version, STRING16_LITERAL(""));
 }
 
 // Test an update check where an update is available.
@@ -821,6 +843,9 @@ TEST_P(GoogleUpdateWinTest, UpdateCheckUpdateAvailable) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_NO_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->new_version, new_version_);
 }
 
 // Test a successful upgrade.
@@ -866,6 +891,9 @@ TEST_P(GoogleUpdateWinTest, UpdateInstalled) {
   BeginUpdateCheck(std::string(), true, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_NO_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->new_version, new_version_);
 }
 
 // Test a failed upgrade where Google Update reports that the installer failed.
@@ -917,6 +945,12 @@ TEST_P(GoogleUpdateWinTest, UpdateFailed) {
   BeginUpdateCheck(std::string(), true, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_ERROR_UPDATING);
+  EXPECT_EQ(GetLastUpdateState()->new_version, new_version_);
+  EXPECT_EQ(GetLastUpdateState()->hresult, GOOPDATEINSTALL_E_INSTALLER_FAILED);
+  ASSERT_TRUE(GetLastUpdateState()->installer_exit_code);
+  EXPECT_EQ(GetLastUpdateState()->installer_exit_code.value(), kInstallerError);
 }
 
 // Test that a retry after a USING_EXTERNAL_UPDATER failure succeeds.
@@ -958,6 +992,9 @@ TEST_P(GoogleUpdateWinTest, RetryAfterExternalUpdaterError) {
   BeginUpdateCheck(std::string(), false, 0,
                    mock_update_check_delegate_.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_NO_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->new_version, STRING16_LITERAL(""));
 }
 
 TEST_P(GoogleUpdateWinTest, UpdateInstalledMultipleDelegates) {
@@ -1020,6 +1057,9 @@ TEST_P(GoogleUpdateWinTest, UpdateInstalledMultipleDelegates) {
   BeginUpdateCheck(std::string(), true, 0,
                    mock_update_check_delegate_2.AsWeakPtr());
   task_runner_->RunUntilIdle();
+  ASSERT_TRUE(GetLastUpdateState());
+  EXPECT_EQ(GetLastUpdateState()->error_code, GOOGLE_UPDATE_NO_ERROR);
+  EXPECT_EQ(GetLastUpdateState()->new_version, new_version_);
 }
 
 INSTANTIATE_TEST_SUITE_P(UserLevel, GoogleUpdateWinTest, Values(false));