[Windows] Do not include string terminator in GetWindowText

WindowEnumerator::GetWindowText was including the trailing string
terminator in the body of the returned string. This CL fixes that as
well as makes several other changes:

- WindowEnumeratorTest.EnumerateTopLevelWindows had the same bug in its
  independent implementation of GetWindowText. It is also fixed.
- WindowEnumerator has been whittled down to a single function, so
  there's no need for a class anymore. It's now simply
  base::win::EnumerateChildWindows, which is a little easier to use.
- base/win/window_enumerator.h no longer includes windows.h.
- GetWindowText has been renamed to GetWindowTextString so that it
  doesn't accidentally get renamed to GetWindowTextW by windows.h.
- GetWindowTextString also handles the case where the actual text is
  fewer characters than reported by GetWindowTextLength. MSDN explains a
  few cases where this could happen.

Bug: 1482637
Change-Id: I8cb9b5706fb43bdb10e79afc9c832d9a4e5ee6af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4868390
Auto-Submit: Greg Thompson <[email protected]>
Reviewed-by: S Ganesh <[email protected]>
Reviewed-by: Avi Drissman <[email protected]>
Commit-Queue: Avi Drissman <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1197669}

base/win/window_enumerator.cc

@@ -12,29 +12,28 @@
 
 namespace base::win {
 
-WindowEnumerator::WindowEnumerator(
-    HWND parent,
-    base::RepeatingCallback<bool(HWND hwnd)> filter)
-    : parent_(parent), filter_(filter) {}
-WindowEnumerator::~WindowEnumerator() = default;
-
-void WindowEnumerator::Run() const {
-  ::EnumChildWindows(parent_, &OnWindowProc, reinterpret_cast<LPARAM>(this));
+namespace {
+
+BOOL CALLBACK OnWindowProc(HWND hwnd, LPARAM lparam) {
+  return !reinterpret_cast<WindowEnumeratorCallback*>(lparam)->Run(hwnd);
 }
 
-// static
-bool WindowEnumerator::IsTopmostWindow(HWND hwnd) {
+}  // namespace
+
+void EnumerateChildWindows(HWND parent, WindowEnumeratorCallback filter) {
+  ::EnumChildWindows(parent, &OnWindowProc, reinterpret_cast<LPARAM>(&filter));
+}
+
+bool IsTopmostWindow(HWND hwnd) {
   return ::GetWindowLong(hwnd, GWL_EXSTYLE) & WS_EX_TOPMOST;
 }
 
-// static
-bool WindowEnumerator::IsSystemDialog(HWND hwnd) {
-  constexpr wchar_t kSystemDialogClass[] = L"#32770";
+bool IsSystemDialog(HWND hwnd) {
+  static constexpr wchar_t kSystemDialogClass[] = L"#32770";
   return GetWindowClass(hwnd) == kSystemDialogClass;
 }
 
-// static
-bool WindowEnumerator::IsShellWindow(HWND hwnd) {
+bool IsShellWindow(HWND hwnd) {
   const std::wstring class_name = GetWindowClass(hwnd);
 
   // 'Button' is the start button, 'Shell_TrayWnd' the taskbar, and
@@ -43,36 +42,33 @@ bool WindowEnumerator::IsShellWindow(HWND hwnd) {
          class_name == L"Shell_SecondaryTrayWnd";
 }
 
-// static
-std::wstring WindowEnumerator::GetWindowClass(HWND hwnd) {
+std::wstring GetWindowClass(HWND hwnd) {
   constexpr int kMaxWindowClassNameLength = 256;
   std::wstring window_class(kMaxWindowClassNameLength, L'\0');
   const int name_len =
-      ::GetClassName(hwnd, &window_class[0], kMaxWindowClassNameLength);
+      ::GetClassName(hwnd, window_class.data(), kMaxWindowClassNameLength);
   if (name_len <= 0 || name_len > kMaxWindowClassNameLength) {
     return {};
   }
   window_class.resize(static_cast<size_t>(name_len));
   return window_class;
 }
 
-// static
-std::wstring WindowEnumerator::GetWindowText(HWND hwnd) {
-  int num_chars = ::GetWindowTextLength(hwnd);
-  if (!num_chars) {
+std::wstring GetWindowTextString(HWND hwnd) {
+  auto num_chars = ::GetWindowTextLength(hwnd);
+  if (num_chars <= 0) {
     return {};
   }
-  std::wstring text(static_cast<size_t>(++num_chars), L'\0');
-  return ::GetWindowText(hwnd, &text[0], num_chars) ? text : std::wstring();
-}
-
-bool WindowEnumerator::OnWindow(HWND hwnd) const {
-  return !filter_.Run(hwnd);
-}
-
-// static
-BOOL CALLBACK WindowEnumerator::OnWindowProc(HWND hwnd, LPARAM lparam) {
-  return reinterpret_cast<WindowEnumerator*>(lparam)->OnWindow(hwnd);
+  std::wstring text(static_cast<size_t>(num_chars), L'\0');
+  // MSDN says that GetWindowText will not write anything other than a string
+  // terminator to the last position in the buffer.
+  auto len = ::GetWindowText(hwnd, text.data(), num_chars + 1);
+  if (len <= 0) {
+    return std::wstring();
+  }
+  // GetWindowText may return a shorter string than reported above.
+  text.resize(static_cast<size_t>(len));
+  return text;
 }
 
 }  // namespace base::win

base/win/window_enumerator.h

@@ -5,54 +5,34 @@
 #ifndef BASE_WIN_WINDOW_ENUMERATOR_H_
 #define BASE_WIN_WINDOW_ENUMERATOR_H_
 
-#include <windows.h>
-
 #include <string>
 
 #include "base/base_export.h"
 #include "base/functional/callback.h"
+#include "base/win/windows_types.h"
 
 namespace base::win {
 
-// Enumerates immediate child windows of `parent`, and calls `filter.Run` for
-// each window:
-// * If `filter.Run` returns `false`, continues enumerating.
-// * If `filter.Run` returns `true`, stops enumerating.
-class BASE_EXPORT WindowEnumerator {
- public:
-  WindowEnumerator(HWND parent,
-                   base::RepeatingCallback<bool(HWND hwnd)> filter);
-  WindowEnumerator(const WindowEnumerator&) = delete;
-  WindowEnumerator& operator=(const WindowEnumerator&) = delete;
-  ~WindowEnumerator();
-
-  void Run() const;
-
-  // Returns true if `hwnd` is an always-on-top window.
-  static bool IsTopmostWindow(HWND hwnd);
-
-  // Returns true if `hwnd` is a system dialog.
-  static bool IsSystemDialog(HWND hwnd);
-
-  // Returns true if `hwnd` is a window owned by the Windows shell.
-  static bool IsShellWindow(HWND hwnd);
+// Enumerates immediate child windows of `parent`, running `filter` for each
+// window until `filter` returns true.
+using WindowEnumeratorCallback = base::RepeatingCallback<bool(HWND hwnd)>;
+BASE_EXPORT void EnumerateChildWindows(HWND parent,
+                                       WindowEnumeratorCallback filter);
 
-  // Returns the class name of `hwnd` or an empty string on error.
-  static std::wstring GetWindowClass(HWND hwnd);
+// Returns true if `hwnd` is an always-on-top window.
+BASE_EXPORT bool IsTopmostWindow(HWND hwnd);
 
-  // Returns the window text for `hwnd`, or an empty string on error.
-  static std::wstring GetWindowText(HWND hwnd);
+// Returns true if `hwnd` is a system dialog.
+BASE_EXPORT bool IsSystemDialog(HWND hwnd);
 
- private:
-  // Main processing function run for each window.
-  bool OnWindow(HWND hwnd) const;
+// Returns true if `hwnd` is a window owned by the Windows shell.
+BASE_EXPORT bool IsShellWindow(HWND hwnd);
 
-  // An EnumWindowsProc invoked by EnumWindows once for each window.
-  static BOOL CALLBACK OnWindowProc(HWND hwnd, LPARAM lparam);
+// Returns the class name of `hwnd` or an empty string on error.
+BASE_EXPORT std::wstring GetWindowClass(HWND hwnd);
 
-  const HWND parent_;
-  base::RepeatingCallback<bool(HWND hwnd)> filter_;
-};
+// Returns the window text for `hwnd`, or an empty string on error.
+BASE_EXPORT std::wstring GetWindowTextString(HWND hwnd);
 
 }  // namespace base::win
 

base/win/window_enumerator_unittest.cc

@@ -15,10 +15,9 @@
 namespace base::win {
 
 TEST(WindowEnumeratorTest, EnumerateTopLevelWindows) {
-  WindowEnumerator(
+  EnumerateChildWindows(
       ::GetDesktopWindow(), base::BindLambdaForTesting([&](HWND hwnd) {
-        const std::wstring window_class =
-            WindowEnumerator::GetWindowClass(hwnd);
+        const std::wstring window_class = GetWindowClass(hwnd);
         EXPECT_EQ(window_class, [&]() {
           constexpr int kMaxWindowClassNameLength = 256;
           wchar_t buffer[kMaxWindowClassNameLength + 1] = {0};
@@ -29,17 +28,16 @@ TEST(WindowEnumeratorTest, EnumerateTopLevelWindows) {
           return std::wstring(&buffer[0], static_cast<size_t>(name_len));
         }());
 
-        EXPECT_EQ(WindowEnumerator::IsTopmostWindow(hwnd),
+        EXPECT_EQ(IsTopmostWindow(hwnd),
                   (::GetWindowLong(hwnd, GWL_EXSTYLE) & WS_EX_TOPMOST) != 0);
 
-        EXPECT_EQ(WindowEnumerator::IsSystemDialog(hwnd),
-                  window_class == L"#32770");
+        EXPECT_EQ(IsSystemDialog(hwnd), window_class == L"#32770");
 
-        EXPECT_EQ(WindowEnumerator::IsShellWindow(hwnd),
+        EXPECT_EQ(IsShellWindow(hwnd),
                   window_class == L"Button" ||
                       window_class == L"Shell_TrayWnd" ||
                       window_class == L"Shell_SecondaryTrayWnd");
-        EXPECT_EQ(WindowEnumerator::GetWindowText(hwnd), [&]() {
+        EXPECT_EQ(GetWindowTextString(hwnd), [&]() {
           const int num_chars = ::GetWindowTextLength(hwnd);
           if (!num_chars) {
             return std::wstring();
@@ -49,11 +47,10 @@ TEST(WindowEnumeratorTest, EnumerateTopLevelWindows) {
                                static_cast<int>(text.size()))) {
             return std::wstring();
           }
-          return std::wstring(text.begin(), text.end());
+          return std::wstring(text.begin(), --text.end());
         }());
         return false;
-      }))
-      .Run();
+      }));
 }
 
 }  // namespace base::win

chrome/test/base/always_on_top_window_killer_win.cc

@@ -66,23 +66,22 @@ void KillAlwaysOnTopWindows(RunType run_type,
     }
   }
 
-  base::win::WindowEnumerator(
+  base::win::EnumerateChildWindows(
       ::GetDesktopWindow(), base::BindLambdaForTesting([&](HWND hwnd) {
         const bool kContinueIterating = false;
 
         if (!::IsWindowVisible(hwnd) || ::IsIconic(hwnd) ||
-            !base::win::WindowEnumerator::IsTopmostWindow(hwnd)) {
+            !base::win::IsTopmostWindow(hwnd)) {
           return kContinueIterating;
         }
 
-        const std::wstring class_name =
-            base::win::WindowEnumerator::GetWindowClass(hwnd);
+        const std::wstring class_name = base::win::GetWindowClass(hwnd);
         if (class_name.empty()) {
           return kContinueIterating;
         }
 
         // Ignore specific windows owned by the shell.
-        if (base::win::WindowEnumerator::IsShellWindow(hwnd)) {
+        if (base::win::IsShellWindow(hwnd)) {
           return kContinueIterating;
         }
 
@@ -95,7 +94,7 @@ void KillAlwaysOnTopWindows(RunType run_type,
         if (LOG_IS_ON(ERROR)) {
           std::wostringstream sstream;
 
-          if (!base::win::WindowEnumerator::IsSystemDialog(hwnd)) {
+          if (!base::win::IsSystemDialog(hwnd)) {
             sstream << " window class name: " << class_name << ";";
           }
 
@@ -131,7 +130,7 @@ void KillAlwaysOnTopWindows(RunType run_type,
 
         // System dialogs may be present if a child process triggers an
         // assert(), for example.
-        if (base::win::WindowEnumerator::IsSystemDialog(hwnd)) {
+        if (base::win::IsSystemDialog(hwnd)) {
           LOG(ERROR) << (run_type == RunType::BEFORE_SHARD
                              ? kDialogFoundBeforeTest
                              : kDialogFoundPostTest)
@@ -163,6 +162,5 @@ void KillAlwaysOnTopWindows(RunType run_type,
         }
 
         return kContinueIterating;
-      }))
-      .Run();
+      }));
 }

chrome/updater/test/integration_tests_win.cc

@@ -1767,31 +1767,27 @@ void CloseInstallCompleteDialog(const std::wstring& child_window_text_to_find) {
       [&]() {
         if (!found) {
           // Enumerate the top-level dialogs to find the setup dialog.
-          base::win::WindowEnumerator(
+          base::win::EnumerateChildWindows(
               ::GetDesktopWindow(), base::BindLambdaForTesting([&](HWND hwnd) {
-                if (!base::win::WindowEnumerator::IsSystemDialog(hwnd) ||
-                    !base::Contains(
-                        base::win::WindowEnumerator::GetWindowText(hwnd),
-                        window_title)) {
+                if (!base::win::IsSystemDialog(hwnd) ||
+                    !base::Contains(base::win::GetWindowTextString(hwnd),
+                                    window_title)) {
                   return false;
                 }
                 // Enumerate the child windows to search for
                 // `child_window_text_to_find`. If found, close the dialog.
-                base::win::WindowEnumerator(
+                base::win::EnumerateChildWindows(
                     hwnd, base::BindLambdaForTesting([&](HWND hwnd) {
-                      if (!base::Contains(
-                              base::win::WindowEnumerator::GetWindowText(hwnd),
-                              child_window_text_to_find)) {
+                      if (!base::Contains(base::win::GetWindowTextString(hwnd),
+                                          child_window_text_to_find)) {
                         return false;
                       }
                       found = true;
                       ::PostMessage(::GetParent(hwnd), WM_CLOSE, 0, 0);
                       return found;
-                    }))
-                    .Run();
+                    }));
                 return found;
-              }))
-              .Run();
+              }));
         }
         return found && !IsUpdaterRunning();
       },