[base] Handle negative values returned from ::GetThreadPriority().

::GetThreadPriority() sometimes returns -3 or -6 on bots.

-3
https://logs.chromium.org/logs/chrome/buildbucket/cr-buildbucket.appspot.com/8900924268806719008/+/steps/base_unittests/0/logs/PlatformThreadWinTest.SetBackgroundThreadModeFailsInIdlePriorityProcess/0

-6
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8900658140238913376/+/steps/browser_tests__with_patch__on_Windows-10-15063/0/logs/Flaky_failure:_BackgroundFetchBrowserTest.OfflineItemCollection_VerifyResourceDownloadedWhenDownloadTotalSmallerThanActualSize__status_FAILURE_SUCCESS_/0

Handling these values is a prerequisite to enable
THREAD_MODE_BACKGROUND_* by default.

Bug: 931720, 919466
Change-Id: Ib109a06bf95907a2988427158ea233b8395bf1f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829570
Commit-Queue: François Doray <fdoray@chromium.org>
Auto-Submit: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705262}

base/threading/platform_thread_win.cc

@@ -29,9 +29,9 @@ namespace base {
 
 namespace {
 
-// The value returned by ::GetThreadPriority() after background thread mode is
-// enabled on Windows 8+.
-constexpr int kWin8AboveBackgroundThreadModePriority = -4;
+// The most common value returned by ::GetThreadPriority() after background
+// thread mode is enabled on Windows 7.
+constexpr int kWin7BackgroundThreadModePriority = 4;
 
 // The information on how to set the thread name comes from
 // a MSDN article: http://msdn2.microsoft.com/en-us/library/xcb2z8hs.aspx
@@ -401,17 +401,45 @@ void PlatformThread::SetCurrentThreadPriorityImpl(ThreadPriority priority) {
 
 // static
 ThreadPriority PlatformThread::GetCurrentThreadPriority() {
+  static_assert(
+      THREAD_PRIORITY_IDLE < 0,
+      "THREAD_PRIORITY_IDLE is >= 0 and will incorrectly cause errors.");
+  static_assert(
+      THREAD_PRIORITY_LOWEST < 0,
+      "THREAD_PRIORITY_LOWEST is >= 0 and will incorrectly cause errors.");
+  static_assert(THREAD_PRIORITY_BELOW_NORMAL < 0,
+                "THREAD_PRIORITY_BELOW_NORMAL is >= 0 and will incorrectly "
+                "cause errors.");
+  static_assert(
+      THREAD_PRIORITY_NORMAL == 0,
+      "The logic below assumes that THREAD_PRIORITY_NORMAL is zero. If it is "
+      "not, ThreadPriority::BACKGROUND may be incorrectly detected.");
+  static_assert(THREAD_PRIORITY_ABOVE_NORMAL >= 0,
+                "THREAD_PRIORITY_ABOVE_NORMAL is < 0 and will incorrectly be "
+                "translated to ThreadPriority::BACKGROUND.");
+  static_assert(THREAD_PRIORITY_HIGHEST >= 0,
+                "THREAD_PRIORITY_HIGHEST is < 0 and will incorrectly be "
+                "translated to ThreadPriority::BACKGROUND.");
+  static_assert(THREAD_PRIORITY_TIME_CRITICAL >= 0,
+                "THREAD_PRIORITY_TIME_CRITICAL is < 0 and will incorrectly be "
+                "translated to ThreadPriority::BACKGROUND.");
+  static_assert(THREAD_PRIORITY_ERROR_RETURN >= 0,
+                "THREAD_PRIORITY_ERROR_RETURN is < 0 and will incorrectly be "
+                "translated to ThreadPriority::BACKGROUND.");
+
   const int priority =
       ::GetThreadPriority(PlatformThread::CurrentHandle().platform_handle());
 
+  // Negative values represent a background priority. We have observed -3, -4,
+  // -6 when THREAD_MODE_BACKGROUND_* is used. THREAD_PRIORITY_IDLE,
+  // THREAD_PRIORITY_LOWEST and THREAD_PRIORITY_BELOW_NORMAL are other possible
+  // negative values.
+  if (priority < THREAD_PRIORITY_NORMAL)
+    return ThreadPriority::BACKGROUND;
+
   switch (priority) {
-    case THREAD_PRIORITY_IDLE:
-    case internal::kWin7BackgroundThreadModePriority:
+    case kWin7BackgroundThreadModePriority:
       DCHECK_EQ(win::GetVersion(), win::Version::WIN7);
-      FALLTHROUGH;
-    case kWin8AboveBackgroundThreadModePriority:
-    case THREAD_PRIORITY_LOWEST:
-    case THREAD_PRIORITY_BELOW_NORMAL:
       return ThreadPriority::BACKGROUND;
     case THREAD_PRIORITY_NORMAL:
       return ThreadPriority::NORMAL;
@@ -421,10 +449,10 @@ ThreadPriority PlatformThread::GetCurrentThreadPriority() {
     case THREAD_PRIORITY_TIME_CRITICAL:
       return ThreadPriority::REALTIME_AUDIO;
     case THREAD_PRIORITY_ERROR_RETURN:
-      DPCHECK(false) << "GetThreadPriority error";
+      DPCHECK(false) << "::GetThreadPriority error";
   }
 
-  NOTREACHED() << "GetCurrentThreadPriority returned " << priority << ".";
+  NOTREACHED() << "::GetThreadPriority returned " << priority << ".";
   return ThreadPriority::NORMAL;
 }
 

base/threading/platform_thread_win.h

@@ -25,10 +25,6 @@ BASE_EXPORT extern const Feature kWindowsThreadModeBackground;
 
 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/threading/platform_thread_win_unittest.cc

@@ -51,23 +51,19 @@ TEST(PlatformThreadWinTest, SetBackgroundThreadModeFailsInIdlePriorityProcess) {
   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
+  // switch to one of the various priorities that are observed after entering
   // thread mode background in a NORMAL_PRIORITY_CLASS process. On all Windows
-  // verisons, memory priority becomes VERY_LOW.
+  // versions, 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,
-                testing::Contains(priority_after_thread_mode_background_begin));
+    if (priority_after_thread_mode_background_begin != THREAD_PRIORITY_NORMAL) {
+      EXPECT_EQ(ThreadPriority::BACKGROUND,
+                base::PlatformThread::GetCurrentThreadPriority());
+    }
   } else {
     EXPECT_EQ(priority_after_thread_mode_background_begin,
               THREAD_PRIORITY_NORMAL);