Backed out 3 changesets (bug 980027) for causing a serious race in hal with the pref service
authorWes Kocher <wkocher@mozilla.com>
Wed, 19 Mar 2014 15:36:13 -0700
changeset 174299 aba5b8498b307acd54321ca4b6fcc09c73531b6c
parent 174298 d6e549d77ab282dec7bb132c55cadec8001a7bd0
child 174300 e84a391b604b6690c8360f20518a70172679fff3
push id26449
push userkwierso@gmail.com
push dateWed, 19 Mar 2014 22:36:21 +0000
treeherdermozilla-central@aba5b8498b30 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs980027
milestone31.0a1
backs out5cd05df56f67c87afcef98e6a0b3be7936233d7a
6344d660651730f541359cbad751de00a97853e8
098a43b537e927416d1a0aee067dd04a5a3bd3f6
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out 3 changesets (bug 980027) for causing a serious race in hal with the pref service Backed out changeset 5cd05df56f67 (bug 980027) Backed out changeset 6344d6606517 (bug 980027) Backed out changeset 098a43b537e9 (bug 980027)
b2g/app/b2g.js
gfx/layers/ipc/CompositorParent.cpp
hal/Hal.cpp
hal/Hal.h
hal/HalTypes.h
hal/fallback/FallbackThreadPriority.cpp
hal/gonk/GonkHal.cpp
hal/moz.build
hal/sandbox/SandboxHal.cpp
--- a/b2g/app/b2g.js
+++ b/b2g/app/b2g.js
@@ -671,29 +671,16 @@ pref("hal.processPriorityManager.gonk.BA
 
 pref("hal.processPriorityManager.gonk.BACKGROUND.OomScoreAdjust", 667);
 pref("hal.processPriorityManager.gonk.BACKGROUND.KillUnderKB", 20480);
 pref("hal.processPriorityManager.gonk.BACKGROUND.Nice", 18);
 
 // Processes get this niceness when they have low CPU priority.
 pref("hal.processPriorityManager.gonk.LowCPUNice", 18);
 
-// By default the compositor thread on gonk runs without real-time priority.  RT
-// priority can be enabled by setting this pref to a value between 1 and 99.
-// Note that audio processing currently runs at RT priority 2 or 3 at most.
-//
-// If RT priority is disabled, then the compositor nice value is used.  The
-// code will default to ANDROID_PRIORITY_URGENT_DISPLAY which is -8.  Per gfx
-// request we are keeping the compositor at nice level 0 until we can complete
-// the investigation in bug 982972.
-//
-// Do not change these values without gfx team review.
-pref("hal.gonk.compositor.rt_priority", 0);
-pref("hal.gonk.compositor.nice", 0);
-
 // Fire a memory pressure event when the system has less than Xmb of memory
 // remaining.  You should probably set this just above Y.KillUnderKB for
 // the highest priority class Y that you want to make an effort to keep alive.
 // (For example, we want BACKGROUND_PERCEIVABLE to stay alive.)  If you set
 // this too high, then we'll send out a memory pressure event every Z seconds
 // (see below), even while we have processes that we would happily kill in
 // order to free up memory.
 pref("hal.processPriorityManager.gonk.notifyLowMemUnderKB", 14336);
--- a/gfx/layers/ipc/CompositorParent.cpp
+++ b/gfx/layers/ipc/CompositorParent.cpp
@@ -46,18 +46,16 @@
 #include "nsThreadUtils.h"              // for NS_IsMainThread
 #include "nsXULAppAPI.h"                // for XRE_GetIOMessageLoop
 #ifdef XP_WIN
 #include "mozilla/layers/CompositorD3D11.h"
 #include "mozilla/layers/CompositorD3D9.h"
 #endif
 #include "GeckoProfiler.h"
 #include "mozilla/ipc/ProtocolTypes.h"
-#include "mozilla/Hal.h"
-#include "mozilla/HalTypes.h"
 
 using namespace base;
 using namespace mozilla;
 using namespace mozilla::ipc;
 using namespace mozilla::gfx;
 using namespace std;
 
 namespace mozilla {
@@ -106,21 +104,16 @@ static void DeleteCompositorThread()
 
 static void ReleaseCompositorThread()
 {
   if(--sCompositorThreadRefCount == 0) {
     DeleteCompositorThread();
   }
 }
 
-static void SetThreadPriority()
-{
-  hal::SetCurrentThreadPriority(hal::THREAD_PRIORITY_COMPOSITOR);
-}
-
 void
 CompositorParent::StartUpWithExistingThread(MessageLoop* aMsgLoop,
                                             PlatformThreadId aThreadID)
 {
   MOZ_ASSERT(!sCompositorThread);
   CreateCompositorMap();
   sCompositorLoop = aMsgLoop;
   sCompositorThreadID = aThreadID;
@@ -164,17 +157,16 @@ bool CompositorParent::CreateThread()
      than the default hang timeout on major platforms (about 5 seconds). */
   options.permanent_hang_timeout = 8192; // milliseconds
 
   if (!sCompositorThread->StartWithOptions(options)) {
     delete sCompositorThread;
     sCompositorThread = nullptr;
     return false;
   }
-
   return true;
 }
 
 void CompositorParent::DestroyThread()
 {
   NS_ASSERTION(NS_IsMainThread(), "Should be on the main Thread!");
   ReleaseCompositorThread();
 }
@@ -203,18 +195,16 @@ CompositorParent::CompositorParent(nsIWi
   MOZ_COUNT_CTOR(CompositorParent);
   mCompositorID = 0;
   // FIXME: This holds on the the fact that right now the only thing that
   // can destroy this instance is initialized on the compositor thread after
   // this task has been processed.
   CompositorLoop()->PostTask(FROM_HERE, NewRunnableFunction(&AddCompositor,
                                                           this, &mCompositorID));
 
-  CompositorLoop()->PostTask(FROM_HERE, NewRunnableFunction(SetThreadPriority));
-
   mRootLayerTreeID = AllocateLayerTreeId();
   sIndirectLayerTrees[mRootLayerTreeID].mParent = this;
 
   mApzcTreeManager = new APZCTreeManager();
   ++sCompositorThreadRefCount;
 }
 
 PlatformThreadId
--- a/hal/Hal.cpp
+++ b/hal/Hal.cpp
@@ -866,22 +866,16 @@ SetProcessPriority(int aPid,
 {
   // n.b. The sandboxed implementation crashes; SetProcessPriority works only
   // from the main process.
   MOZ_ASSERT(aBackgroundLRU == 0 || aPriority == PROCESS_PRIORITY_BACKGROUND);
   PROXY_IF_SANDBOXED(SetProcessPriority(aPid, aPriority, aCPUPriority,
                                         aBackgroundLRU));
 }
 
-void
-SetCurrentThreadPriority(ThreadPriority aPriority)
-{
-  PROXY_IF_SANDBOXED(SetCurrentThreadPriority(aPriority));
-}
-
 // From HalTypes.h.
 const char*
 ProcessPriorityToString(ProcessPriority aPriority)
 {
   switch (aPriority) {
   case PROCESS_PRIORITY_MASTER:
     return "MASTER";
   case PROCESS_PRIORITY_FOREGROUND_HIGH:
@@ -899,28 +893,16 @@ ProcessPriorityToString(ProcessPriority 
   case PROCESS_PRIORITY_UNKNOWN:
     return "UNKNOWN";
   default:
     MOZ_ASSERT(false);
     return "???";
   }
 }
 
-const char *
-ThreadPriorityToString(ThreadPriority aPriority)
-{
-  switch (aPriority) {
-  case THREAD_PRIORITY_COMPOSITOR:
-    return "COMPOSITOR";
-  default:
-    MOZ_ASSERT(false);
-    return "???";
-  }
-}
-
 // From HalTypes.h.
 const char*
 ProcessPriorityToString(ProcessPriority aPriority,
                         ProcessCPUPriority aCPUPriority)
 {
   // Sorry this is ugly.  At least it's all in one place.
   //
   // We intentionally fall through if aCPUPriority is invalid; we won't hit any
--- a/hal/Hal.h
+++ b/hal/Hal.h
@@ -492,23 +492,16 @@ bool SetAlarm(int32_t aSeconds, int32_t 
  * ignore this call entirely.
  */
 void SetProcessPriority(int aPid,
                         hal::ProcessPriority aPriority,
                         hal::ProcessCPUPriority aCPUPriority,
                         uint32_t aLRU = 0);
 
 /**
- * Set the current thread's priority to appropriate platform-specific value for
- * given functionality.  Instead of providing arbitrary priority numbers you
- * must specify a type of function like THREAD_PRIORITY_COMPOSITOR.
- */
-void SetCurrentThreadPriority(hal::ThreadPriority aPriority);
-
-/**
  * Register an observer for the FM radio.
  */
 void RegisterFMRadioObserver(hal::FMRadioObserver* aRadioObserver);
 
 /**
  * Unregister the observer for the FM radio.
  */
 void UnregisterFMRadioObserver(hal::FMRadioObserver* aRadioObserver);
--- a/hal/HalTypes.h
+++ b/hal/HalTypes.h
@@ -94,47 +94,29 @@ enum ProcessPriority {
 };
 
 enum ProcessCPUPriority {
   PROCESS_CPU_PRIORITY_LOW,
   PROCESS_CPU_PRIORITY_NORMAL,
   NUM_PROCESS_CPU_PRIORITY
 };
 
-// Values that can be passed to hal::SetThreadPriority().  These should be
-// functional in nature, such as COMPOSITOR, instead of levels, like LOW/HIGH.
-// This allows us to tune our priority scheme for the system in one place such
-// that it makes sense holistically for the overall operating system.  On gonk
-// or android we may want different priority schemes than on windows, etc.
-enum ThreadPriority {
-  THREAD_PRIORITY_COMPOSITOR,
-  NUM_THREAD_PRIORITY
-};
-
 // Convert a ProcessPriority enum value (with an optional ProcessCPUPriority)
 // to a string.  The strings returned by this function are statically
 // allocated; do not attempt to free one!
 //
 // If you pass an unknown process priority (or NUM_PROCESS_PRIORITY), we
 // fatally assert in debug builds and otherwise return "???".
 const char*
 ProcessPriorityToString(ProcessPriority aPriority);
 
 const char*
 ProcessPriorityToString(ProcessPriority aPriority,
                         ProcessCPUPriority aCPUPriority);
 
-// Convert a ThreadPriority enum value to a string.  The strings returned by
-// this function are statically allocated; do not attempt to free one!
-//
-// If you pass an unknown process priority (or NUM_THREAD_PRIORITY), we
-// fatally assert in debug builds and otherwise return "???".
-const char *
-ThreadPriorityToString(ThreadPriority aPriority);
-
 /**
  * Used by ModifyWakeLock
  */
 enum WakeLockControl {
   WAKE_LOCK_REMOVE_ONE = -1,
   WAKE_LOCK_NO_CHANGE  = 0,
   WAKE_LOCK_ADD_ONE    = 1,
   NUM_WAKE_LOCK
deleted file mode 100644
--- a/hal/fallback/FallbackThreadPriority.cpp
+++ /dev/null
@@ -1,20 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-#include "Hal.h"
-
-using namespace mozilla::hal;
-
-namespace mozilla {
-namespace hal_impl {
-
-void
-SetCurrentThreadPriority(ThreadPriority aPriority)
-{
-  HAL_LOG(("FallbackThreadPriority - SetCurrentThreadPriority(%d)\n",
-           ThreadPriorityToString(aPriority)));
-}
-
-} // hal_impl
-} // namespace mozilla
--- a/hal/gonk/GonkHal.cpp
+++ b/hal/gonk/GonkHal.cpp
@@ -22,29 +22,27 @@
 #include <math.h>
 #include <regex.h>
 #include <stdio.h>
 #include <sys/klog.h>
 #include <sys/syscall.h>
 #include <sys/resource.h>
 #include <time.h>
 #include <asm/page.h>
-#include <sched.h>
 
 #include "mozilla/DebugOnly.h"
 
 #include "android/log.h"
 #include "cutils/properties.h"
 #include "hardware/hardware.h"
 #include "hardware/lights.h"
 #include "hardware_legacy/uevent.h"
 #include "hardware_legacy/vibrator.h"
 #include "hardware_legacy/power.h"
 #include "libdisplay/GonkDisplay.h"
-#include "utils/threads.h"
 
 #include "base/message_loop.h"
 
 #include "Hal.h"
 #include "HalImpl.h"
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/dom/battery/Constants.h"
 #include "mozilla/FileUtils.h"
@@ -1351,45 +1349,28 @@ SetNiceForPid(int aPid, int aNice)
       // (The |tidlong == aPid| check is very important; without it, we'll
       // renice aPid twice, and the second renice will be relative to the
       // priority set by the first renice.)
       continue;
     }
 
     int tid = static_cast<int>(tidlong);
 
-    // Do not set the priority of threads running with a real-time policy
-    // as part of the bulk process adjustment.  These threads need to run
-    // at their specified priority in order to meet timing guarantees.
-    int schedPolicy = sched_getscheduler(tid);
-    if (schedPolicy == SCHED_FIFO || schedPolicy == SCHED_RR) {
-      continue;
-    }
-
     errno = 0;
     // Get and set the task's new priority.
     int origtaskpriority = getpriority(PRIO_PROCESS, tid);
     if (errno) {
       LOG("Unable to get nice for tid=%d (pid=%d); error %d.  This isn't "
           "necessarily a problem; it could be a benign race condition.",
           tid, aPid, errno);
       continue;
     }
 
     int newtaskpriority =
       std::max(origtaskpriority - origProcPriority + aNice, aNice);
-
-    // Do not reduce priority of threads already running at priorities greater
-    // than normal.  These threads are likely special service threads that need
-    // elevated priorities to process audio, display composition, etc.
-    if (newtaskpriority > origtaskpriority &&
-        origtaskpriority < ANDROID_PRIORITY_NORMAL) {
-      continue;
-    }
-
     rv = setpriority(PRIO_PROCESS, tid, newtaskpriority);
 
     if (rv) {
       LOG("Unable to set nice for tid=%d (pid=%d); error %d.  This isn't "
           "necessarily a problem; it could be a benign race condition.",
           tid, aPid, errno);
       continue;
     }
@@ -1475,64 +1456,16 @@ SetProcessPriority(int aPid,
 
   if (NS_SUCCEEDED(rv)) {
     LOG("Setting nice for pid %d to %d", aPid, nice);
     SetNiceForPid(aPid, nice);
   }
 }
 
 void
-SetCurrentThreadPriority(ThreadPriority aPriority)
-{
-  int policy = SCHED_OTHER;
-  int priorityOrNice = ANDROID_PRIORITY_NORMAL;
-
-  switch(aPriority) {
-  case THREAD_PRIORITY_COMPOSITOR:
-    priorityOrNice = Preferences::GetInt("hal.gonk.compositor.rt_priority", 0);
-    if (priorityOrNice >= sched_get_priority_min(SCHED_FIFO) &&
-        priorityOrNice <= sched_get_priority_max(SCHED_FIFO)) {
-      policy = SCHED_FIFO;
-    } else {
-      priorityOrNice = Preferences::GetInt("hal.gonk.compositor.nice",
-                                           ANDROID_PRIORITY_URGENT_DISPLAY);
-    }
-    break;
-  default:
-    LOG("Unrecognized thread priority %d; Doing nothing", aPriority);
-    return;
-  }
-
-  int tid = gettid();
-  int rv = 0;
-
-  // If a RT scheduler policy is used, then we must set the priority using
-  // sched_setscheduler() and the sched_param.sched_priority value.
-  if (policy == SCHED_FIFO || policy == SCHED_RR) {
-    LOG("Setting thread %d to priority level %s; RT priority %d",
-        tid, ThreadPriorityToString(aPriority), priorityOrNice);
-    sched_param schedParam;
-    schedParam.sched_priority = priorityOrNice;
-    rv = sched_setscheduler(tid, policy, &schedParam);
-
-  // Otherwise priority is solely defined by the nice level, so use the
-  // setpriority() function.
-  } else {
-    LOG("Setting thread %d to priority level %s; nice level %d",
-        tid, ThreadPriorityToString(aPriority), priorityOrNice);
-    rv = setpriority(PRIO_PROCESS, tid, priorityOrNice);
-  }
-
-  if (rv) {
-    LOG("Failed to set thread %d to priority level %s; error code %d",
-        tid, ThreadPriorityToString(aPriority), rv);
-  }
-}
-
-void
 FactoryReset()
 {
   nsCOMPtr<nsIRecoveryService> recoveryService =
     do_GetService("@mozilla.org/recovery-service;1");
   if (!recoveryService) {
     NS_WARNING("Could not get recovery service!");
     return;
   }
--- a/hal/moz.build
+++ b/hal/moz.build
@@ -146,17 +146,16 @@ if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk
     UNIFIED_SOURCES += [
         'fallback/FallbackDiskSpaceWatcher.cpp',
         'fallback/FallbackFactoryReset.cpp',
         'fallback/FallbackFMRadio.cpp',
         'fallback/FallbackLights.cpp',
         'fallback/FallbackProcessPriority.cpp',
         'fallback/FallbackScreenPower.cpp',
         'fallback/FallbackSwitch.cpp',
-        'fallback/FallbackThreadPriority.cpp',
         'fallback/FallbackTime.cpp',
         'fallback/FallbackWakeLocks.cpp',
     ]
 
 # Fallbacks for backends implemented on Android only.
 if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
     UNIFIED_SOURCES += [
         'fallback/FallbackNetwork.cpp',
--- a/hal/sandbox/SandboxHal.cpp
+++ b/hal/sandbox/SandboxHal.cpp
@@ -358,22 +358,16 @@ SetProcessPriority(int aPid,
                    ProcessPriority aPriority,
                    ProcessCPUPriority aCPUPriority,
                    uint32_t aBackgroundLRU)
 {
   NS_RUNTIMEABORT("Only the main process may set processes' priorities.");
 }
 
 void
-SetCurrentThreadPriority(ThreadPriority aPriority)
-{
-  NS_RUNTIMEABORT("Only the main process may set thread priorities.");
-}
-
-void
 EnableFMRadio(const hal::FMRadioSettings& aSettings)
 {
   NS_RUNTIMEABORT("FM radio cannot be called from sandboxed contexts.");
 }
 
 void
 DisableFMRadio()
 {