Bug 1166320 - Make volume service safer to use off main thread. r=dhylands
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 03 Jun 2015 13:44:53 -0400
changeset 269613 371c3a42b25c1ccc576bb4923c4b47da4d318143
parent 269612 2fb400ee696e693f989a8b228f0ea4f03c73aebb
child 269614 2e166ab30db122e945afda7e67eeca7a22b99f49
push id2540
push userwcosta@mozilla.com
push dateWed, 03 Jun 2015 20:55:41 +0000
reviewersdhylands
bugs1166320
milestone41.0a1
Bug 1166320 - Make volume service safer to use off main thread. r=dhylands
dom/system/gonk/nsVolumeService.cpp
dom/system/gonk/nsVolumeService.h
--- a/dom/system/gonk/nsVolumeService.cpp
+++ b/dom/system/gonk/nsVolumeService.cpp
@@ -363,81 +363,63 @@ nsVolumeService::FindVolumeByMountLockNa
     if (mountLockName.Equals(aMountLockName)) {
       return vol.forget();
     }
   }
   return nullptr;
 }
 
 already_AddRefed<nsVolume>
-nsVolumeService::FindVolumeByName(const nsAString& aName)
+nsVolumeService::FindVolumeByName(const nsAString& aName, nsVolume::Array::index_type* aIndex)
 {
   mArrayMonitor.AssertCurrentThreadOwns();
 
   nsVolume::Array::size_type numVolumes = mVolumeArray.Length();
   nsVolume::Array::index_type volIndex;
   for (volIndex = 0; volIndex < numVolumes; volIndex++) {
     nsRefPtr<nsVolume> vol = mVolumeArray[volIndex];
     if (vol->Name().Equals(aName)) {
+      if (aIndex) {
+        *aIndex = volIndex;
+      }
       return vol.forget();
     }
   }
   return nullptr;
 }
 
-//static
-already_AddRefed<nsVolume>
-nsVolumeService::CreateOrFindVolumeByName(const nsAString& aName, bool aIsFake /*= false*/)
-{
-  MonitorAutoLock autoLock(mArrayMonitor);
-
-  nsRefPtr<nsVolume> vol;
-  vol = FindVolumeByName(aName);
-  if (vol) {
-    return vol.forget();
-  }
-  // Volume not found - add a new one
-  vol = new nsVolume(aName);
-  vol->SetIsFake(aIsFake);
-  mVolumeArray.AppendElement(vol);
-  return vol.forget();
-}
-
 void
-nsVolumeService::UpdateVolume(nsIVolume* aVolume, bool aNotifyObservers)
+nsVolumeService::UpdateVolume(nsVolume* aVolume, bool aNotifyObservers)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
-  nsString volName;
-  aVolume->GetName(volName);
-  bool aIsFake;
-  aVolume->GetIsFake(&aIsFake);
-  nsRefPtr<nsVolume> vol = CreateOrFindVolumeByName(volName, aIsFake);
-  if (vol->Equals(aVolume)) {
-    // Nothing has really changed. Don't bother telling anybody.
-    return;
+  {
+    MonitorAutoLock autoLock(mArrayMonitor);
+    nsVolume::Array::index_type volIndex;
+    nsRefPtr<nsVolume> vol = FindVolumeByName(aVolume->Name(), &volIndex);
+    if (!vol) {
+      mVolumeArray.AppendElement(aVolume);
+    } else if (vol->Equals(aVolume) || (!vol->IsFake() && aVolume->IsFake())) {
+      // Ignore if nothing changed or if a fake tries to override a real volume.
+      return;
+    } else {
+      mVolumeArray.ReplaceElementAt(volIndex, aVolume);
+    }
   }
 
-  if (!vol->IsFake() && aIsFake) {
-    // Prevent an incoming fake volume from overriding an existing real volume.
-    return;
-  }
-
-  vol->Set(aVolume);
-
   if (!aNotifyObservers) {
     return;
   }
 
   nsCOMPtr<nsIObserverService> obs = GetObserverService();
   if (!obs) {
     return;
   }
-  NS_ConvertUTF8toUTF16 stateStr(vol->StateStr());
-  obs->NotifyObservers(vol, NS_VOLUME_STATE_CHANGED, stateStr.get());
+  NS_ConvertUTF8toUTF16 stateStr(aVolume->StateStr());
+  obs->NotifyObservers(aVolume, NS_VOLUME_STATE_CHANGED, stateStr.get());
 }
 
 NS_IMETHODIMP
 nsVolumeService::CreateFakeVolume(const nsAString& name, const nsAString& path)
 {
   if (XRE_GetProcessType() == GeckoProcessType_Default) {
     nsRefPtr<nsVolume> vol = new nsVolume(name, path, nsIVolume::STATE_INIT,
                                           -1    /* mountGeneration */,
@@ -466,19 +448,17 @@ nsVolumeService::SetFakeVolumeState(cons
     {
       MonitorAutoLock autoLock(mArrayMonitor);
       vol = FindVolumeByName(name);
     }
     if (!vol || !vol->IsFake()) {
       return NS_ERROR_NOT_AVAILABLE;
     }
 
-    // UpdateVolume expects the volume passed in to NOT be the
-    // same pointer as what CreateOrFindVolumeByName would return,
-    // which is why we allocate a temporary volume here.
+    // Clone the existing volume so we can replace it
     nsRefPtr<nsVolume> volume = new nsVolume(name);
     volume->Set(vol);
     volume->SetState(state);
     volume->LogState();
     UpdateVolume(volume.get());
     return NS_OK;
   }
 
@@ -497,25 +477,25 @@ nsVolumeService::RemoveFakeVolume(const 
 
   ContentChild::GetSingleton()->SendRemoveFakeVolume(nsString(name));
   return NS_OK;
 }
 
 void
 nsVolumeService::RemoveVolumeByName(const nsAString& aName)
 {
-  nsRefPtr<nsVolume> vol;
   {
     MonitorAutoLock autoLock(mArrayMonitor);
-    vol = FindVolumeByName(aName);
+    nsVolume::Array::index_type volIndex;
+    nsRefPtr<nsVolume> vol = FindVolumeByName(aName, &volIndex);
+    if (!vol) {
+      return;
+    }
+    mVolumeArray.RemoveElementAt(volIndex);
   }
-  if (!vol) {
-    return;
-  }
-  mVolumeArray.RemoveElement(vol);
 
   if (XRE_GetProcessType() == GeckoProcessType_Default) {
     nsCOMPtr<nsIObserverService> obs = GetObserverService();
     if (!obs) {
       return;
     }
     obs->NotifyObservers(nullptr, NS_VOLUME_REMOVED, nsString(aName).get());
   }
--- a/dom/system/gonk/nsVolumeService.h
+++ b/dom/system/gonk/nsVolumeService.h
@@ -42,32 +42,33 @@ public:
 
   static already_AddRefed<nsVolumeService> GetSingleton();
   //static nsVolumeService* GetSingleton();
   static void Shutdown();
 
   void DumpNoLock(const char* aLabel);
 
   // To use this function, you have to create a new volume and pass it in.
-  void UpdateVolume(nsIVolume* aVolume, bool aNotifyObservers = true);
+  void UpdateVolume(nsVolume* aVolume, bool aNotifyObservers = true);
   void UpdateVolumeIOThread(const Volume* aVolume);
 
   void RecvVolumesFromParent(const nsTArray<dom::VolumeInfo>& aVolumes);
   void GetVolumesForIPC(nsTArray<dom::VolumeInfo>* aResult);
 
   void RemoveVolumeByName(const nsAString& aName);
 
 private:
   ~nsVolumeService();
 
   void CheckMountLock(const nsAString& aMountLockName,
                       const nsAString& aMountLockState);
   already_AddRefed<nsVolume> FindVolumeByMountLockName(const nsAString& aMountLockName);
-  already_AddRefed<nsVolume> FindVolumeByName(const nsAString& aName);
-  already_AddRefed<nsVolume> CreateOrFindVolumeByName(const nsAString& aName, bool aIsFake = false);
+
+  already_AddRefed<nsVolume> FindVolumeByName(const nsAString& aName,
+                                              nsVolume::Array::index_type* aIndex = nullptr);
 
   Monitor mArrayMonitor;
   nsVolume::Array mVolumeArray;
 
   static StaticRefPtr<nsVolumeService> sSingleton;
   bool mGotVolumesFromParent;
 };