Bug 1089619 - Stop abusing the Settings API from Gonk GPS. r=kanru
☠☠ backed out by 374ebf41e67c ☠ ☠
authorAlexandre Lissy <lissyx@lissyx.dyndns.org>
Thu, 30 Oct 2014 01:56:00 +0100
changeset 213173 6797040a2034f95629cbea00f44014fce9e0fb1b
parent 213172 b4665be856d700582b246da370b515271ba280a2
child 213174 10962f2deab176cd40b85e70fb80a18d164f36ea
push id27742
push userryanvm@gmail.com
push dateThu, 30 Oct 2014 20:15:35 +0000
treeherdermozilla-central@e0b505a37b1c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskanru
bugs1089619
milestone36.0a1
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
Bug 1089619 - Stop abusing the Settings API from Gonk GPS. r=kanru GPS handling for Gonk abuses the Settings API in two ways. First, on mozsettings-changed event, it will trigger two new requests to read values (debug enabled and GPS locations ignore). This is useless since the event already contains the key that has changed and the new value, so there is no need to do a createLock().get() call. Then, in startup code, the Init() method is supposed to check itself whether it is already running. This is done through the mStarted boolean. The same Init() method is responsible for adding the mozsettings-changed observer, which is removed by the Shutdown() method. Investigation on device by using the Geolocation API has proven that we were leaking some observers. This is because checking mStarted boolean is performed after we request settings values and we install the mozsettings-changed observer. So any time the Init() gets called, we install it but we just remove it once in Shutdown().
dom/system/gonk/GonkGPSGeolocationProvider.cpp
--- a/dom/system/gonk/GonkGPSGeolocationProvider.cpp
+++ b/dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ -27,16 +27,17 @@
 #include "nsIInterfaceRequestorUtils.h"
 #include "nsINetworkManager.h"
 #include "nsIObserverService.h"
 #include "nsJSUtils.h"
 #include "nsPrintfCString.h"
 #include "nsServiceManagerUtils.h"
 #include "nsThreadUtils.h"
 #include "prtime.h"
+#include "mozilla/dom/SettingChangeNotificationBinding.h"
 
 #ifdef MOZ_B2G_RIL
 #include "nsIIccInfo.h"
 #include "nsIMobileConnectionInfo.h"
 #include "nsIMobileConnectionService.h"
 #include "nsIMobileCellInfo.h"
 #include "nsIRadioInterfaceLayer.h"
 #endif
@@ -395,19 +396,31 @@ void
 GonkGPSGeolocationProvider::RequestSettingValue(const char* aKey)
 {
   MOZ_ASSERT(aKey);
   nsCOMPtr<nsISettingsService> ss = do_GetService("@mozilla.org/settingsService;1");
   if (!ss) {
     MOZ_ASSERT(ss);
     return;
   }
+
   nsCOMPtr<nsISettingsServiceLock> lock;
-  ss->CreateLock(nullptr, getter_AddRefs(lock));
-  lock->Get(aKey, this);
+  nsresult rv = ss->CreateLock(nullptr, getter_AddRefs(lock));
+  if (NS_FAILED(rv)) {
+    nsContentUtils::LogMessageToConsole(
+      "geo: error while createLock setting '%s': %d\n", aKey, rv);
+    return;
+  }
+
+  rv = lock->Get(aKey, this);
+  if (NS_FAILED(rv)) {
+    nsContentUtils::LogMessageToConsole(
+      "geo: error while get setting '%s': %d\n", aKey, rv);
+    return;
+  }
 }
 
 #ifdef MOZ_B2G_RIL
 void
 GonkGPSGeolocationProvider::RequestDataConnection()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -809,32 +822,32 @@ GonkGPSGeolocationProvider::NetworkLocat
   return NS_OK;
 }
 
 NS_IMETHODIMP
 GonkGPSGeolocationProvider::Startup()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (mStarted) {
+    return NS_OK;
+  }
+
   RequestSettingValue(kSettingDebugEnabled);
   RequestSettingValue(kSettingDebugGpsIgnored);
 
   // Setup an observer to watch changes to the setting.
   nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
   if (observerService) {
     nsresult rv = observerService->AddObserver(this, kMozSettingsChangedTopic, false);
     if (NS_FAILED(rv)) {
       NS_WARNING("geo: Gonk GPS AddObserver failed");
     }
   }
 
-  if (mStarted) {
-    return NS_OK;
-  }
-
   if (!mInitThread) {
     nsresult rv = NS_NewThread(getter_AddRefs(mInitThread));
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   mInitThread->Dispatch(NS_NewRunnableMethod(this, &GonkGPSGeolocationProvider::Init),
                         NS_DISPATCH_NORMAL);
 
@@ -863,28 +876,36 @@ GonkGPSGeolocationProvider::Watch(nsIGeo
 NS_IMETHODIMP
 GonkGPSGeolocationProvider::Shutdown()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (!mStarted) {
     return NS_OK;
   }
+
   mStarted = false;
   if (mNetworkLocationProvider) {
     mNetworkLocationProvider->Shutdown();
     mNetworkLocationProvider = nullptr;
   }
 
   nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
   if (obs) {
+    nsresult rv;
 #ifdef MOZ_B2G_RIL
-    obs->RemoveObserver(this, kNetworkConnStateChangedTopic);
+    rv = obs->RemoveObserver(this, kNetworkConnStateChangedTopic);
+    if (NS_FAILED(rv)) {
+      NS_WARNING("geo: Gonk GPS network state RemoveObserver failed");
+    }
 #endif
-    obs->RemoveObserver(this, kMozSettingsChangedTopic);
+    rv = obs->RemoveObserver(this, kMozSettingsChangedTopic);
+    if (NS_FAILED(rv)) {
+      NS_WARNING("geo: Gonk GPS mozsettings RemoveObserver failed");
+    }
   }
 
   mInitThread->Dispatch(NS_NewRunnableMethod(this, &GonkGPSGeolocationProvider::ShutdownGPS),
                         NS_DISPATCH_NORMAL);
 
   return NS_OK;
 }
 
@@ -986,18 +1007,40 @@ GonkGPSGeolocationProvider::Observe(nsIS
       return NS_OK;
     }
 
     RequestSettingValue("ril.supl.apn");
   }
 #endif
 
   if (!strcmp(aTopic, kMozSettingsChangedTopic)) {
-    RequestSettingValue(kSettingDebugEnabled);
-    RequestSettingValue(kSettingDebugGpsIgnored);
+    // Read changed setting value
+    AutoJSAPI jsapi;
+    jsapi.Init();
+    JSContext* cx = jsapi.cx();
+    RootedDictionary<SettingChangeNotification> setting(cx);
+    if (!WrappedJSToDictionary(cx, aSubject, setting)) {
+      return NS_OK;
+    }
+
+    if (setting.mKey.EqualsASCII(kSettingDebugGpsIgnored)) {
+      nsContentUtils::LogMessageToConsole("geo: received mozsettings-changed: ignoring\n");
+      gDebug_isGPSLocationIgnored =
+        setting.mValue.isBoolean() ? setting.mValue.toBoolean() : false;
+      if (gDebug_isLoggingEnabled) {
+        nsContentUtils::LogMessageToConsole("geo: Debug: GPS ignored %d\n",
+                                            gDebug_isGPSLocationIgnored);
+      }
+      return NS_OK;
+    } else if (setting.mKey.EqualsASCII(kSettingDebugEnabled)) {
+      nsContentUtils::LogMessageToConsole("geo: received mozsettings-changed: logging\n");
+      gDebug_isLoggingEnabled =
+        setting.mValue.isBoolean() ? setting.mValue.toBoolean() : false;
+      return NS_OK;
+    }
   }
 
   return NS_OK;
 }
 
 /** nsISettingsServiceCallback **/
 
 NS_IMETHODIMP
@@ -1015,28 +1058,18 @@ GonkGPSGeolocationProvider::Handle(const
       nsAutoJSString apn;
       if (!apn.init(cx, aResult.toString())) {
         return NS_ERROR_FAILURE;
       }
       if (!apn.IsEmpty()) {
         SetAGpsDataConn(apn);
       }
     }
-  } else
+  }
 #endif // MOZ_B2G_RIL
-  if (aName.EqualsASCII(kSettingDebugGpsIgnored)) {
-    gDebug_isGPSLocationIgnored = aResult.isBoolean() ? aResult.toBoolean() : false;
-    if (gDebug_isLoggingEnabled) {
-      nsContentUtils::LogMessageToConsole("geo: Debug: GPS ignored %d\n", gDebug_isGPSLocationIgnored);
-    }
-    return NS_OK;
-  } else if (aName.EqualsASCII(kSettingDebugEnabled)) {
-    gDebug_isLoggingEnabled = aResult.isBoolean() ? aResult.toBoolean() : false;
-    return NS_OK;
-  }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 GonkGPSGeolocationProvider::HandleError(const nsAString& aErrorMessage)
 {
   return NS_OK;
 }