Fix dbus- and geolocation- related crash on Linux.

The NetworkManagerWlanApi class in wifi_data_provider_linux.cc was
using a shared dbus connection which was referenced as a GSource in
the glib message loop in the main thread, and also from the
WifiDataProviderCommon thread. When the dbus proxy object and connection
was unreferenced in the WifiDataCommon thread, the glib main loop in the
main thread could concurrently reference it during object destruction,
causing a crash.

We fix this by using a private dbus connection instead, referencing its
own glib main loop separate to that of the main thread.

BUG=59913
TEST=GeolocationWifiDataProviderPlatformTest.CreateAndDestroy

Review URL: http://codereview.chromium.org/4307001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65185 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 99 insertions, 13 deletions

diff --git a/chrome/browser/geolocation/device_data_provider_unittest.cc b/chrome/browser/geolocation/device_data_provider_unittest.cc
new file mode 100644
index 0000000..f6e9d96
--- /dev/null
+++ b/chrome/browser/geolocation/device_data_provider_unittest.cc
@@ -0,0 +1,39 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/geolocation/device_data_provider.h"
+
+#include "chrome/browser/geolocation/wifi_data_provider_common.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+class NullWifiDataListenerInterface
+    : public WifiDataProviderCommon::ListenerInterface {
+ public:
+  // ListenerInterface
+  virtual void DeviceDataUpdateAvailable(
+      DeviceDataProvider<WifiData>* provider) {}
+};
+}
+
+TEST(GeolocationDeviceDataProviderWifiData, CreateDestroy) {
+  // See http://crbug.com/59913 .  The main_message_loop is not required to be
+  // run for correct behaviour, but we run it in this test to help smoke out
+  // any race conditions between processing in the main loop and the setup /
+  // tear down of the DeviceDataProvider thread.
+  MessageLoopForUI main_message_loop;
+  NullWifiDataListenerInterface listener;
+  for (int i = 0; i < 10; i++) {
+    DeviceDataProvider<WifiData>::Register(&listener);
+    for (int j = 0; j < 10; j++) {
+      PlatformThread::Sleep(0);
+      main_message_loop.RunAllPending();  // See comment above
+    }
+    DeviceDataProvider<WifiData>::Unregister(&listener);
+    for (int j = 0; j < 10; j++) {
+      PlatformThread::Sleep(0);
+      main_message_loop.RunAllPending();  // See comment above
+    }
+  }
+}
diff --git a/chrome/browser/geolocation/wifi_data_provider_common_unittest.cc b/chrome/browser/geolocation/wifi_data_provider_common_unittest.cc
index b830040..256fbcb 100644
--- a/chrome/browser/geolocation/wifi_data_provider_common_unittest.cc
+++ b/chrome/browser/geolocation/wifi_data_provider_common_unittest.cc
@@ -216,7 +216,6 @@ TEST_F(GeolocationWifiDataProviderCommonTest, DoScanWithResults) {
   EXPECT_EQ(single_access_point.ssid, data.access_point_data.begin()->ssid);
 }
 
-// TODO(Joth) Convert to gmock style
 TEST_F(GeolocationWifiDataProviderCommonTest,
        StartThreadViaDeviceDataProvider) {
   MessageLoopQuitListener quit_listener(&main_message_loop_);
diff --git a/chrome/browser/geolocation/wifi_data_provider_linux.cc b/chrome/browser/geolocation/wifi_data_provider_linux.cc
index 6c043e0..5c0fbb0 100644
--- a/chrome/browser/geolocation/wifi_data_provider_linux.cc
+++ b/chrome/browser/geolocation/wifi_data_provider_linux.cc
@@ -11,6 +11,7 @@
 #include <dbus/dbus-glib.h>
 #include <dbus/dbus-glib-lowlevel.h>
 #include <dbus/dbus.h>
+#include <dlfcn.h>
 #include <glib.h>
 
 #include "base/scoped_ptr.h"
@@ -31,6 +32,12 @@ const char kNetworkManagerInterface[] = "org.freedesktop.NetworkManager";
 // From http://projects.gnome.org/NetworkManager/developers/spec.html
 enum { NM_DEVICE_TYPE_WIFI = 2 };
 
+// Function type matching dbus_g_bus_get_private so that we can
+// dynamically determine the presence of this symbol (see
+// NetworkManagerWlanApi::Init)
+typedef DBusGConnection* DBusGBusGetPrivateFunc(
+    DBusBusType type, GMainContext* context, GError** error);
+
 // Utility wrappers to make various GLib & DBus structs into scoped objects.
 class ScopedGPtrArrayFree {
  public:
@@ -69,6 +76,23 @@ class ScopedGValue {
   GValue v;
 };
 
+// Use ScopedDLHandle to automatically clean up after dlopen.
+class ScopedDLHandle {
+ public:
+  ScopedDLHandle(void* handle)
+      : handle_(handle) {
+  }
+  ~ScopedDLHandle() {
+    if (handle_)
+      dlclose(handle_);
+  }
+  void* get() {
+    return handle_;
+  }
+ private:
+  void *handle_;
+};
+
 // Wifi API binding to NetworkManager, to allow reuse of the polling behavior
 // defined in WifiDataProviderCommon.
 // TODO(joth): NetworkManager also allows for notification based handling,
@@ -115,7 +139,9 @@ class NetworkManagerWlanApi : public WifiDataProviderCommon::WlanApiInterface {
   GError* error_;
   // Connection to the dbus system bus.
   DBusGConnection* connection_;
-  // Proxy to the network maanger dbus service.
+  // Main context
+  GMainContext* context_;
+  // Proxy to the network manager dbus service.
   ScopedDBusGProxyPtr proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkManagerWlanApi);
@@ -135,14 +161,21 @@ int frquency_in_khz_to_channel(int frequency_khz) {
 }
 
 NetworkManagerWlanApi::NetworkManagerWlanApi()
-    : error_(NULL), connection_(NULL) {
+    : error_(NULL),
+      connection_(NULL),
+      context_(NULL)
+{
 }
 
 NetworkManagerWlanApi::~NetworkManagerWlanApi() {
   proxy_.reset();
   if (connection_) {
+    dbus_connection_close(dbus_g_connection_get_connection(connection_));
     dbus_g_connection_unref(connection_);
   }
+  if (context_)
+    g_main_context_unref(context_);
+
   DCHECK(!error_) << "Missing a call to CheckError() to clear |error_|";
 }
 
@@ -151,19 +184,35 @@ bool NetworkManagerWlanApi::Init() {
   // get caught up with that nonsense here, lets just assert our requirement.
   CHECK(g_thread_supported());
 
-  // Get a connection to the session bus.
-  connection_ = dbus_g_bus_get(DBUS_BUS_SYSTEM, &error_);
+  // We require a private bus to ensure that we don't interfere with the
+  // default loop on the main thread.  Unforunately this functionality is only
+  // available in dbus-glib-0.84 and later. We do a dynamic symbol lookup
+  // to determine if dbus_g_bus_get_private is available. See bug
+  // http://code.google.com/p/chromium/issues/detail?id=59913 for more
+  // information.
+  ScopedDLHandle handle(dlopen(NULL, RTLD_LAZY));
+  if (!handle.get())
+    return false;
+
+  DBusGBusGetPrivateFunc *my_dbus_g_bus_get_private =
+      (DBusGBusGetPrivateFunc *) dlsym(handle.get(), "dbus_g_bus_get_private");
+
+  if (!my_dbus_g_bus_get_private) {
+    LOG(ERROR) << "We need dbus-glib >= 0.84 for wifi geolocation.";
+    return false;
+  }
+  // Get a private connection to the session bus.
+  context_ = g_main_context_new();
+  DCHECK(context_);
+  connection_ = my_dbus_g_bus_get_private(DBUS_BUS_SYSTEM, context_, &error_);
+
   if (CheckError())
     return false;
   DCHECK(connection_);
 
-  // dbus-glib queues timers that get fired on the default loop, unfortunately
-  // it isn't thread safe in it's handling of these timers. We can't easily
-  // tell it which loop to queue them on instead, but as we only make
-  // blocking sync calls we don't need timers anyway, so disable them.
-  // See http://crbug.com/40803 TODO(joth): This is not an ideal solution, as
-  // we're reconfiguring the process global system bus connection, so could
-  // impact other users of DBus.
+  // Disable timers on the connection since we don't need them and
+  // we're not going to run them anyway as the connection is associated
+  // with a private context. See bug http://crbug.com/40803
   dbus_bool_t ok = dbus_connection_set_timeout_functions(
       dbus_g_connection_get_connection(connection_),
       NULL, NULL, NULL, NULL, NULL);
@@ -376,4 +425,3 @@ PollingPolicyInterface* WifiDataProviderLinux::NewPollingPolicy() {
                                   kTwoNoChangePollingIntervalMilliseconds,
                                   kNoWifiPollingIntervalMilliseconds>;
 }
-