Replace WMI call in GetHardwareInfoSync with simple registry read

During early startup a call is made to GetHardwareInfoSync which
queries WMI for the model, manufacturer and serial number.  Querying
WMI is an outbound call that can hang.  I have looked at traces
showing 100-500ms outbound hangs.  The model and manufacturer strings
can be read from the registry for windows in a known location.  This
isn't the case for the bios serial number, but it turns out nobody is
using that anyways (at least through GetHardwareInfoSync) so I am
removing it.

Bug: 1164623

Change-Id: Iabb414f42f75008730440529b055d5951ce37dcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2619419Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Commit-Queue: Chris Davis <chrdavis@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#842638}

base/system/sys_info.cc

@@ -17,7 +17,6 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
-#include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
@@ -92,16 +91,7 @@ std::string SysInfo::HardwareModelName() {
 #endif
 
 void SysInfo::GetHardwareInfo(base::OnceCallback<void(HardwareInfo)> callback) {
-#if defined(OS_WIN)
-  // On Windows the calls to GetHardwareInfoSync can take a really long time to
-  // complete as they depend on WMI, using the CONTINUE_ON_SHUTDOWN traits will
-  // prevent this task from blocking shutdown.
-  base::PostTaskAndReplyWithResult(
-      base::ThreadPool::CreateCOMSTATaskRunner(
-          {TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})
-          .get(),
-      FROM_HERE, base::BindOnce(&GetHardwareInfoSync), std::move(callback));
-#elif defined(OS_ANDROID) || defined(OS_APPLE)
+#if defined(OS_WIN) || defined(OS_ANDROID) || defined(OS_APPLE)
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {}, base::BindOnce(&GetHardwareInfoSync), std::move(callback));
 #elif defined(OS_LINUX) || defined(OS_CHROMEOS)

base/system/sys_info.h

@@ -85,16 +85,6 @@ class BASE_EXPORT SysInfo {
   struct HardwareInfo {
     std::string manufacturer;
     std::string model;
-    // On Windows, this is the BIOS serial number. Unsupported platforms will be
-    // set to an empty string.
-    // Note: validate any new usage with the privacy team.
-    // TODO(crbug.com/907518): Implement support on other platforms.
-    std::string serial_number;
-
-    bool operator==(const HardwareInfo& rhs) const {
-      return manufacturer == rhs.manufacturer && model == rhs.model &&
-             serial_number == rhs.serial_number;
-    }
   };
   // Returns via |callback| a struct containing descriptive UTF-8 strings for
   // the current machine manufacturer and model, or empty strings if the

base/system/sys_info_unittest.cc

@@ -16,12 +16,22 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
 #include "base/test/scoped_chromeos_version_info.h"
 #include "base/test/scoped_running_on_chromeos.h"
 #include "base/test/task_environment.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "base/time/time.h"
+#if defined(OS_WIN)
+#include "base/win/com_init_util.h"
+#include "base/win/scoped_bstr.h"
+#include "base/win/scoped_com_initializer.h"
+#include "base/win/scoped_variant.h"
+#include "base/win/wmi.h"
+#endif  // defined(OS_WIN)
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "testing/gtest/include/gtest/gtest-death-test.h"
@@ -207,14 +217,63 @@ TEST_F(SysInfoTest, GetHardwareInfo) {
 #endif
   EXPECT_EQ(hardware_info->manufacturer.empty(), empty_result_expected);
   EXPECT_EQ(hardware_info->model.empty(), empty_result_expected);
+}
 
 #if defined(OS_WIN)
-  EXPECT_FALSE(hardware_info->serial_number.empty());
-#else
-  // TODO(crbug.com/907518): Implement support on other platforms.
-  EXPECT_EQ(hardware_info->serial_number, std::string());
-#endif
+TEST_F(SysInfoTest, GetHardwareInfoWMIMatchRegistry) {
+  base::win::ScopedCOMInitializer com_initializer;
+  test::TaskEnvironment task_environment;
+  base::Optional<SysInfo::HardwareInfo> hardware_info;
+
+  auto callback = base::BindOnce(
+      [](base::Optional<SysInfo::HardwareInfo>* target_info,
+         SysInfo::HardwareInfo info) { *target_info = std::move(info); },
+      &hardware_info);
+  SysInfo::GetHardwareInfo(std::move(callback));
+  task_environment.RunUntilIdle();
+
+  ASSERT_TRUE(hardware_info.has_value());
+
+  Microsoft::WRL::ComPtr<IWbemServices> wmi_services;
+  EXPECT_TRUE(base::win::CreateLocalWmiConnection(true, &wmi_services));
+
+  static constexpr wchar_t query_computer_system[] =
+      L"SELECT Manufacturer,Model FROM Win32_ComputerSystem";
+
+  Microsoft::WRL::ComPtr<IEnumWbemClassObject> enumerator_computer_system;
+  HRESULT hr = wmi_services->ExecQuery(
+      base::win::ScopedBstr(L"WQL").Get(),
+      base::win::ScopedBstr(query_computer_system).Get(),
+      WBEM_FLAG_FORWARD_ONLY | WBEM_FLAG_RETURN_IMMEDIATELY, nullptr,
+      &enumerator_computer_system);
+  EXPECT_FALSE(FAILED(hr) || !enumerator_computer_system.Get());
+
+  Microsoft::WRL::ComPtr<IWbemClassObject> class_object;
+  ULONG items_returned = 0;
+  hr = enumerator_computer_system->Next(WBEM_INFINITE, 1, &class_object,
+                                        &items_returned);
+  EXPECT_FALSE(FAILED(hr) || !items_returned);
+
+  base::win::ScopedVariant manufacturerVar;
+  std::wstring manufacturer;
+  hr = class_object->Get(L"Manufacturer", 0, manufacturerVar.Receive(), nullptr,
+                         nullptr);
+  if (SUCCEEDED(hr) && manufacturerVar.type() == VT_BSTR) {
+    manufacturer.assign(V_BSTR(manufacturerVar.ptr()),
+                        ::SysStringLen(V_BSTR(manufacturerVar.ptr())));
+  }
+  base::win::ScopedVariant modelVar;
+  std::wstring model;
+  hr = class_object->Get(L"Model", 0, modelVar.Receive(), nullptr, nullptr);
+  if (SUCCEEDED(hr) && modelVar.type() == VT_BSTR) {
+    model.assign(V_BSTR(modelVar.ptr()),
+                 ::SysStringLen(V_BSTR(modelVar.ptr())));
+  }
+
+  EXPECT_TRUE(hardware_info->manufacturer == base::SysWideToUTF8(manufacturer));
+  EXPECT_TRUE(hardware_info->model == base::SysWideToUTF8(model));
 }
+#endif
 
 #if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
 

base/system/sys_info_win.cc

@@ -16,10 +16,11 @@
 #include "base/process/process_metrics.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/scoped_blocking_call.h"
+#include "base/win/registry.h"
 #include "base/win/windows_version.h"
-#include "base/win/wmi.h"
 
 namespace {
 
@@ -168,15 +169,26 @@ void SysInfo::OperatingSystemVersionNumbers(int32_t* major_version,
 
 // static
 SysInfo::HardwareInfo SysInfo::GetHardwareInfoSync() {
-  win::WmiComputerSystemInfo wmi_info = win::WmiComputerSystemInfo::Get();
+  constexpr base::char16 kSystemBiosInformationRegKey[] =
+      L"HARDWARE\\DESCRIPTION\\System\\BIOS";
 
   HardwareInfo info;
-  info.manufacturer = WideToUTF8(wmi_info.manufacturer());
-  info.model = WideToUTF8(wmi_info.model());
-  info.serial_number = WideToUTF8(wmi_info.serial_number());
-  DCHECK(IsStringUTF8(info.manufacturer));
-  DCHECK(IsStringUTF8(info.model));
-  DCHECK(IsStringUTF8(info.serial_number));
+  base::win::RegKey system_information_key;
+  if (system_information_key.Open(HKEY_LOCAL_MACHINE,
+                                  kSystemBiosInformationRegKey,
+                                  KEY_READ) == ERROR_SUCCESS) {
+    base::string16 value16;
+    if (system_information_key.ReadValue(L"SystemManufacturer", &value16) ==
+        ERROR_SUCCESS) {
+      info.manufacturer = base::SysWideToUTF8(value16);
+    }
+
+    if (system_information_key.ReadValue(L"SystemProductName", &value16) ==
+        ERROR_SUCCESS) {
+      info.model = base::SysWideToUTF8(value16);
+    }
+  }
+
   return info;
 }
 

base/win/wmi.cc

@@ -143,46 +143,11 @@ WmiComputerSystemInfo WmiComputerSystemInfo::Get() {
   if (!CreateLocalWmiConnection(true, &services))
     return info;
 
-  info.PopulateModelAndManufacturer(services);
   info.PopulateSerialNumber(services);
 
   return info;
 }
 
-void WmiComputerSystemInfo::PopulateModelAndManufacturer(
-    const ComPtr<IWbemServices>& services) {
-  static constexpr WStringPiece query_computer_system =
-      L"SELECT Manufacturer,Model FROM Win32_ComputerSystem";
-
-  ComPtr<IEnumWbemClassObject> enumerator_computer_system;
-  HRESULT hr = services->ExecQuery(
-      ScopedBstr(L"WQL").Get(), ScopedBstr(query_computer_system).Get(),
-      WBEM_FLAG_FORWARD_ONLY | WBEM_FLAG_RETURN_IMMEDIATELY, nullptr,
-      &enumerator_computer_system);
-  if (FAILED(hr) || !enumerator_computer_system.Get())
-    return;
-
-  ComPtr<IWbemClassObject> class_object;
-  ULONG items_returned = 0;
-  hr = enumerator_computer_system->Next(WBEM_INFINITE, 1, &class_object,
-                                        &items_returned);
-  if (FAILED(hr) || !items_returned)
-    return;
-
-  ScopedVariant manufacturer;
-  hr = class_object->Get(L"Manufacturer", 0, manufacturer.Receive(), nullptr,
-                         nullptr);
-  if (SUCCEEDED(hr) && manufacturer.type() == VT_BSTR) {
-    manufacturer_.assign(V_BSTR(manufacturer.ptr()),
-                         ::SysStringLen(V_BSTR(manufacturer.ptr())));
-  }
-  ScopedVariant model;
-  hr = class_object->Get(L"Model", 0, model.Receive(), nullptr, nullptr);
-  if (SUCCEEDED(hr) && model.type() == VT_BSTR) {
-    model_.assign(V_BSTR(model.ptr()), ::SysStringLen(V_BSTR(model.ptr())));
-  }
-}
-
 void WmiComputerSystemInfo::PopulateSerialNumber(
     const ComPtr<IWbemServices>& services) {
   static constexpr WStringPiece query_bios =

base/win/wmi.h

@@ -67,22 +67,19 @@ BASE_EXPORT bool WmiLaunchProcess(const std::wstring& command_line,
 // 'Win32_Bios' WMI classes; see :
 // https://docs.microsoft.com/en-us/windows/desktop/CIMWin32Prov/win32-computersystem
 // https://docs.microsoft.com/en-us/windows/desktop/CIMWin32Prov/win32-systembios
+// Note that while model and manufacturer can be obtained through WMI, it is
+// more efficient to obtain them via SysInfo::GetHardwareInfo() which uses the
+// registry.
 class BASE_EXPORT WmiComputerSystemInfo {
  public:
   static WmiComputerSystemInfo Get();
 
-  const std::wstring& manufacturer() const { return manufacturer_; }
-  const std::wstring& model() const { return model_; }
   const std::wstring& serial_number() const { return serial_number_; }
 
  private:
-  void PopulateModelAndManufacturer(
-      const Microsoft::WRL::ComPtr<IWbemServices>& services);
   void PopulateSerialNumber(
       const Microsoft::WRL::ComPtr<IWbemServices>& services);
 
-  std::wstring manufacturer_;
-  std::wstring model_;
   std::wstring serial_number_;
 };
 

base/win/wmi_unittest.cc

@@ -59,8 +59,7 @@ TEST_F(WMITest, TestLaunchProcess) {
 
 TEST_F(WMITest, TestComputerSystemInfo) {
   WmiComputerSystemInfo info = WmiComputerSystemInfo::Get();
-  EXPECT_FALSE(info.manufacturer().empty());
-  EXPECT_FALSE(info.model().empty());
+  EXPECT_FALSE(info.serial_number().empty());
 }
 
 }  // namespace win

components/enterprise/browser/controller/browser_dm_token_storage.cc

@@ -15,10 +15,8 @@
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/no_destructor.h"
-#include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/system/sys_info.h"
 #include "base/task/post_task.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -31,13 +29,6 @@ namespace {
 
 constexpr char kInvalidTokenValue[] = "INVALID_DM_TOKEN";
 
-void OnHardwarePlatformInfo(base::OnceClosure quit_closure,
-                            std::string* out,
-                            base::SysInfo::HardwareInfo info) {
-  *out = info.serial_number;
-  std::move(quit_closure).Run();
-}
-
 DMToken CreateValidToken(const std::string& dm_token) {
   DCHECK_NE(dm_token, kInvalidTokenValue);
   DCHECK(!dm_token.empty());
@@ -96,17 +87,6 @@ std::string BrowserDMTokenStorage::RetrieveClientId() {
   return client_id_;
 }
 
-std::string BrowserDMTokenStorage::RetrieveSerialNumber() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (!serial_number_) {
-    serial_number_ = InitSerialNumber();
-    DVLOG(1) << "Serial number= " << serial_number_.value();
-  }
-
-  return serial_number_.value();
-}
-
 std::string BrowserDMTokenStorage::RetrieveEnrollmentToken() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -209,20 +189,4 @@ void BrowserDMTokenStorage::SaveDMToken(const std::string& token) {
                                    std::move(reply));
 }
 
-std::string BrowserDMTokenStorage::InitSerialNumber() {
-  // GetHardwareInfo is asynchronous, but we need this synchronously. This call
-  // will only happens once, as we cache the value. This will eventually be
-  // moved earlier in Chrome's startup as it will be needed by the registration
-  // as well.
-  // TODO(crbug.com/907518): Move this earlier and make it async.
-  base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
-  std::string serial_number;
-  base::SysInfo::GetHardwareInfo(base::BindOnce(
-      &OnHardwarePlatformInfo, run_loop.QuitClosure(), &serial_number));
-
-  run_loop.Run();
-
-  return serial_number;
-}
-
 }  // namespace policy

components/enterprise/browser/controller/browser_dm_token_storage.h

@@ -69,8 +69,6 @@ class BrowserDMTokenStorage {
 
   // Returns a client ID unique to the machine.
   std::string RetrieveClientId();
-  // Returns the serial number of the machine.
-  std::string RetrieveSerialNumber();
   // Returns the enrollment token, or an empty string if there is none.
   std::string RetrieveEnrollmentToken();
   // Asynchronously stores |dm_token| and calls |callback| with a boolean to
@@ -120,10 +118,6 @@ class BrowserDMTokenStorage {
   // is called the first time the BrowserDMTokenStorage is interacted with.
   void InitIfNeeded();
 
-  // Gets the client ID and returns it. This implementation is shared by all
-  // platforms.
-  std::string InitSerialNumber();
-
   // Saves the DM token.
   void SaveDMToken(const std::string& token);
 
@@ -133,7 +127,6 @@ class BrowserDMTokenStorage {
   bool is_initialized_;
 
   std::string client_id_;
-  base::Optional<std::string> serial_number_;
   std::string enrollment_token_;
   DMToken dm_token_;
   bool should_display_error_message_on_failure_;

components/enterprise/browser/controller/browser_dm_token_storage_unittest.cc

@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/macros.h"
-#include "base/run_loop.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/enterprise/browser/controller/fake_browser_dm_token_storage.h"