Linux: use our spiffy new DBus client library for KWallet, instead of dbus-glib.

chrome/browser/password_manager/native_backend_kwallet_x.cc

 // Copyright (c) 2011 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/password_manager/native_backend_kwallet_x.h"
 
-#include <sstream>
+#include <vector>
 
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/browser/browser_thread.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
 #include "grit/chromium_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using std::string;
-using std::vector;
 using webkit_glue::PasswordForm;
 
 // We could localize this string, but then changing your locale would cause
 // you to lose access to all your stored passwords. Maybe best not to do that.
 const char NativeBackendKWallet::kKWalletFolder[] = "Chrome Form Data";
 
 const char NativeBackendKWallet::kKWalletServiceName[] = "org.kde.kwalletd";
 const char NativeBackendKWallet::kKWalletPath[] = "/modules/kwalletd";
 const char NativeBackendKWallet::kKWalletInterface[] = "org.kde.KWallet";
 const char NativeBackendKWallet::kKLauncherServiceName[] = "org.kde.klauncher";
 const char NativeBackendKWallet::kKLauncherPath[] = "/KLauncher";
 const char NativeBackendKWallet::kKLauncherInterface[] = "org.kde.KLauncher";
 
 NativeBackendKWallet::NativeBackendKWallet(LocalProfileId id,
                                            PrefService* prefs)
     : profile_id_(id), prefs_(prefs),
-      error_(NULL), connection_(NULL), proxy_(NULL),
       app_name_(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME)) {
   if (PasswordStoreX::PasswordsUseLocalProfileId(prefs)) {
     folder_name_ = GetProfileSpecificFolderName();
     // We already did the migration previously. Don't try again.
     migrate_tried_ = true;
   } else {
     folder_name_ = kKWalletFolder;
     migrate_tried_ = false;
   }
 }
 
 NativeBackendKWallet::~NativeBackendKWallet() {
-  if (proxy_)
-    g_object_unref(proxy_);
+  // This destructor is called on the thread that is destroying the Profile
+  // containing the PasswordStore that owns this NativeBackend. Generally that
+  // won't be the DB thread; it will be the UI thread. So we post a message to
+  // shut it down on the DB thread, and it will be destructed afterward when the
+  // scoped_refptr<dbus::Bus> goes out of scope. The NativeBackend will be
+  // destroyed before that occurs, but that's OK.
+  if (session_bus_.get()) {
+    BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
+                            NewRunnableMethod(

[satorux1, 2011/09/12 19:28:57]

base::Bind is preferable.

[Mike Mammarella, 2011/09/12 21:26:13]

I am inclined to stick with this for now since I know how to use it. I can look
into base::Bind in a follow-up CL: it's not obvious to me how to use it for a
member method, for instance.

[satorux1, 2011/09/12 22:28:21]

That's fine, but it should be as easy as:

BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
                        base::Bind(&dbus::Bus::ShutdownAndBlock),
                                   session_bus_));

session_bus_ is a ref counted object so this should compile.

If the object is not a ref counted object, you need to surround it with
base::Unretained() like base::Unretained(this).

+                                session_bus_.get(),
+                                &dbus::Bus::ShutdownAndBlock));
+  }
 }
 
 bool NativeBackendKWallet::Init() {
-  // Get a connection to the session bus.
-  connection_ = dbus_g_bus_get(DBUS_BUS_SESSION, &error_);
-  if (CheckError())
+  // We must synchronously do a few DBus calls to figure out if initialization
+  // succeeds, but later, we'll want to do most work on the DB thread. So we
+  // have to do the initialization on the DB thread here too, and wait for it.
+  scoped_refptr<dbus::Bus> optional_bus;  // Will construct its own.
+  bool success = false;
+  base::WaitableEvent event(false, false);
+  BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
+                          NewRunnableMethod(

[satorux1, 2011/09/12 19:28:57]

ditto.

[Mike Mammarella, 2011/09/12 21:26:13]

same

+                              this, &NativeBackendKWallet::InitWithBus,
+                              optional_bus, &event, &success));
+  event.Wait();
+  return success;
+}
+
+// NativeBackendKWallet isn't reference counted, but the one place we post a
+// message to it (in InitWithBus below) waits for the task to run. So we can
+// disable needing reference counting safely here.
+template<>
+struct RunnableMethodTraits<NativeBackendKWallet> {
+  void RetainCallee(NativeBackendKWallet*) {}
+  void ReleaseCallee(NativeBackendKWallet*) {}
+};
+
+void NativeBackendKWallet::InitWithBus(scoped_refptr<dbus::Bus> optional_bus,
+                                       base::WaitableEvent* event,
+                                       bool* success) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  DCHECK(!session_bus_.get());
+  if (optional_bus.get()) {

[satorux1, 2011/09/12 19:28:57]

This is a bit tricky. You might want to add a comment like

// optional_bus is supplied when this function is called for testing.

[Mike Mammarella, 2011/09/12 21:26:13]

Done.

+    session_bus_ = optional_bus;
+  } else {
+    // Get a connection to the session bus.
+    dbus::Bus::Options options;
+    options.bus_type = dbus::Bus::SESSION;
+    options.connection_type = dbus::Bus::PRIVATE;
+    session_bus_ = new dbus::Bus(options);
+  }
+  kwallet_proxy_ =
+      session_bus_->GetObjectProxy(kKWalletServiceName, kKWalletPath);
+  // kwalletd may not be running. If it fails to initialize, try to start it
+  // and then try to initialize it again. (Note the short-circuit evaluation.)
+  *success = InitWallet() || (StartKWalletd() && InitWallet());

[satorux1, 2011/09/12 19:28:57]

I wasn't sure if || had higher precedence than =. Adding parens may make it
clearer.


*success = (InitWallet() || (StartKWalletd() && InitWallet()));

[Mike Mammarella, 2011/09/12 21:26:13]

Some of the operators (the bitwise ones especially) don't have well-known
precedence, but I was under the impression that most people know that = and
friends are at the bottom of the list (above only the little-used , operator),
but I've added the extra () anyway.

+  if (event)
+    event->Signal();
+}
+
+bool NativeBackendKWallet::StartKWalletd() {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

Done.

+  // Sadly kwalletd doesn't use DBus activation, so we have to make a call to
+  // klauncher to start it.
+  dbus::ObjectProxy* klauncher =
+      session_bus_->GetObjectProxy(kKLauncherServiceName, kKLauncherPath);
+
+  dbus::MethodCall method_call(kKLauncherInterface,
+                               "start_service_by_desktop_name");
+  dbus::MessageWriter builder(&method_call);
+  dbus::MessageWriter empty(&method_call);
+  builder.AppendString("kwalletd");  // serviceName
+  builder.OpenArray("s", &empty);    // urls
+  builder.CloseContainer(&empty);
+  builder.OpenArray("s", &empty);    // envs
+  builder.CloseContainer(&empty);
+  builder.AppendString("");          // startup_id
+  builder.AppendBool(false);         // blind
+  scoped_ptr<dbus::Response> response(
+      klauncher->CallMethodAndBlock(
+          &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+  if (!response.get()) {
+    LOG(ERROR) << "Error contacting klauncher to start kwalletd";
     return false;
-
-  if (!InitWallet()) {
-    // kwalletd may not be running. Try to start it and try again.
-    if (!StartKWalletd() || !InitWallet())
-      return false;
+  }
+  dbus::MessageReader reader(response.get());
+  int32_t ret = -1;
+  std::string dbus_name;
+  std::string error;
+  int32_t pid = -1;
+  if (!reader.PopInt32(&ret) || !reader.PopString(&dbus_name) ||
+      !reader.PopString(&error) || !reader.PopInt32(&pid)) {
+    LOG(ERROR) << "Error reading response from klauncher to start kwalletd: "
+               << response->ToString();
+    return false;
+  }
+  if (!error.empty() || ret) {
+    LOG(ERROR) << "Error launching kwalletd: error '" << error << "' "
+               << " (code " << ret << ")";
+    return false;
   }
 
   return true;
 }
 
-bool NativeBackendKWallet::StartKWalletd() {
-  // Sadly kwalletd doesn't use DBUS activation, so we have to make a call to
-  // klauncher to start it.
-  DBusGProxy* klauncher_proxy =
-      dbus_g_proxy_new_for_name(connection_, kKLauncherServiceName,
-                                kKLauncherPath, kKLauncherInterface);
-
-  char* empty_string_list = NULL;
-  int ret = 1;
-  char* error = NULL;
-  dbus_g_proxy_call(klauncher_proxy, "start_service_by_desktop_name", &error_,
-                    G_TYPE_STRING,  "kwalletd",          // serviceName
-                    G_TYPE_STRV,    &empty_string_list,  // urls
-                    G_TYPE_STRV,    &empty_string_list,  // envs
-                    G_TYPE_STRING,  "",                  // startup_id
-                    G_TYPE_BOOLEAN, (gboolean) false,    // blind
-                    G_TYPE_INVALID,
-                    G_TYPE_INT,     &ret,                // result
-                    G_TYPE_STRING,  NULL,                // dubsName
-                    G_TYPE_STRING,  &error,              // error
-                    G_TYPE_INT,     NULL,                // pid
-                    G_TYPE_INVALID);
-
-  if (error && *error) {
-    LOG(ERROR) << "Error launching kwalletd: " << error;
-    ret = 1;  // Make sure we return false after freeing.
+bool NativeBackendKWallet::InitWallet() {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

Done.

+  {
+    // Check that KWallet is enabled.
+    dbus::MethodCall method_call(kKWalletInterface, "isEnabled");
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (isEnabled)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    bool enabled = false;
+    if (!reader.PopBool(&enabled)) {
+      LOG(ERROR) << "Error reading response from kwalletd (isEnabled): "
+                 << response->ToString();
+      return false;
+    }
+    // Not enabled? Don't use KWallet. But also don't warn here.
+    if (!enabled)
+      return false;

[satorux1, 2011/09/12 19:28:57]

VLOG(1) may be helpful for debugging?

[Mike Mammarella, 2011/09/12 21:26:13]

Done.

   }
 
-  g_free(error);
-  g_object_unref(klauncher_proxy);
-
-  if (CheckError() || ret != 0)
-    return false;
-  return true;
-}
-
-bool NativeBackendKWallet::InitWallet() {
-  // Make a proxy to KWallet.
-  proxy_ = dbus_g_proxy_new_for_name(connection_, kKWalletServiceName,
-                                     kKWalletPath, kKWalletInterface);
-
-  // Check KWallet is enabled.
-  gboolean is_enabled = false;
-  dbus_g_proxy_call(proxy_, "isEnabled", &error_,
-                    G_TYPE_INVALID,
-                    G_TYPE_BOOLEAN, &is_enabled,
-                    G_TYPE_INVALID);
-  if (CheckError() || !is_enabled)
-    return false;
-
-  // Get the wallet name.
-  char* wallet_name = NULL;
-  dbus_g_proxy_call(proxy_, "networkWallet", &error_,
-                    G_TYPE_INVALID,
-                    G_TYPE_STRING, &wallet_name,
-                    G_TYPE_INVALID);
-  if (CheckError() || !wallet_name)
-    return false;
-
-  wallet_name_.assign(wallet_name);
-  g_free(wallet_name);
+  {
+    // Get the wallet name.
+    dbus::MethodCall method_call(kKWalletInterface, "networkWallet");
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (networkWallet)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    if (!reader.PopString(&wallet_name_)) {
+      LOG(ERROR) << "Error reading response from kwalletd (networkWallet): "
+                 << response->ToString();
+      return false;
+    }
+  }
 
   return true;
 }
 
 bool NativeBackendKWallet::AddLogin(const PasswordForm& form) {
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
 
   PasswordFormList forms;
@@ skipping 58 matching lines @@
 
   // Update the entry in the wallet, possibly deleting it.
   bool ok = SetLoginsList(kept_forms, form.signon_realm, wallet_handle);
 
   STLDeleteElements(&kept_forms);
   return ok;
 }
 
 bool NativeBackendKWallet::RemoveLoginsCreatedBetween(
     const base::Time& delete_begin,
     const base::Time& delete_end) {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

WalletHandle() has such a check, and this method necessarily calls
WalletHandle(). So I don't think we need one here; in the extreme, we could add
such a check to every method everywhere that's only supposed to be called on one
thread, but I think that's probably overkill.

[satorux1, 2011/09/12 22:28:21]

It's a matter of taste so it's up to you, but I think adding one line DCHECK
won't be overkill.

I'd add that line where we called CallMethodAndBlock() which should only be
called on the DB thread, to make it clear that we are on the right thread to do
this.

   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
 
   // We could probably also use readEntryList here.
-  char** realm_list = NULL;
-  dbus_g_proxy_call(proxy_, "entryList", &error_,
-                    G_TYPE_INT,     wallet_handle,             // handle
-                    G_TYPE_STRING,  folder_name_.c_str(),      // folder
-                    G_TYPE_STRING,  app_name_.c_str(),         // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_STRV,    &realm_list,
-                    G_TYPE_INVALID);
-  if (CheckError())
-    return false;
+  std::vector<std::string> realm_list;
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "entryList");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (entryList)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    dbus::MessageReader array(response.get());
+    if (!reader.PopArray(&array)) {
+      LOG(ERROR) << "Error reading response from kwalletd (entryList): "
+                 << response->ToString();
+      return false;
+    }
+    while (array.HasMoreData()) {
+      std::string realm;
+      if (!array.PopString(&realm)) {
+        LOG(ERROR) << "Error reading response from kwalletd (entryList): "
+                   << response->ToString();
+        return false;
+      }
+      realm_list.push_back(realm);
+    }
+  }
 
   bool ok = true;
-  for (char** realm = realm_list; *realm; ++realm) {
-    GArray* byte_array = NULL;
-    dbus_g_proxy_call(proxy_, "readEntry", &error_,
-                      G_TYPE_INT,     wallet_handle,           // handle
-                      G_TYPE_STRING,  folder_name_.c_str(),    // folder
-                      G_TYPE_STRING,  *realm,                  // key
-                      G_TYPE_STRING,  app_name_.c_str(),       // appid
-                      G_TYPE_INVALID,
-                      DBUS_TYPE_G_UCHAR_ARRAY, &byte_array,
-                      G_TYPE_INVALID);
-
-    if (CheckError() || !byte_array ||
-        !CheckSerializedValue(byte_array, *realm)) {
+  for (size_t i = 0; i < realm_list.size(); ++i) {
+    const std::string& signon_realm = realm_list[i];
+    dbus::MethodCall method_call(kKWalletInterface, "readEntry");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(signon_realm);  // key
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (readEntry)";
       continue;
     }
+    dbus::MessageReader reader(response.get());
+    uint8_t* bytes = NULL;
+    size_t length = 0;
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
+                 << response->ToString();
+      continue;
+    }
+    if (!bytes || !CheckSerializedValue(bytes, length, signon_realm))
+      continue;
 
-    string signon_realm(*realm);
-    Pickle pickle(byte_array->data, byte_array->len);
+    // Can't we all just agree on whether bytes are signed or not? Please?
+    Pickle pickle(reinterpret_cast<const char*>(bytes), length);
     PasswordFormList all_forms;
     DeserializeValue(signon_realm, pickle, &all_forms);
-    g_array_free(byte_array, true);
 
     PasswordFormList kept_forms;
     kept_forms.reserve(all_forms.size());
     for (size_t i = 0; i < all_forms.size(); ++i) {
       if (delete_begin <= all_forms[i]->date_created &&
           (delete_end.is_null() || all_forms[i]->date_created < delete_end)) {
         delete all_forms[i];
       } else {
         kept_forms.push_back(all_forms[i]);
       }
     }
 
     if (!SetLoginsList(kept_forms, signon_realm, wallet_handle))
       ok = false;
     STLDeleteElements(&kept_forms);
   }
-  g_strfreev(realm_list);
   return ok;
 }
 
 bool NativeBackendKWallet::GetLogins(const PasswordForm& form,
                                      PasswordFormList* forms) {
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
   return GetLoginsList(forms, form.signon_realm, wallet_handle);
 }
@@ skipping 15 matching lines @@
 }
 
 bool NativeBackendKWallet::GetBlacklistLogins(PasswordFormList* forms) {
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
   return GetLoginsList(forms, false, wallet_handle);
 }
 
 bool NativeBackendKWallet::GetLoginsList(PasswordFormList* forms,
-                                         const string& signon_realm,
+                                         const std::string& signon_realm,
                                          int wallet_handle) {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

Although this method doesn't actually call WalletHandle(), it takes a wallet
handle that came from such a call. I think it's overkill here.

   // Is there an entry in the wallet?
-  gboolean has_entry = false;
-  dbus_g_proxy_call(proxy_, "hasEntry", &error_,
-                    G_TYPE_INT,     wallet_handle,         // handle
-                    G_TYPE_STRING,  folder_name_.c_str(),  // folder
-                    G_TYPE_STRING,  signon_realm.c_str(),  // key
-                    G_TYPE_STRING,  app_name_.c_str(),     // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_BOOLEAN, &has_entry,
-                    G_TYPE_INVALID);
-
-  if (CheckError())
-    return false;
-  if (!has_entry) {
-    // This is not an error. There just isn't a matching entry.
-    return true;
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "hasEntry");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(signon_realm);  // key
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (hasEntry)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    bool has_entry = false;
+    if (!reader.PopBool(&has_entry)) {
+      LOG(ERROR) << "Error reading response from kwalletd (hasEntry): "
+                 << response->ToString();
+      return false;
+    }
+    if (!has_entry) {
+      // This is not an error. There just isn't a matching entry.
+      return true;
+    }
   }
 
-  GArray* byte_array = NULL;
-  dbus_g_proxy_call(proxy_, "readEntry", &error_,
-                    G_TYPE_INT,     wallet_handle,         // handle
-                    G_TYPE_STRING,  folder_name_.c_str(),  // folder
-                    G_TYPE_STRING,  signon_realm.c_str(),  // key
-                    G_TYPE_STRING,  app_name_.c_str(),     // appid
-                    G_TYPE_INVALID,
-                    DBUS_TYPE_G_UCHAR_ARRAY, &byte_array,
-                    G_TYPE_INVALID);
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "readEntry");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(signon_realm);  // key
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (readEntry)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    uint8_t* bytes = NULL;
+    size_t length = 0;
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
+                 << response->ToString();
+      return false;
+    }
+    if (!bytes)
+      return false;
+    if (!CheckSerializedValue(bytes, length, signon_realm)) {
+      // This is weird, but we choose not to call it an error. There is an
+      // invalid entry somehow, but by just ignoring it, we make it easier to
+      // repair without having to delete it using kwalletmanager (that is, by
+      // just saving a new password within this realm to overwrite it).

[satorux1, 2011/09/12 19:28:57]

Worth adding a LOG(WARNING) or something?

[Mike Mammarella, 2011/09/12 21:26:13]

CheckSerializedValue() has one.

+      return true;
+    }
 
-  if (CheckError() || !byte_array)
-    return false;
-  if (!CheckSerializedValue(byte_array, signon_realm.c_str())) {
-    // This is weird, but we choose not to call it an error. There's an invalid
-    // entry somehow, but by pretending it just doesn't exist, we make it easier
-    // to repair without having to delete it using kwalletmanager (that is, by
-    // just saving a new password within this realm to overwrite it).
-    g_array_free(byte_array, true);
-    return true;
+    // Can't we all just agree on whether bytes are signed or not? Please?
+    Pickle pickle(reinterpret_cast<const char*>(bytes), length);
+    PasswordFormList all_forms;
+    DeserializeValue(signon_realm, pickle, forms);
   }
 
-  Pickle pickle(byte_array->data, byte_array->len);
-  DeserializeValue(signon_realm, pickle, forms);
-  g_array_free(byte_array, true);
-
   return true;
 }
 
 bool NativeBackendKWallet::GetLoginsList(PasswordFormList* forms,
                                          bool autofillable,
                                          int wallet_handle) {
   PasswordFormList all_forms;
   if (!GetAllLogins(&all_forms, wallet_handle))
     return false;
 
@@ skipping 25 matching lines @@
       forms->push_back(all_forms[i]);
     } else {
       delete all_forms[i];
     }
   }
 
   return true;
 }
 
 bool NativeBackendKWallet::GetAllLogins(PasswordFormList* forms,
                                         int wallet_handle) {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

Same reasoning about WalletHandle().

   // We could probably also use readEntryList here.
-  char** realm_list = NULL;
-  dbus_g_proxy_call(proxy_, "entryList", &error_,
-                    G_TYPE_INT,     wallet_handle,             // handle
-                    G_TYPE_STRING,  folder_name_.c_str(),      // folder
-                    G_TYPE_STRING,  app_name_.c_str(),         // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_STRV,    &realm_list,
-                    G_TYPE_INVALID);
-  if (CheckError())
-    return false;
+  std::vector<std::string> realm_list;
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "entryList");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (entryList)";
+      return false;
+    }
+    dbus::MessageReader reader(response.get());
+    dbus::MessageReader array(response.get());
+    if (!reader.PopArray(&array)) {
+      LOG(ERROR) << "Error reading response from kwalletd (entryList): "
+                 << response->ToString();
+      return false;
+    }
+    while (array.HasMoreData()) {

[satorux1, 2011/09/12 19:28:57]

This is the second time I see this. I guess it's worth adding PopArrayOfString()
in dbus::Message, which is generic enough to be there. If you think it's a good
idea, you can add do it in a separate patch.

[Mike Mammarella, 2011/09/12 21:26:13]

That would be useful. I'll consider adding one in a follow-up CL.

+      std::string realm;
+      if (!array.PopString(&realm)) {
+        LOG(ERROR) << "Error reading response from kwalletd (entryList): "
+                   << response->ToString();
+        return false;
+      }
+      realm_list.push_back(realm);
+    }
+  }
 
-  for (char** realm = realm_list; *realm; ++realm) {
-    GArray* byte_array = NULL;
-    dbus_g_proxy_call(proxy_, "readEntry", &error_,
-                      G_TYPE_INT,     wallet_handle,           // handle
-                      G_TYPE_STRING,  folder_name_.c_str(),    // folder
-                      G_TYPE_STRING,  *realm,                  // key
-                      G_TYPE_STRING,  app_name_.c_str(),       // appid
-                      G_TYPE_INVALID,
-                      DBUS_TYPE_G_UCHAR_ARRAY, &byte_array,
-                      G_TYPE_INVALID);
-
-    if (CheckError() || !byte_array ||
-        !CheckSerializedValue(byte_array, *realm)) {
+  for (size_t i = 0; i < realm_list.size(); ++i) {
+    const std::string& signon_realm = realm_list[i];
+    dbus::MethodCall method_call(kKWalletInterface, "readEntry");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(signon_realm);  // key
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (readEntry)";
       continue;
     }
+    dbus::MessageReader reader(response.get());
+    uint8_t* bytes = NULL;
+    size_t length = 0;
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
+                 << response->ToString();
+      continue;
+    }
+    if (!bytes || !CheckSerializedValue(bytes, length, signon_realm))
+      continue;
 
-    Pickle pickle(byte_array->data, byte_array->len);
-    DeserializeValue(*realm, pickle, forms);
-    g_array_free(byte_array, true);
+    // Can't we all just agree on whether bytes are signed or not? Please?
+    Pickle pickle(reinterpret_cast<const char*>(bytes), length);
+    PasswordFormList all_forms;
+    DeserializeValue(signon_realm, pickle, forms);
   }
-  g_strfreev(realm_list);
   return true;
 }
 
 bool NativeBackendKWallet::SetLoginsList(const PasswordFormList& forms,
-                                         const string& signon_realm,
+                                         const std::string& signon_realm,
                                          int wallet_handle) {

[satorux1, 2011/09/12 19:28:57]

You might want to add 

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

[Mike Mammarella, 2011/09/12 21:26:13]

Same.

   if (forms.empty()) {
     // No items left? Remove the entry from the wallet.
+    dbus::MethodCall method_call(kKWalletInterface, "removeEntry");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(wallet_handle);  // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(signon_realm);  // key
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (removeEntry)";
+      return kInvalidKWalletHandle;
+    }
+    dbus::MessageReader reader(response.get());
     int ret = 0;
-    dbus_g_proxy_call(proxy_, "removeEntry", &error_,
-                      G_TYPE_INT,     wallet_handle,         // handle
-                      G_TYPE_STRING,  folder_name_.c_str(),  // folder
-                      G_TYPE_STRING,  signon_realm.c_str(),  // key
-                      G_TYPE_STRING,  app_name_.c_str(),     // appid
-                      G_TYPE_INVALID,
-                      G_TYPE_INT,     &ret,
-                      G_TYPE_INVALID);
-    if (CheckError())
+    if (!reader.PopInt32(&ret)) {
+      LOG(ERROR) << "Error reading response from kwalletd (removeEntry): "
+                 << response->ToString();
       return false;
+    }
     if (ret != 0)
       LOG(ERROR) << "Bad return code " << ret << " from KWallet removeEntry";
     return ret == 0;
   }
 
   Pickle value;
   SerializeValue(forms, &value);
 
-  // Convert the pickled bytes to a GByteArray.
-  GArray* byte_array = g_array_sized_new(false, false, sizeof(char),
-                                         value.size());
-  g_array_append_vals(byte_array, value.data(), value.size());
-
-  // Make the call.
+  dbus::MethodCall method_call(kKWalletInterface, "writeEntry");
+  dbus::MessageWriter builder(&method_call);
+  builder.AppendInt32(wallet_handle);  // handle
+  builder.AppendString(folder_name_);  // folder
+  builder.AppendString(signon_realm);  // key
+  builder.AppendArrayOfBytes(static_cast<const uint8_t*>(value.data()),
+                             value.size());  // value
+  builder.AppendString(app_name_);     // appid
+  scoped_ptr<dbus::Response> response(
+      kwallet_proxy_->CallMethodAndBlock(
+          &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+  if (!response.get()) {
+    LOG(ERROR) << "Error contacting kwalletd (writeEntry)";
+    return kInvalidKWalletHandle;
+  }
+  dbus::MessageReader reader(response.get());
   int ret = 0;
-  dbus_g_proxy_call(proxy_, "writeEntry", &error_,
-                    G_TYPE_INT,           wallet_handle,         // handle
-                    G_TYPE_STRING,        folder_name_.c_str(),  // folder
-                    G_TYPE_STRING,        signon_realm.c_str(),  // key
-                    DBUS_TYPE_G_UCHAR_ARRAY, byte_array,         // value
-                    G_TYPE_STRING,        app_name_.c_str(),     // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_INT,           &ret,
-                    G_TYPE_INVALID);
-  g_array_free(byte_array, true);
-
-  if (CheckError())
+  if (!reader.PopInt32(&ret)) {
+    LOG(ERROR) << "Error reading response from kwalletd (writeEntry): "
+               << response->ToString();
     return false;
+  }
   if (ret != 0)
     LOG(ERROR) << "Bad return code " << ret << " from KWallet writeEntry";
   return ret == 0;
 }
 
 bool NativeBackendKWallet::CompareForms(const PasswordForm& a,
                                         const PasswordForm& b,
                                         bool update_check) {
   // An update check doesn't care about the submit element.
   if (!update_check && a.submit_element != b.submit_element)
@@ skipping 20 matching lines @@
     pickle->WriteString16(form->password_element);
     pickle->WriteString16(form->password_value);
     pickle->WriteString16(form->submit_element);
     pickle->WriteBool(form->ssl_valid);
     pickle->WriteBool(form->preferred);
     pickle->WriteBool(form->blacklisted_by_user);
     pickle->WriteInt64(form->date_created.ToTimeT());
   }
 }
 
-bool NativeBackendKWallet::CheckSerializedValue(const GArray* byte_array,
-                                                const char* realm) {
+bool NativeBackendKWallet::CheckSerializedValue(const uint8_t* byte_array,
+                                                size_t length,
+                                                const std::string& realm) {
   const Pickle::Header* header =
-      reinterpret_cast<const Pickle::Header*>(byte_array->data);
-  if (byte_array->len < sizeof(*header) ||
-      header->payload_size > byte_array->len - sizeof(*header)) {
+      reinterpret_cast<const Pickle::Header*>(byte_array);
+  if (length < sizeof(*header) ||
+      header->payload_size > length - sizeof(*header)) {
     LOG(WARNING) << "Invalid KWallet entry detected (realm: " << realm << ")";
     return false;
   }
   return true;
 }
 
-void NativeBackendKWallet::DeserializeValue(const string& signon_realm,
+void NativeBackendKWallet::DeserializeValue(const std::string& signon_realm,
                                             const Pickle& pickle,
                                             PasswordFormList* forms) {
   void* iter = NULL;
 
   int version = -1;
   if (!pickle.ReadInt(&iter, &version) || version != kPickleVersion) {
     // This is the only version so far, so anything else is an error.
     LOG(ERROR) << "Failed to deserialize KWallet entry "
                << "(realm: " << signon_realm << ")";
     return;
@@ skipping 41 matching lines @@
       break;
     }
     form->scheme = static_cast<PasswordForm::Scheme>(scheme);
     form->date_created = base::Time::FromTimeT(date_created);
     forms->push_back(form.release());
   }
 }
 
 bool NativeBackendKWallet::ReadGURL(const Pickle& pickle, void** iter,
                                     GURL* url) {
-  string url_string;
+  std::string url_string;
   if (!pickle.ReadString(iter, &url_string)) {
     LOG(ERROR) << "Failed to deserialize URL";
     *url = GURL();
     return false;
   }
   *url = GURL(url_string);
   return true;
 }
 
-bool NativeBackendKWallet::CheckError() {
-  if (error_) {
-    LOG(ERROR) << "Failed to complete KWallet call: " << error_->message;
-    g_error_free(error_);
-    error_ = NULL;
-    return true;
-  }
-  return false;
-}
-
 int NativeBackendKWallet::WalletHandle() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+
   // Open the wallet.
   // TODO(mdm): Are we leaking these handles? Find out.
-  int handle = kInvalidKWalletHandle;
-  dbus_g_proxy_call(proxy_, "open", &error_,
-                    G_TYPE_STRING, wallet_name_.c_str(),    // wallet
-                    G_TYPE_INT64,  0LL,                     // wid
-                    G_TYPE_STRING, app_name_.c_str(),       // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_INT,    &handle,
-                    G_TYPE_INVALID);
-  if (CheckError() || handle == kInvalidKWalletHandle)
-    return kInvalidKWalletHandle;
+  int32_t handle = kInvalidKWalletHandle;
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "open");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendString(wallet_name_);  // wallet
+    builder.AppendInt64(0);              // wid
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (open)";
+      return kInvalidKWalletHandle;
+    }
+    dbus::MessageReader reader(response.get());
+    if (!reader.PopInt32(&handle)) {
+      LOG(ERROR) << "Error reading response from kwalletd (open): "
+                 << response->ToString();
+      return kInvalidKWalletHandle;
+    }
+    if (handle == kInvalidKWalletHandle) {
+      LOG(ERROR) << "Error obtaining KWallet handle";
+      return kInvalidKWalletHandle;
+    }
+  }
 
   // Check if our folder exists.
-  gboolean has_folder = false;
-  dbus_g_proxy_call(proxy_, "hasFolder", &error_,
-                    G_TYPE_INT,    handle,                  // handle
-                    G_TYPE_STRING, folder_name_.c_str(),    // folder
-                    G_TYPE_STRING, app_name_.c_str(),       // appid
-                    G_TYPE_INVALID,
-                    G_TYPE_BOOLEAN, &has_folder,
-                    G_TYPE_INVALID);
-  if (CheckError())
-    return kInvalidKWalletHandle;
+  bool has_folder = false;
+  {
+    dbus::MethodCall method_call(kKWalletInterface, "hasFolder");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(handle);         // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (hasFolder)";
+      return kInvalidKWalletHandle;
+    }
+    dbus::MessageReader reader(response.get());
+    if (!reader.PopBool(&has_folder)) {
+      LOG(ERROR) << "Error reading response from kwalletd (hasFolder): "
+                 << response->ToString();
+      return kInvalidKWalletHandle;
+    }
+  }
 
   // Create it if it didn't.
   if (!has_folder) {
-    gboolean success = false;
-    dbus_g_proxy_call(proxy_, "createFolder", &error_,
-                      G_TYPE_INT,    handle,                // handle
-                      G_TYPE_STRING, folder_name_.c_str(),  // folder
-                      G_TYPE_STRING, app_name_.c_str(),     // appid
-                      G_TYPE_INVALID,
-                      G_TYPE_BOOLEAN, &success,
-                      G_TYPE_INVALID);
-    if (CheckError() || !success)
+    dbus::MethodCall method_call(kKWalletInterface, "createFolder");
+    dbus::MessageWriter builder(&method_call);
+    builder.AppendInt32(handle);         // handle
+    builder.AppendString(folder_name_);  // folder
+    builder.AppendString(app_name_);     // appid
+    scoped_ptr<dbus::Response> response(
+        kwallet_proxy_->CallMethodAndBlock(
+            &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
+    if (!response.get()) {
+      LOG(ERROR) << "Error contacting kwalletd (createFolder)";
       return kInvalidKWalletHandle;
+    }
+    dbus::MessageReader reader(response.get());
+    bool success = false;
+    if (!reader.PopBool(&success)) {
+      LOG(ERROR) << "Error reading response from kwalletd (createFolder): "
+                 << response->ToString();
+      return kInvalidKWalletHandle;
+    }
+    if (!success) {
+      LOG(ERROR) << "Error creating KWallet folder";
+      return kInvalidKWalletHandle;
+    }
   }
 
   // Successful initialization. Try migration if necessary.
   if (!migrate_tried_)
     MigrateToProfileSpecificLogins();
 
   return handle;
 }
 
 std::string NativeBackendKWallet::GetProfileSpecificFolderName() const {
@@ skipping 42 matching lines @@
     // Each other profile must be able to migrate the shared data as well,
     // so we must leave it alone. After a few releases, we'll add code to
     // delete them, and eventually remove this migration code.
     // TODO(mdm): follow through with the plan above.
     PasswordStoreX::SetPasswordsUseLocalProfileId(prefs_);
   } else {
     // We failed to migrate for some reason. Use the old folder name.
     folder_name_ = kKWalletFolder;
   }
 }