base/threading: restrict to set only current thread priority

base/threading/platform_thread_unittest.cc

 // Copyright (c) 2012 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/compiler_specific.h"
 #include "base/macros.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_WIN)
+#if defined(OS_POSIX)
+#include <sys/types.h>
+#include <unistd.h>
+#elif defined(OS_WIN)
 #include <windows.h>
 #endif
 
 namespace base {
 
 // Trivial tests that thread runs and doesn't crash on create and join ---------
 
 class TrivialThread : public PlatformThread::Delegate {
  public:
   TrivialThread() : did_run_(false) {}
@@ skipping 141 matching lines @@
   for (size_t n = 0; n < arraysize(thread); n++)
     ASSERT_FALSE(thread[n].IsRunning());
 
   // Make sure that the thread ID is the same across calls.
   EXPECT_EQ(main_thread_id, PlatformThread::CurrentId());
 }
 
 namespace {
 
 const ThreadPriority kThreadPriorityTestValues[] = {
-// Disable non-normal priority toggling on POSIX as it appears to be broken
-// (http://crbug.com/468793). This is prefered to disabling the tests altogether
-// on POSIX as it at least provides coverage for running this code under
-// "normal" priority.
-#if !defined(OS_POSIX)
+    // Basically, the order should be higher to lower to cover as much cases

[gab, 2015/06/26 15:57:13]

Remove "Basically" I'd say, i.e.:

// The order should be (...)

[Takashi Toyoshima, 2015/06/29 05:48:51]

Done.

+    // as possible on Linux trybots running without CAP_SYS_NICE permission.
+    ThreadPriority::REALTIME_AUDIO,
     ThreadPriority::DISPLAY,
-    ThreadPriority::REALTIME_AUDIO,
-    // Keep BACKGROUND second to last to test backgrounding from other
-    // priorities.
+    // This redundant BACKGROUND priority is to test backgrounding from other
+    // priorities, and unbackgrounding.
     ThreadPriority::BACKGROUND,
-#endif  // !defined(OS_POSIX)
-    // Keep NORMAL last to test unbackgrounding.
-    ThreadPriority::NORMAL
+    ThreadPriority::NORMAL,
+    ThreadPriority::BACKGROUND
 };
 
 }  // namespace
 
-// Test changing another thread's priority.
-// NOTE: This test is partially disabled on POSIX, see note above and
-// http://crbug.com/468793.
-TEST(PlatformThreadTest, ThreadPriorityOtherThread) {
-  PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
-
-  // Confirm that the current thread's priority is as expected.
-  EXPECT_EQ(ThreadPriority::NORMAL,
-            PlatformThread::GetThreadPriority(current_handle));
-
-  // Create a test thread.
-  FunctionTestThread thread;
-  PlatformThreadHandle handle;
-  ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
-  thread.WaitForThreadStart();
-  EXPECT_NE(thread.thread_id(), kInvalidThreadId);
-  EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
-
-  // New threads should get normal priority by default.
-  EXPECT_EQ(ThreadPriority::NORMAL, PlatformThread::GetThreadPriority(handle));
-
-  // Toggle each supported priority on the test thread and confirm it only
-  // affects it (and not the current thread).
-  for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
-    SCOPED_TRACE(i);
-
-    // Alter and verify the test thread's priority.
-    PlatformThread::SetThreadPriority(handle, kThreadPriorityTestValues[i]);
-    EXPECT_EQ(kThreadPriorityTestValues[i],
-              PlatformThread::GetThreadPriority(handle));
-
-    // Make sure the current thread was otherwise unaffected.
-    EXPECT_EQ(ThreadPriority::NORMAL,
-              PlatformThread::GetThreadPriority(current_handle));
-  }
-
-  thread.MarkForTermination();
-  PlatformThread::Join(handle);
-}
+#if defined(OS_MACOSX) || defined(OS_ANDROID)
+// PlatformThread::GetCurrentThreadPriority() is not implemented on OS X.
+#define MAYBE_ThreadPriorityCurrentThread DISABLED_ThreadPriorityCurrentThread
+#else
+#define MAYBE_ThreadPriorityCurrentThread ThreadPriorityCurrentThread
+#endif
 
 // Test changing the current thread's priority (which has different semantics on
 // some platforms).
-// NOTE: This test is partially disabled on POSIX, see note above and
-// http://crbug.com/468793.
-TEST(PlatformThreadTest, ThreadPriorityCurrentThread) {
-  PlatformThreadHandle current_handle(PlatformThread::CurrentHandle());
-
+TEST(PlatformThreadTest, MAYBE_ThreadPriorityCurrentThread) {
   // Confirm that the current thread's priority is as expected.
   EXPECT_EQ(ThreadPriority::NORMAL,
-            PlatformThread::GetThreadPriority(current_handle));
+            PlatformThread::GetCurrentThreadPriority());
 
-  // Create a test thread for verification purposes only.
-  FunctionTestThread thread;
-  PlatformThreadHandle handle;
-  ASSERT_TRUE(PlatformThread::Create(0, &thread, &handle));
-  thread.WaitForThreadStart();
-  EXPECT_NE(thread.thread_id(), kInvalidThreadId);
-  EXPECT_NE(thread.thread_id(), PlatformThread::CurrentId());
-
-  // Confirm that the new thread's priority is as expected.
-  EXPECT_EQ(ThreadPriority::NORMAL, PlatformThread::GetThreadPriority(handle));
-
-  // Toggle each supported priority on the current thread and confirm it only
-  // affects it (and not the test thread).
+  // Toggle each supported priority on the current thread and confirm it
+  // affects it.
   for (size_t i = 0; i < arraysize(kThreadPriorityTestValues); ++i) {
     SCOPED_TRACE(i);
+#if defined(OS_POSIX)
+    // Only root or a user with CAP_SYS_NICE permission can raise the thread
+    // priority.
+    ThreadPriority current_priority =
+        PlatformThread::GetCurrentThreadPriority();

[gab, 2015/06/26 15:57:13]

inline this in the conditional below maybe? (otherwise you end up with a local
scope variable only on POSIX -- i.e., if someone is changing this code on
Windows and adds a variable with the same name outside the ifdef, it won't fail
to compile until they hit a Linux trybot).

[Takashi Toyoshima, 2015/06/29 05:48:51]

Done.

+    if (geteuid() != 0 && kThreadPriorityTestValues[i] > current_priority)

[gab, 2015/06/26 15:57:13]

According to
http://stackoverflow.com/questions/4159910/check-if-user-is-root-in-c checking
for root is perhaps incorrect/overkill (i.e. the user could be non-root but have
CAP_SYS_NICE?).

Is it worth checking for this permission rather than checking for root?

Also, please check for this once outside the loop instead of every time inside,
i.e.:
const bool bumping_priority_allowed =
#if defined(OS_POSIX)
   IsCapSysNicePermissionSet();
#else
   true;
#endif 

[Takashi Toyoshima, 2015/06/29 05:48:51]

To check the capability, we need libcap.so and the code works only on Linux. I
think depending on libcap.so only for this check is over engineering. I updated
code and comments to explain details.

+      continue;
+#endif
 
     // Alter and verify the current thread's priority.
-    PlatformThread::SetThreadPriority(current_handle,
-                                      kThreadPriorityTestValues[i]);
+    PlatformThread::SetCurrentThreadPriority(kThreadPriorityTestValues[i]);
     EXPECT_EQ(kThreadPriorityTestValues[i],
-              PlatformThread::GetThreadPriority(current_handle));
-
-    // Make sure the test thread was otherwise unaffected.
-    EXPECT_EQ(ThreadPriority::NORMAL,
-              PlatformThread::GetThreadPriority(handle));
+              PlatformThread::GetCurrentThreadPriority());
   }
 
   // Restore current thread priority for follow-up tests.
-  PlatformThread::SetThreadPriority(current_handle, ThreadPriority::NORMAL);
-
-  thread.MarkForTermination();
-  PlatformThread::Join(handle);
+  PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
 }
 
 }  // namespace base