base/threading: restrict to set only current thread priority

base/threading/platform_thread_unittest.cc

Index: base/threading/platform_thread_unittest.cc
diff --git a/base/threading/platform_thread_unittest.cc b/base/threading/platform_thread_unittest.cc
index c4b3d5d7ecc6092a89ca519800b55f92d3fd77da..89f0b225242c05229d2906776dc4539759bd482e 100644
--- a/base/threading/platform_thread_unittest.cc
+++ b/base/threading/platform_thread_unittest.cc
@@ -8,7 +8,10 @@
 #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
 
@@ -170,106 +173,47 @@ TEST(PlatformThreadTest, FunctionTimesTen) {
 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)
-    ThreadPriority::DISPLAY,
     ThreadPriority::REALTIME_AUDIO,
-    // Keep BACKGROUND second to last to test backgrounding from other
-    // priorities.
-    ThreadPriority::BACKGROUND,
-#endif  // !defined(OS_POSIX)
-    // Keep NORMAL last to test unbackgrounding.
-    ThreadPriority::NORMAL

[gab, 2015/06/23 17:01:32]

The order here was intentional, please keep it this way (with comments) rather
than having it match the enum's declaration order).

[Takashi Toyoshima, 2015/06/24 04:15:39]

Yep, I changed this order because of the reason gap@ also mentioned in the
following comment.

Also, NORMAL is not needed to be the last since we already have a code to set it
back to NORMAL at the end of the test.

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

But the code at the end of the test doesn't test the result whereas this was
explicitly testing unbackgrounding -- the latest patch set with the redundant
BACKGROUND,NORMAL,BACKGROUND is okay I think.

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

Oh, that's right. I missed the point that the last NORMAL is for testing
unbackgrounding.

+    ThreadPriority::DISPLAY,
+    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)
+// PlatformThread::GetCurrentThreadPriority() is not implemented on OS X.

[gab, 2015/06/23 17:01:32]

Nor on OS_ANDROID

[Takashi Toyoshima, 2015/06/24 04:15:39]

Ah, you are right. But it's curious that Android build bots pass the test.
Anyway, I'll add OS_ANDROID here.

[gab, 2015/06/25 14:49:00]

Well actually Android should work for all but REALTIME_AUDIO (still very
surprising that it passed that one though?). Can we add a selective ifdef above
rather than disable the entire test?

[Takashi Toyoshima, 2015/06/26 12:28:53]

No, GetCurrentThreadPriorityForPlatform() has NOTREACHED() internally, and it is
called first in GetCurrentThreadPriority(). So, it should crash with
NOTREACHED() in debug build as OS X does.
Another option is to enable it only in release build?

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

No, the Android code has NOTIMPLEMENTED() which only results in a log message
not in a crash like NOTREACHED().

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

Oh, I confused them. Right, I'll enable the test for Android with an exception
for REALTIME. Thanks!

+#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));
-
-  // 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));
+            PlatformThread::GetCurrentThreadPriority());
 
   // Toggle each supported priority on the current thread and confirm it only
   // affects it (and not the test thread).

[gab, 2015/06/23 17:01:32]

Fix this comment (there no longer is a test thread).

[Takashi Toyoshima, 2015/06/24 04:15:39]

Done.

   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.

[gab, 2015/06/23 17:01:32]

Please document this in the SetCurrentThreadPriority() API; i.e. a thread may
not be able to raise its priority back up after lowering it.

[Takashi Toyoshima, 2015/06/24 04:15:39]

Done.

+    if (geteuid() != 0 && kThreadPriorityTestValues[i] > ThreadPriority::NORMAL)

[gab, 2015/06/23 17:01:32]

Ah ok I now understand why you want to test backgrounding last on Linux (can't
bring priority back up after)... hmmm... perhaps we need different orders on
different platforms...?

Perhaps the enum should be:

AUDIO, DISPLAY, BACKGROUND, NORMAL, BACKGROUND

and here this code would check > CurrentPriority() instead of explicitly >
NORMAL? To skip unbackgrounding?

[Takashi Toyoshima, 2015/06/24 04:15:39]

Thanks you, that idea sounds nice.

+      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