Base: Ensure that SetCurrentThreadPriority(BACKGROUND) sets CPU priority on Windows (reland).

Previously landed as
https://chromium-review.googlesource.com/c/1318438.
Reverted because
PlatformThreadWinTest.SetBackgroundThreadModeFailsInIdlePriorityProcess
failed on Windows 7. This reland adds more possible return values for
::GetThreadPriority() on Windows 7 (see diff between ps1 and ps7).

Previously, SetCurrentThreadPriority(BACKGROUND) did not affect CPU
priority on Windows in a IDLE process when the
"WindowsThreadModeBackground" feature was enabled. It only affected I/O
and memory priorities.

With this CL, a second call to ::SetThreadPriority() is made to
ensure that CPU priority is affected.

TBR=gab@chromium.org

Bug: 901483
Change-Id: I21ebc6465b3b8d10a956714d55ef88b28bf89fff
Reviewed-on: https://chromium-review.googlesource.com/c/1326423Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607269}

base/BUILD.gn

@@ -2497,6 +2497,7 @@ test("base_unittests") {
     "test/trace_event_analyzer_unittest.cc",
     "thread_annotations_unittest.cc",
     "threading/platform_thread_unittest.cc",
+    "threading/platform_thread_win_unittest.cc",
     "threading/post_task_and_reply_impl_unittest.cc",
     "threading/scoped_blocking_call_unittest.cc",
     "threading/sequence_bound_unittest.cc",

base/threading/platform_thread_unittest.cc

@@ -295,6 +295,21 @@ TEST(PlatformThreadTest, SetCurrentThreadPriorityWithThreadModeBackground) {
       features::kWindowsThreadModeBackground);
   TestSetCurrentThreadPriority();
 }
+
+// Test changing a created thread's priority, with the
+// kWindowsThreadModeBackground feature enabled, in an IDLE_PRIORITY_CLASS
+// process (regression test for https://crbug.com/901483).
+TEST(PlatformThreadTest,
+     SetCurrentThreadPriorityWithThreadModeBackgroundIdleProcess) {
+  ::SetPriorityClass(Process::Current().Handle(), IDLE_PRIORITY_CLASS);
+
+  test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      features::kWindowsThreadModeBackground);
+  TestSetCurrentThreadPriority();
+
+  ::SetPriorityClass(Process::Current().Handle(), NORMAL_PRIORITY_CLASS);
+}
 #endif  // defined(OS_WIN)
 
 // Ideally PlatformThread::CanIncreaseThreadPriority() would be true on all

base/threading/platform_thread_win.cc

@@ -23,10 +23,6 @@ namespace base {
 
 namespace {
 
-// The value returned by ::GetThreadPriority() after background thread mode is
-// enabled on Windows 7.
-constexpr int kWin7BackgroundThreadModePriority = 4;
-
 // The value returned by ::GetThreadPriority() after background thread mode is
 // enabled on Windows 8+.
 constexpr int kWin8AboveBackgroundThreadModePriority = -4;
@@ -158,6 +154,31 @@ const Feature kWindowsThreadModeBackground{"WindowsThreadModeBackground",
                                            FEATURE_DISABLED_BY_DEFAULT};
 }  // namespace features
 
+namespace internal {
+
+void AssertMemoryPriority(HANDLE thread, int memory_priority) {
+#if DCHECK_IS_ON()
+  static const auto get_thread_information_fn =
+      reinterpret_cast<decltype(&::GetThreadInformation)>(::GetProcAddress(
+          ::GetModuleHandle(L"Kernel32.dll"), "GetThreadInformation"));
+
+  if (!get_thread_information_fn) {
+    DCHECK_EQ(win::GetVersion(), win::VERSION_WIN7);
+    return;
+  }
+
+  MEMORY_PRIORITY_INFORMATION memory_priority_information = {};
+  DCHECK(get_thread_information_fn(thread, ::ThreadMemoryPriority,
+                                   &memory_priority_information,
+                                   sizeof(memory_priority_information)));
+
+  DCHECK_EQ(memory_priority,
+            static_cast<int>(memory_priority_information.MemoryPriority));
+#endif
+}
+
+}  // namespace internal
+
 // static
 PlatformThreadId PlatformThread::CurrentId() {
   return ::GetCurrentThreadId();
@@ -290,11 +311,14 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
            : (FeatureList::GetInstance() &&
               FeatureList::IsEnabled(features::kWindowsThreadModeBackground)));
 
+  PlatformThreadHandle::Handle thread_handle =
+      PlatformThread::CurrentHandle().platform_handle();
+
   if (use_thread_mode_background && priority != ThreadPriority::BACKGROUND) {
     // Exit background mode if the new priority is not BACKGROUND. This is a
     // no-op if not in background mode.
-    ::SetThreadPriority(PlatformThread::CurrentHandle().platform_handle(),
-                        THREAD_MODE_BACKGROUND_END);
+    ::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_END);
+    internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
   }
 
   int desired_priority = THREAD_PRIORITY_ERROR_RETURN;
@@ -322,13 +346,24 @@ void PlatformThread::SetCurrentThreadPriority(ThreadPriority priority) {
 #if DCHECK_IS_ON()
   const BOOL success =
 #endif
-      ::SetThreadPriority(PlatformThread::CurrentHandle().platform_handle(),
-                          desired_priority);
+      ::SetThreadPriority(thread_handle, desired_priority);
   DPLOG_IF(ERROR, !success) << "Failed to set thread priority to "
                             << desired_priority;
 
-  // Sanity check that GetCurrentThreadPriority() is consistent with
-  // SetCurrentThreadPriority().
+  if (use_thread_mode_background && priority == ThreadPriority::BACKGROUND) {
+    // In a background process, THREAD_MODE_BACKGROUND_BEGIN lowers the memory
+    // and I/O priorities but not the CPU priority (kernel bug?). Use
+    // THREAD_PRIORITY_LOWEST to also lower the CPU priority.
+    // https://crbug.com/901483
+    if (GetCurrentThreadPriority() != ThreadPriority::BACKGROUND) {
+      ::SetThreadPriority(thread_handle, THREAD_PRIORITY_LOWEST);
+      // Make sure that using THREAD_PRIORITY_LOWEST didn't affect the memory
+      // priority set by THREAD_MODE_BACKGROUND_BEGIN. There is no practical
+      // way to verify the I/O priority.
+      internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
+    }
+  }
+
   DCHECK_EQ(GetCurrentThreadPriority(), priority);
 }
 
@@ -339,7 +374,7 @@ ThreadPriority PlatformThread::GetCurrentThreadPriority() {
 
   switch (priority) {
     case THREAD_PRIORITY_IDLE:
-    case kWin7BackgroundThreadModePriority:
+    case internal::kWin7BackgroundThreadModePriority:
       DCHECK_EQ(win::GetVersion(), win::VERSION_WIN7);
       FALLTHROUGH;
     case kWin8AboveBackgroundThreadModePriority:

base/threading/platform_thread_win.h

@@ -11,6 +11,7 @@
 #include "base/feature_list.h"
 
 namespace base {
+
 namespace features {
 
 // Use THREAD_MODE_BACKGROUND_BEGIN instead of THREAD_PRIORITY_LOWEST for
@@ -21,6 +22,20 @@ namespace features {
 BASE_EXPORT extern const Feature kWindowsThreadModeBackground;
 
 }  // namespace features
+
+namespace internal {
+
+// The value returned by ::GetThreadPriority() after background thread mode is
+// enabled on Windows 7. Exposed for unit tests.
+constexpr int kWin7BackgroundThreadModePriority = 4;
+
+// Assert that the memory priority of |thread| is |memory_priority|. No-op on
+// Windows 7 because ::GetThreadInformation() is not available. Exposed for unit
+// tests.
+BASE_EXPORT void AssertMemoryPriority(HANDLE thread, int memory_priority);
+
+}  // namespace internal
+
 }  // namespace base
 
 #endif  // BASE_THREADING_PLATFORM_THREAD_WIN_H_

base/threading/platform_thread_win_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/threading/platform_thread_win.h"
+
+#include <windows.h>
+
+#include <array>
+
+#include "base/process/process.h"
+#include "base/win/windows_version.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Contains;
+
+namespace base {
+
+// It has been observed that calling
+// :SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN) in an IDLE_PRIORITY_CLASS
+// process doesn't always affect the return value of ::GetThreadPriority() or
+// the base priority reported in Process Explorer (on Win7, the values are
+// sometimes affected while on Win8+ they are never affected). It does however
+// set the memory and I/O priorities to very low. This test confirms that
+// behavior which we suspect is a Windows kernel bug. If this test starts
+// failing, the mitigation for https://crbug.com/901483 in
+// PlatformThread::SetCurrentThreadPriority() should be revisited.
+TEST(PlatformThreadWinTest, SetBackgroundThreadModeFailsInIdlePriorityProcess) {
+  PlatformThreadHandle::Handle thread_handle =
+      PlatformThread::CurrentHandle().platform_handle();
+
+  // ::GetThreadPriority() is NORMAL. Memory priority is NORMAL.
+  // Note: There is no practical way to verify the I/O priority.
+  EXPECT_EQ(::GetThreadPriority(thread_handle), THREAD_PRIORITY_NORMAL);
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
+
+  // Set the process priority to IDLE.
+  // Note: Do not use Process::SetProcessBackgrounded() because it uses
+  // PROCESS_MODE_BACKGROUND_BEGIN instead of IDLE_PRIORITY_CLASS when
+  // the target is the current process.
+  EXPECT_EQ(::GetPriorityClass(Process::Current().Handle()),
+            static_cast<DWORD>(NORMAL_PRIORITY_CLASS));
+  ::SetPriorityClass(Process::Current().Handle(), IDLE_PRIORITY_CLASS);
+  EXPECT_EQ(Process::Current().GetPriority(),
+            static_cast<int>(IDLE_PRIORITY_CLASS));
+
+  // GetThreadPriority() stays NORMAL. Memory priority stays NORMAL.
+  EXPECT_EQ(::GetThreadPriority(thread_handle), THREAD_PRIORITY_NORMAL);
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
+
+  // Begin thread mode background.
+  EXPECT_TRUE(::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_BEGIN));
+
+  // On Win8, GetThreadPriority() stays NORMAL. On Win7, it can stay NORMAL or
+  // switch to one of the 2 priorities that are usually observed after entering
+  // thread mode background in a NORMAL_PRIORITY_CLASS process. On all Windows
+  // verisons, memory priority becomes VERY_LOW.
+  //
+  // Note: this documents the aforementioned kernel bug. Ideally this would
+  // *not* be the case.
+  const float priority_after_thread_mode_background_begin =
+      ::GetThreadPriority(thread_handle);
+  if (win::GetVersion() == win::VERSION_WIN7) {
+    constexpr std::array<int, 3> kExpectedWin7Priorities(
+        {// Priority if GetThreadPriority() is not affected.
+         THREAD_PRIORITY_NORMAL,
+         // Priorities if GetThreadPriority() behaves like in a
+         // NORMAL_PRIORITY_CLASS process.
+         THREAD_PRIORITY_IDLE, internal::kWin7BackgroundThreadModePriority});
+    EXPECT_THAT(kExpectedWin7Priorities,
+                Contains(priority_after_thread_mode_background_begin));
+  } else {
+    EXPECT_EQ(priority_after_thread_mode_background_begin,
+              THREAD_PRIORITY_NORMAL);
+  }
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
+
+  PlatformThread::Sleep(base::TimeDelta::FromSeconds(1));
+
+  // After 1 second, GetThreadPriority() and memory priority don't change (this
+  // refutes the hypothesis that it simply takes time before GetThreadPriority()
+  // is updated after entering thread mode background).
+  EXPECT_EQ(::GetThreadPriority(thread_handle),
+            priority_after_thread_mode_background_begin);
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
+
+  // Set the process priority to NORMAL.
+  ::SetPriorityClass(Process::Current().Handle(), NORMAL_PRIORITY_CLASS);
+
+  // GetThreadPriority() and memory priority don't change when the process
+  // priority changes.
+  EXPECT_EQ(::GetThreadPriority(thread_handle),
+            priority_after_thread_mode_background_begin);
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_VERY_LOW);
+
+  // End thread mode background.
+  //
+  // Note: at least "ending" the semi-enforced background mode works...
+  EXPECT_TRUE(::SetThreadPriority(thread_handle, THREAD_MODE_BACKGROUND_END));
+
+  // GetThreadPriority() stays/becomes NORMAL. Memory priority becomes NORMAL.
+  EXPECT_EQ(::GetThreadPriority(thread_handle), THREAD_PRIORITY_NORMAL);
+  internal::AssertMemoryPriority(thread_handle, MEMORY_PRIORITY_NORMAL);
+}
+
+}  // namespace base