Linux: split GNOME Keyring integration into a separate process.

chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc

Index: chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc
===================================================================
--- chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc	(revision 0)
+++ chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc	(revision 0)
@@ -0,0 +1,430 @@
+// 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/keyring_proxy/keyring_proxy_client.h"
+
+#include <errno.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include "base/base_paths.h"
+#include "base/bind.h"
+#include "base/file_path.h"
+#include "base/message_loop.h"
+#include "base/message_pump_libevent.h"
+#include "base/path_service.h"
+#include "base/process_util.h"
+#include "base/string_number_conversions.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/password_manager/keyring_proxy/keyring_proxy.h"
+#include "chrome/browser/password_manager/keyring_proxy/message_reader.h"
+#include "content/public/browser/browser_thread.h"
+#include "webkit/glue/password_form.h"
+
+using content::BrowserThread;
+
+namespace keyring_proxy {
+
+// We need a MessageLoopForIO to watch a file descriptor, but the DB thread
+// doesn't have one. This class handles actually watching and reading from the
+// file descriptor on the file thread, then notifying the keyring proxy which
+// parses the messages and notifies the native backend on the DB thread.
+class KeyringProxyFDWatcher
+    : public base::RefCounted<KeyringProxyFDWatcher>,
+      public base::MessagePumpLibevent::Watcher {
+ public:
+  // Takes ownership of proxy_fd and will close it in the destructor.
+  // Does not take ownership of |client|. ShutDown() must be called before
+  // the client is destroyed to prevent further callbacks; the client
+  // should keep a reference to the new KeyringProxyFDWatcher until then.
+  KeyringProxyFDWatcher(int proxy_fd, KeyringProxyClient* client);
+
+  void Init();
+
+  int fd() const { return proxy_fd_; }
+
+  void ShutDown();
+
+ private:
+  friend class base::RefCounted<KeyringProxyFDWatcher>;
+  ~KeyringProxyFDWatcher();
+
+  // These run on the file thread.
+  void StartWatching();
+  void StopWatching();
+
+  // Implement base::MessagePumpLibevent::Watcher.
+  virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE;
+  virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE;
+
+  const int proxy_fd_;
+  KeyringProxyClient* client_;
+  base::MessagePumpLibevent::FileDescriptorWatcher ipc_watcher_;
+
+  // Used to batch data read from the proxy into individual replies.
+  MessageReader reader_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyringProxyFDWatcher);
+};
+
+KeyringProxyFDWatcher::KeyringProxyFDWatcher(int proxy_fd,
+                                             KeyringProxyClient* client)
+    : proxy_fd_(proxy_fd), client_(client) {
+  // It's not safe to post a task referencing a reference counted object from
+  // within its constructor: the execution of the task races with the
+  // constructor returning and the reference it had may be removed before
+  // another is created. So, we have an Init() method below instead.
+}
+
+void KeyringProxyFDWatcher::Init() {
+  if (client_) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                            base::Bind(&KeyringProxyFDWatcher::StartWatching,
+                                       this));
+  }
+}
+
+void KeyringProxyFDWatcher::ShutDown() {
+  // This will prevent any calls back to the client. It is technically

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

Shutdown crashes can be a pain in the butt to diagnose and fix, can you make it
right?

+  // insufficient as this runs on the UI thread and the check occurs on the file
+  // thread, so the file thread may have already checked it before we clear it.
+  // We could fix that with a lock held while client callbacks run on the file
+  // thread, but for now just ignore it - it would only be a problem if you were
+  // actively using the proxy during shutdown, which probably shouldn't happen.
+  client_ = NULL;
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&KeyringProxyFDWatcher::StopWatching,
+                                     this));
+}
+
+KeyringProxyFDWatcher::~KeyringProxyFDWatcher() {
+  // It should not be necessary to stop watching here, since we should already
+  // have done that in StopWatching() prior to the last reference going away.
+  // We do it anyway just to be extra sure.
+  ipc_watcher_.StopWatchingFileDescriptor();

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

Will this always be called on the right thread? Add a DCHECK that we've already
stopped watching in order to prevent regressions?

+  close(proxy_fd_);
+}
+
+void KeyringProxyFDWatcher::StartWatching() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  MessageLoopForIO* file_loop = MessageLoopForIO::current();
+  bool ok = file_loop->WatchFileDescriptor(
+      proxy_fd_, true, MessageLoopForIO::WATCH_READ, &ipc_watcher_, this);
+  DCHECK(ok);
+}
+
+void KeyringProxyFDWatcher::StopWatching() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  ipc_watcher_.StopWatchingFileDescriptor();
+}
+
+void KeyringProxyFDWatcher::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK_EQ(proxy_fd_, fd);
+  // We don't need to read all the available data. If there is still data left
+  // after we return, then we'll get called again because the descriptor will
+  // still be ready to read. So just read a fixed amount, for simplicity.
+  char buffer[256];

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

constant somewhere?

+  ssize_t available = read(proxy_fd_, buffer, sizeof(buffer));
+  if (available > 0) {
+    // Got some data. Handle it. Note that this may not be a complete message.
+    ssize_t used = 0;
+    while (used < available) {
+      ssize_t handled = reader_.HandleData(&buffer[used], available);
+      DCHECK_GT(handled, 0);
+      used += handled;
+      available -= handled;
+      if (reader_.is_complete()) {
+        if (client_)
+          client_->HandleProxyReply(reader_.lines());
+        reader_.Reset();
+      } else {
+        DCHECK_EQ(0, available);
+      }
+    }
+  } else if (available == 0 || errno != EINTR) {
+    // EOF, or an unexpected error. Stop watching and notify.
+    LOG(WARNING) << "Unexpected failure reading from keyring proxy: " << errno;
+    ipc_watcher_.StopWatchingFileDescriptor();
+    if (client_)
+      client_->HandleProxyError(available == 0);
+  }
+  // Note that if the read is interrupted, we'll just end up here. That's fine.
+  // The file descriptor will still be ready and we'll get called again soon.
+}
+
+void KeyringProxyFDWatcher::OnFileCanWriteWithoutBlocking(int fd) {
+  // We didn't ask to be notified of writability.
+  NOTREACHED();
+}
+
+const char KeyringProxyClient::kProxyBinary[] = "chrome-keyring-proxy";
+
+KeyringProxyClient::KeyringProxyClient() : next_request_id_(0) {
+}
+
+KeyringProxyClient::~KeyringProxyClient() {
+  base::AutoLock lock(request_lock_);
+  if (fd_watcher_.get())
+    fd_watcher_->ShutDown();
+  CancelAllRequests();
+}
+
+bool KeyringProxyClient::Connect() {
+  base::AutoLock lock(request_lock_);
+  DCHECK(!fd_watcher_.get());
+  int fd = LaunchKeyringProxy();
+  if (fd < 0)
+    return false;
+  fd_watcher_ = new KeyringProxyFDWatcher(fd, this);
+  fd_watcher_->Init();
+  return true;
+}
+
+void KeyringProxyClient::ConnectForTesting(int fd, bool watch_for_reads) {
+  if (watch_for_reads) {
+    fd_watcher_ = new KeyringProxyFDWatcher(fd, this);
+    fd_watcher_->Init();
+  } else {
+    fd_watcher_ = new KeyringProxyFDWatcher(fd, NULL);
+    // There's no need to call Init(), as we passed NULL for |client| above.
+  }
+}
+
+void KeyringProxyClient::AddLogin(const PasswordForm& form,
+                                  const std::string& app_string,
+                                  RequestContext* context) {
+  // If we are asked to save a password with 0 date, use the current time.
+  // We don't want to actually save passwords as though on January 1, 1970.
+  time_t date_created = form.date_created.ToTimeT();
+  if (!date_created)
+    date_created = time(NULL);
+  ProxyMessage request;
+  request.push_back("+" + form.origin.spec());  // Display name.
+  request.push_back("+" + UTF16ToUTF8(form.password_value));
+  request.push_back("+" + form.origin.spec());  // Origin.
+  request.push_back("+" + form.action.spec());
+  request.push_back("+" + UTF16ToUTF8(form.username_element));
+  request.push_back("+" + UTF16ToUTF8(form.username_value));
+  request.push_back("+" + UTF16ToUTF8(form.password_element));
+  request.push_back("+" + UTF16ToUTF8(form.submit_element));
+  request.push_back("+" + form.signon_realm);
+  request.push_back(form.ssl_valid ? "1" : "0");
+  request.push_back(form.preferred ? "1" : "0");
+  request.push_back("+" + base::Int64ToString(date_created));
+  request.push_back(form.blacklisted_by_user ? "1" : "0");
+  request.push_back(StringPrintf("%d", form.scheme));
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kAddLoginCommand, request, context);
+}
+
+void KeyringProxyClient::AddLoginSearch(const PasswordForm& form,
+                                        const std::string& app_string,
+                                        RequestContext* context) {
+  ProxyMessage request;
+  request.push_back("+" + form.origin.spec());
+  request.push_back("+" + UTF16ToUTF8(form.username_element));
+  request.push_back("+" + UTF16ToUTF8(form.username_value));
+  request.push_back("+" + UTF16ToUTF8(form.password_element));
+  request.push_back("+" + UTF16ToUTF8(form.submit_element));
+  request.push_back("+" + form.signon_realm);
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kAddLoginSearchCommand, request, context);
+}
+
+void KeyringProxyClient::UpdateLoginSearch(const PasswordForm& form,
+                                           const std::string& app_string,
+                                           RequestContext* context) {
+  ProxyMessage request;
+  request.push_back("+" + form.origin.spec());
+  request.push_back("+" + UTF16ToUTF8(form.username_element));
+  request.push_back("+" + UTF16ToUTF8(form.username_value));
+  request.push_back("+" + UTF16ToUTF8(form.password_element));
+  request.push_back("+" + form.signon_realm);
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kUpdateLoginSearchCommand, request, context);
+}
+
+void KeyringProxyClient::RemoveLogin(const PasswordForm& form,
+                                     const std::string& app_string,
+                                     RequestContext* context) {
+  ProxyMessage request;
+  request.push_back("+" + form.origin.spec());
+  request.push_back("+" + form.action.spec());
+  request.push_back("+" + UTF16ToUTF8(form.username_element));
+  request.push_back("+" + UTF16ToUTF8(form.username_value));
+  request.push_back("+" + UTF16ToUTF8(form.password_element));
+  request.push_back("+" + UTF16ToUTF8(form.submit_element));
+  request.push_back("+" + form.signon_realm);
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kRemoveLoginCommand, request, context);
+}
+
+void KeyringProxyClient::GetLogins(const PasswordForm& form,
+                                   const std::string& app_string,
+                                   RequestContext* context) {
+  ProxyMessage request;
+  request.push_back("+" + form.signon_realm);
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kGetLoginsCommand, request, context);
+}
+
+void KeyringProxyClient::GetLoginsList(bool blacklisted_by_user,
+                                       const std::string& app_string,
+                                       RequestContext* context) {
+  ProxyMessage request;
+  request.push_back(blacklisted_by_user ? "1" : "0");
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kGetLoginsListCommand, request, context);
+}
+
+void KeyringProxyClient::GetAllLogins(const std::string& app_string,
+                                      RequestContext* context) {
+  ProxyMessage request;
+  request.push_back(app_string);
+  SendRequest(KeyringProxy::kGetAllLoginsCommand, request, context);
+}
+
+// static
+int KeyringProxyClient::LaunchKeyringProxy() {
+  FilePath chrome_dir;
+  if (!PathService::Get(base::DIR_EXE, &chrome_dir))
+    return -1;
+  std::vector<std::string> proxy_command;
+  proxy_command.push_back(chrome_dir.Append(kProxyBinary).value());
+
+  int fds[2];
+  if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) < 0)
+    return -1;
+  base::file_handle_mapping_vector ipc;
+  ipc.push_back(std::make_pair(fds[1], STDIN_FILENO));
+  ipc.push_back(std::make_pair(fds[1], STDOUT_FILENO));
+  base::LaunchOptions options;
+  options.fds_to_remap = &ipc;
+
+  base::ProcessHandle proxy_handle;
+  if (!base::LaunchProcess(proxy_command, options, &proxy_handle)) {
+    close(fds[0]);
+    close(fds[1]);
+    return -1;
+  }
+  close(fds[1]);
+  return fds[0];
+}
+
+int KeyringProxyClient::GetRequestId() {
+  for (;;) {

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

Any reason not to use while(true) { ?

+    // |next_request_id_| is actually the most recently used request ID, because

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

Can we then name it appropriately? last_request_id_ ?

+    // that makes it easier to write this loop with a preincrement operator.
+    if (++next_request_id_ < 0)
+      next_request_id_ = 1;
+    if (proxy_requests_.find(next_request_id_) == proxy_requests_.end())
+      return next_request_id_;
+  }
+}
+
+void KeyringProxyClient::CancelRequest(RequestContext* context) {
+  context->result_code = GNOME_KEYRING_RESULT_CANCELLED;
+  context->event.Signal();
+}
+
+void KeyringProxyClient::CancelAllRequests() {
+  for (std::map<int, RequestContext*>::iterator it = proxy_requests_.begin();

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

request_lock_.AssertAcquired();

+       it != proxy_requests_.end(); ++it) {
+    CancelRequest(it->second);
+  }
+  proxy_requests_.clear();
+}
+
+void KeyringProxyClient::SendRequest(char type,
+                                     const ProxyMessage& request,
+                                     RequestContext* context) {
+  base::AutoLock lock(request_lock_);
+  if (!fd_watcher_.get()) {
+    LOG(WARNING) << "Attempted to use unconnected keyring proxy";
+    CancelRequest(context);
+    return;
+  }
+  int id = GetRequestId();
+  proxy_requests_[id] = context;
+  std::string message = StringPrintf("%c%d\n", type, id);
+  for (size_t i = 0; i < request.size(); ++i)

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

You put the message in a vector of strings and then later join the strings. 
Might it be more efficient to have an explicit message glass, that lets you
append directly onto the string while still ensuring that each line is newline
terminated?  It could also ensure there are no embedded new lines in the
parameters and add your leading +

+    message += request[i] + "\n";
+  message += "\n";
+  size_t written = write(fd_watcher_->fd(), message.data(), message.size());

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

Can you compose your message first, then get the lock and fill in the id?  Or do
you need the lock to access fd_watcher_?

+  if (written != message.size()) {
+    LOG(ERROR) << "Failed to write to keyring proxy! Likely failure soon...";
+    CancelRequest(context);
+  }
+}
+
+void KeyringProxyClient::HandleProxyReply(const ProxyMessage& reply) {
+  if (!reply.size()) {
+    LOG(WARNING) << "Got empty reply from keyring proxy";
+    return;
+  }
+  int id, result_code;
+  if (sscanf(reply[0].c_str(), "%d %d", &id, &result_code) != 2) {
+    // We haven't acquired the lock yet so we can just use HandleProxyError().
+    HandleProxyError(false);
+    return;
+  }
+  base::AutoLock lock(request_lock_);
+  std::map<int, RequestContext*>::iterator it = proxy_requests_.find(id);
+  if (it == proxy_requests_.end()) {
+    LOG(WARNING) << "Unexpected reply from keyring proxy (ID " << id << ")";
+    return;
+  }
+  RequestContext* context = it->second;
+  const size_t kLinesPerPass = 13;

[vandebo (ex-Chrome), 2011/12/01 19:57:30]

This isn't so much lines per pass, but lines per result ?

+  if ((reply.size() - 1) % kLinesPerPass != 0 ||
+      (reply.size() > 1 && !context->result_list)) {
+    LOG(WARNING) << "Invalid reply from keyring proxy (ID " << id << ")";
+    fd_watcher_->ShutDown();
+    fd_watcher_ = NULL;
+    CancelAllRequests();
+    return;
+  }
+  // We don't remove the request from the map until after the checks above.
+  proxy_requests_.erase(it);
+  context->result_code = static_cast<GnomeKeyringResult>(result_code);
+  for (size_t i = 1; i + kLinesPerPass <= reply.size(); i += kLinesPerPass) {
+    PasswordForm* form = new PasswordForm;
+    form->origin = GURL(reply[i].substr(1));
+    form->action = GURL(reply[i + 1].substr(1));
+    form->username_element = UTF8ToUTF16(reply[i + 2].substr(1));
+    form->username_value = UTF8ToUTF16(reply[i + 3].substr(1));
+    form->password_element = UTF8ToUTF16(reply[i + 4].substr(1));
+    form->password_value = UTF8ToUTF16(reply[i + 5].substr(1));
+    form->submit_element = UTF8ToUTF16(reply[i + 6].substr(1));
+    form->signon_realm = reply[i + 7].substr(1);
+    form->ssl_valid = reply[i + 8] != "0";
+    form->preferred = reply[i + 9] != "0";
+    int64 date_created = 0;
+    bool date_ok = base::StringToInt64(reply[i + 10].substr(1), &date_created);
+    DCHECK(date_ok);
+    form->date_created = base::Time::FromTimeT(date_created);
+    form->blacklisted_by_user = reply[i + 11] != "0";
+    unsigned int scheme = 0;
+    int scheme_ok = sscanf(reply[i + 12].c_str(), "%u", &scheme);
+    DCHECK_EQ(1, scheme_ok);
+    form->scheme = static_cast<PasswordForm::Scheme>(scheme);
+    context->result_list->push_back(form);
+  }
+  context->event.Signal();
+}
+
+void KeyringProxyClient::HandleProxyError(bool eof) {
+  base::AutoLock lock(request_lock_);
+  if (eof)
+    LOG(WARNING) << "Unexpected EOF reading from keyring proxy";
+  else
+    LOG(WARNING) << "Invalid reply from keyring proxy";
+  fd_watcher_->ShutDown();
+  fd_watcher_ = NULL;
+  CancelAllRequests();
+}
+
+}  // namespace keyring_proxy
Property changes on: chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc
___________________________________________________________________
Added: svn:eol-style
   + LF