Bug 870480 - Notify wake lock listeners when a process adds or removes a wake lock, even if the wake lock itself doesn't change state. r=kanru
authorJustin Lebar <justin.lebar@gmail.com>
Fri, 10 May 2013 11:20:52 -0400
changeset 142459 150241980e66d828ea9803db901f3f15cda134f0
parent 142458 41ca6c75671e336bd89c83f322e578f720b2cf7e
child 142460 aee46daa226279adc52e2f30f1b825418cfb9f0e
push idunknown
push userunknown
push dateunknown
reviewerskanru
bugs870480
milestone23.0a1
Bug 870480 - Notify wake lock listeners when a process adds or removes a wake lock, even if the wake lock itself doesn't change state. r=kanru This also fixes two other bugs: 1) If a process crashed while holding a wake lock, we'd send a wake lock changed notification without including the list of locking processes. This could cause us to incorrectly believe that no processes were holding the lock. 2) If a process crashed while holding a wake lock on topic X, and if no other process held a wake lock on that topic, and there were no wake lock listeners registered, we wouldn't remove X from our hashtable of wake lock topics. We would correctly record the fact that the lock was not held, so this bug was benign.
hal/HalWakeLock.cpp
hal/sandbox/PHal.ipdl
--- a/hal/HalWakeLock.cpp
+++ b/hal/HalWakeLock.cpp
@@ -9,58 +9,57 @@
 #include "mozilla/StaticPtr.h"
 #include "mozilla/dom/ContentParent.h"
 #include "nsClassHashtable.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "nsIPropertyBag2.h"
 #include "nsObserverService.h"
 
+using namespace mozilla;
 using namespace mozilla::hal;
 
-namespace mozilla {
-namespace hal {
+namespace {
 
-WakeLockState
-ComputeWakeLockState(int aNumLocks, int aNumHidden)
-{
-  if (aNumLocks == 0) {
-    return WAKE_LOCK_STATE_UNLOCKED;
-  } else if (aNumLocks == aNumHidden) {
-    return WAKE_LOCK_STATE_HIDDEN;
-  } else {
-    return WAKE_LOCK_STATE_VISIBLE;
-  }
-}
-
-} // hal
-} // mozilla
-
-namespace mozilla {
-namespace hal_impl {
-
-namespace {
 struct LockCount {
   LockCount()
     : numLocks(0)
     , numHidden(0)
   {}
   uint32_t numLocks;
   uint32_t numHidden;
   nsTArray<uint64_t> processes;
 };
+
 typedef nsDataHashtable<nsUint64HashKey, LockCount> ProcessLockTable;
 typedef nsClassHashtable<nsStringHashKey, ProcessLockTable> LockTable;
 
-static int sActiveListeners = 0;
-static StaticAutoPtr<LockTable> sLockTable;
-static bool sInitialized = false;
-static bool sIsShuttingDown = false;
+int sActiveListeners = 0;
+StaticAutoPtr<LockTable> sLockTable;
+bool sInitialized = false;
+bool sIsShuttingDown = false;
 
-static PLDHashOperator
+WakeLockInformation
+WakeLockInfoFromLockCount(const nsAString& aTopic, const LockCount& aLockCount)
+{
+  // TODO: Once we abandon b2g18, we can switch this to use the
+  // WakeLockInformation constructor, which is better because it doesn't let us
+  // forget to assign a param.  For now we have to do it this way, because
+  // b2g18 doesn't have the nsTArray <--> InfallibleTArray conversion (bug
+  // 819791).
+
+  WakeLockInformation info;
+  info.topic() = aTopic;
+  info.numLocks() = aLockCount.numLocks;
+  info.numHidden() = aLockCount.numHidden;
+  info.lockingProcesses().AppendElements(aLockCount.processes);
+  return info;
+}
+
+PLDHashOperator
 CountWakeLocks(const uint64_t& aKey, LockCount aCount, void* aUserArg)
 {
   MOZ_ASSERT(aUserArg);
 
   LockCount* totalCount = static_cast<LockCount*>(aUserArg);
   totalCount->numLocks += aCount.numLocks;
   totalCount->numHidden += aCount.numHidden;
 
@@ -75,29 +74,27 @@ CountWakeLocks(const uint64_t& aKey, Loc
 static PLDHashOperator
 RemoveChildFromList(const nsAString& aKey, nsAutoPtr<ProcessLockTable>& aTable,
                     void* aUserArg)
 {
   MOZ_ASSERT(aUserArg);
 
   PLDHashOperator op = PL_DHASH_NEXT;
   uint64_t childID = *static_cast<uint64_t*>(aUserArg);
-  if (aTable->Get(childID, NULL)) {
+  if (aTable->Get(childID, nullptr)) {
     aTable->Remove(childID);
+
+    LockCount totalCount;
+    aTable->EnumerateRead(CountWakeLocks, &totalCount);
+    if (!totalCount.numLocks) {
+      op = PL_DHASH_REMOVE;
+    }
+
     if (sActiveListeners) {
-      LockCount totalCount;
-      WakeLockInformation info;
-      aTable->EnumerateRead(CountWakeLocks, &totalCount);
-      if (!totalCount.numLocks) {
-        op = PL_DHASH_REMOVE;
-      }
-      info.numLocks() = totalCount.numLocks;
-      info.numHidden() = totalCount.numHidden;
-      info.topic() = aKey;
-      NotifyWakeLockChange(info);
+      NotifyWakeLockChange(WakeLockInfoFromLockCount(aKey, totalCount));
     }
   }
 
   return op;
 }
 
 class ClearHashtableOnShutdown MOZ_FINAL : public nsIObserver {
 public:
@@ -147,31 +144,52 @@ CleanupOnContentShutdown::Observe(nsISup
   if (NS_SUCCEEDED(rv)) {
     sLockTable->Enumerate(RemoveChildFromList, &childID);
   } else {
     NS_WARNING("ipc:content-shutdown message without childID property");
   }
   return NS_OK;
 }
 
-static void
+void
 Init()
 {
   sLockTable = new LockTable();
   sLockTable->Init();
   sInitialized = true;
 
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
   if (obs) {
     obs->AddObserver(new ClearHashtableOnShutdown(), "xpcom-shutdown", false);
     obs->AddObserver(new CleanupOnContentShutdown(), "ipc:content-shutdown", false);
   }
 }
+
 } // anonymous namespace
 
+namespace mozilla {
+
+namespace hal {
+
+WakeLockState
+ComputeWakeLockState(int aNumLocks, int aNumHidden)
+{
+  if (aNumLocks == 0) {
+    return WAKE_LOCK_STATE_UNLOCKED;
+  } else if (aNumLocks == aNumHidden) {
+    return WAKE_LOCK_STATE_HIDDEN;
+  } else {
+    return WAKE_LOCK_STATE_VISIBLE;
+  }
+}
+
+} // namespace hal
+
+namespace hal_impl {
+
 void
 EnableWakeLockNotifications()
 {
   sActiveListeners++;
 }
 
 void
 DisableWakeLockNotifications()
@@ -210,61 +228,60 @@ ModifyWakeLock(const nsAString& aTopic,
   MOZ_ASSERT(processCount.numLocks >= processCount.numHidden);
   MOZ_ASSERT(aLockAdjust >= 0 || processCount.numLocks > 0);
   MOZ_ASSERT(aHiddenAdjust >= 0 || processCount.numHidden > 0);
   MOZ_ASSERT(totalCount.numLocks >= totalCount.numHidden);
   MOZ_ASSERT(aLockAdjust >= 0 || totalCount.numLocks > 0);
   MOZ_ASSERT(aHiddenAdjust >= 0 || totalCount.numHidden > 0);
 
   WakeLockState oldState = ComputeWakeLockState(totalCount.numLocks, totalCount.numHidden);
+  bool processWasLocked = processCount.numLocks > 0;
 
   processCount.numLocks += aLockAdjust;
   processCount.numHidden += aHiddenAdjust;
 
   totalCount.numLocks += aLockAdjust;
   totalCount.numHidden += aHiddenAdjust;
 
   if (processCount.numLocks) {
     table->Put(aProcessID, processCount);
   } else {
     table->Remove(aProcessID);
   }
   if (!totalCount.numLocks) {
     sLockTable->Remove(aTopic);
   }
 
-  WakeLockState newState = ComputeWakeLockState(totalCount.numLocks, totalCount.numHidden);
+  if (sActiveListeners &&
+      (oldState != ComputeWakeLockState(totalCount.numLocks,
+                                        totalCount.numHidden) ||
+       processWasLocked != processCount.numLocks > 0)) {
 
-  if (sActiveListeners && oldState != newState) {
     WakeLockInformation info;
     GetWakeLockInfo(aTopic, &info);
     NotifyWakeLockChange(info);
   }
 }
 
 void
 GetWakeLockInfo(const nsAString& aTopic, WakeLockInformation* aWakeLockInfo)
 {
   if (sIsShuttingDown) {
     NS_WARNING("You don't want to get wake lock information during xpcom-shutdown!");
+    *aWakeLockInfo = WakeLockInformation();
     return;
   }
   if (!sInitialized) {
     Init();
   }
 
   ProcessLockTable* table = sLockTable->Get(aTopic);
   if (!table) {
-    aWakeLockInfo->numLocks() = 0;
-    aWakeLockInfo->numHidden() = 0;
-    aWakeLockInfo->topic() = aTopic;
+    *aWakeLockInfo = WakeLockInfoFromLockCount(aTopic, LockCount());
     return;
   }
   LockCount totalCount;
   table->EnumerateRead(CountWakeLocks, &totalCount);
-  aWakeLockInfo->numLocks() = totalCount.numLocks;
-  aWakeLockInfo->numHidden() = totalCount.numHidden;
-  aWakeLockInfo->lockingProcesses() = totalCount.processes;
-  aWakeLockInfo->topic() = aTopic;
+  *aWakeLockInfo = WakeLockInfoFromLockCount(aTopic, totalCount);
 }
 
 } // hal_impl
 } // mozilla
--- a/hal/sandbox/PHal.ipdl
+++ b/hal/sandbox/PHal.ipdl
@@ -59,20 +59,20 @@ struct NetworkInformation {
 };
 
 struct SwitchEvent {
   SwitchDevice device;
   SwitchState status;
 };
 
 struct WakeLockInformation {
+  nsString topic;
   uint32_t numLocks;
   uint32_t numHidden;
   uint64_t[] lockingProcesses;
-  nsString topic;
 };
 
 struct ScreenConfiguration {
   nsIntRect rect;
   ScreenOrientation orientation;
   uint32_t colorDepth;
   uint32_t pixelDepth;
 };