Extends DBusThreadManager to connect ibus-bus.

chrome/browser/chromeos/input_method/ibus_controller_impl.cc

Index: chrome/browser/chromeos/input_method/ibus_controller_impl.cc
diff --git a/chrome/browser/chromeos/input_method/ibus_controller_impl.cc b/chrome/browser/chromeos/input_method/ibus_controller_impl.cc
index 10a354a9e7c5bb725a9d1c61288aaa4b0ce4fbb1..409e6f95099aa380e0ac5210947e83b547955c80 100644
--- a/chrome/browser/chromeos/input_method/ibus_controller_impl.cc
+++ b/chrome/browser/chromeos/input_method/ibus_controller_impl.cc
@@ -12,12 +12,19 @@
 #include <stack>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/environment.h"
+#include "base/files/file_path_watcher.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/rand_util.h"
 #include "base/stringprintf.h"
 #include "base/string_split.h"
 #include "chrome/browser/chromeos/input_method/input_method_config.h"
 #include "chrome/browser/chromeos/input_method/input_method_property.h"
 #include "chrome/browser/chromeos/input_method/input_method_util.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "content/public/browser/browser_thread.h"
 
 // TODO(nona): Remove libibus dependency from this file. Then, write unit tests
 // for all functions in this file. crbug.com/26334
@@ -53,6 +60,7 @@ namespace input_method {
 
 #if defined(HAVE_IBUS)
 const char kPanelObjectKey[] = "panel-object";
+const int kIBusDaemonRetryCount = 10;
 
 namespace {
 
@@ -357,13 +365,79 @@ std::string PrintPropList(IBusPropList *prop_list, int tree_level) {
   return stream.str();
 }
 
+void MaybeResetIBusBusAndSetsInitializationDone(
+    const std::string& ibus_address,
+    IBusControllerImpl* controller) {
+  chromeos::DBusThreadManager::Get()->MaybeResetIBusBus(ibus_address);
+  controller->IBusDaemonInitializationDone();
+}
+
+// The implementation of FilePathWatcher::Delegate, used in
+// StartAddressFileWatch function.
+class IBusAddressFileWatcherDelegate
+    : public base::files::FilePathWatcher::Delegate {
+ public:
+  IBusAddressFileWatcherDelegate(const std::string& ibus_address,
+                                 IBusControllerImpl* controller,
+                                 base::files::FilePathWatcher* watcher)
+      : ibus_address_(ibus_address),
+        controller_(controller),
+        watcher_(watcher) {
+    DCHECK(controller);
+    DCHECK(watcher);
+    DCHECK(!ibus_address.empty());
+  }
+
+  virtual ~IBusAddressFileWatcherDelegate() {}
+
+  virtual void OnFilePathChanged(const FilePath& file_path) {
+    if (!watcher_)
+      return;
+    bool success = content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&MaybeResetIBusBusAndSetsInitializationDone,
+                   ibus_address_,
+                   controller_));
+    DCHECK(success);
+    MessageLoop::current()->DeleteSoon(FROM_HERE, watcher_);
+    watcher_ = NULL;
+  }
+
+ private:
+  // The ibus-daemon address.
+  const std::string ibus_address_;
+  IBusControllerImpl* controller_;
+  base::files::FilePathWatcher* watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(IBusAddressFileWatcherDelegate);
+};
+
+void StartAddressFileWatch(const std::string& ibus_address,
+                           IBusControllerImpl* controller) {
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  std::string address_file_path;
+  env->GetVar("IBUS_ADDRESS_FILE", &address_file_path);

[Yusuke Sato, 2012/05/01 07:06:55]

Calling GetVar on FILE thread looks a bit scary. I know it works right now
(because only tests call SetVar as of today), but probably it's better to call
it on UI therad. Add a TODO if you agree.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+  DCHECK(!address_file_path.empty());
+
+  // The |watcher| instance will be released by |delegate|.
+  base::files::FilePathWatcher* watcher = new base::files::FilePathWatcher;

[Yusuke Sato, 2012/05/01 07:06:55]

remove the previous instance here? see my comment in .h.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+
+  // The |delegate| is owned by watcher.
+  IBusAddressFileWatcherDelegate* delegate = new IBusAddressFileWatcherDelegate(
+      ibus_address, controller, watcher);
+  bool result = watcher->Watch(FilePath(address_file_path), delegate);
+  DCHECK(result);
+}

[Yusuke Sato, 2012/05/01 07:06:55]

nit: space between 431 & 432.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

 }  // namespace
 
 IBusControllerImpl::IBusControllerImpl()
     : ibus_(NULL),
       ibus_config_(NULL),
-      should_launch_daemon_(false),
-      process_handle_(base::kNullProcessHandle) {
+      process_handle_(base::kNullProcessHandle),
+      ibus_daemon_status_(IBUS_DAEMON_STOP),
+      ibus_launch_retry_count_(kIBusDaemonRetryCount),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 }
 
 IBusControllerImpl::~IBusControllerImpl() {
@@ -405,10 +479,13 @@ IBusControllerImpl::~IBusControllerImpl() {
 
 bool IBusControllerImpl::Start() {
   MaybeInitializeIBusBus();
-  should_launch_daemon_ = true;
   if (IBusConnectionsAreAlive())
     return true;
-  return MaybeLaunchIBusDaemon();
+  if (ibus_daemon_status_ == IBUS_DAEMON_STOP ||
+      ibus_daemon_status_ == IBUS_DAEMON_SHUTTING_DOWN) {
+    return StartIBusDaemon();
+  }
+  return true;
 }
 
 void IBusControllerImpl::Reset() {
@@ -422,6 +499,13 @@ void IBusControllerImpl::Reset() {
 }
 
 bool IBusControllerImpl::Stop() {
+  if (process_handle_ == base::kNullProcessHandle) {

[Yusuke Sato, 2012/05/01 07:06:55]

You checked |ibus_daemon_status_| in Start() but checked |process_handle_| in
Stop(). This is inconsistent and confusing.

I think we should remove all if (process_handle_ == base::kNullProcessHandle)
and if (process_handle_ != base::kNullProcessHandle) code. This would make your
CL easier to review.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+    // The daemon hasn't been started yet.
+    return true;
+  }
+
+  ibus_daemon_status_ = IBUS_DAEMON_SHUTTING_DOWN;
+  // TODO(nona): Shutdown ibus-bus connection.
   if (IBusConnectionsAreAlive()) {
     // Ask IBus to exit *asynchronously*.
     ibus_bus_exit_async(ibus_,
@@ -436,21 +520,18 @@ bool IBusControllerImpl::Stop() {
       g_object_unref(ibus_config_);
       ibus_config_ = NULL;
     }
-  } else if (process_handle_ != base::kNullProcessHandle) {
+  } else {
+    ibus_daemon_status_ = IBUS_DAEMON_SHUTTING_DOWN;

[Yusuke Sato, 2012/05/01 07:06:55]

nit: looks like this is unnecessary. see L.507.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

     base::KillProcess(process_handle_, -1, false /* wait */);
     DVLOG(1) << "Killing ibus-daemon. PID="
              << base::GetProcId(process_handle_);
-  } else {
-    // The daemon hasn't been started yet.
   }
-
   process_handle_ = base::kNullProcessHandle;
-  should_launch_daemon_ = false;
   return true;
 }
 
 bool IBusControllerImpl::ChangeInputMethod(const std::string& id) {
-  DCHECK(should_launch_daemon_);
+  DCHECK_EQ(IBUS_DAEMON_RUNNING, ibus_daemon_status_);

[Yusuke Sato, 2012/05/01 07:06:55]

I think this DCHECK is harmful. Consider this case:

1. the user enables mozc, and the user uses the IME for a while.
2. for some reason, ibus-daemon crashes. the status is changed to STOP inside
the exit handler.
3. the user switches the IME by Shift+Alt --> DCHECK fail.

Change the DCHECK to

if (ibus_daemon_status_ != IBUS_DAEMON_RUNNING)
  return false;

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

 
   // Sanity checks.
   DCHECK(!InputMethodUtil::IsKeyboardLayout(id));
@@ -892,27 +973,41 @@ void IBusControllerImpl::UpdateProperty(IBusPanelService* panel,
   }
 }
 
-bool IBusControllerImpl::MaybeLaunchIBusDaemon() {
-  static const char kIBusDaemonPath[] = "/usr/bin/ibus-daemon";
-
-  if (process_handle_ != base::kNullProcessHandle) {
-    DVLOG(1) << "MaybeLaunchIBusDaemon: ibus-daemon is already running.";
-    return false;
-  }
-  if (!should_launch_daemon_)
-    return false;
+bool IBusControllerImpl::StartIBusDaemon() {
+  DCHECK_EQ(base::kNullProcessHandle, process_handle_);

[Yusuke Sato, 2012/05/01 07:06:55]

move this to LaunchIBusDaemon()?
you can check ibus_daemon_status_ instead.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+  ibus_daemon_status_ = IBUS_DAEMON_INITIALIZING;
+  const std::string ibus_address = base::StringPrintf(
+      "unix:abstract=/tmp/ibus-%0lX", base::RandUint64());

[Yusuke Sato, 2012/05/01 07:06:55]

nit: It's not a file but a socket in an abstract namespace, right? If so, remove
'/tmp/'.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+
+  // Set up ibus-daemon address file watcher before launching ibus-daemon,
+  // because if watcher start after ibus-daemon, we may miss the ibus connection
+  // initialization.
+  bool success = content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&StartAddressFileWatch,
+                 ibus_address,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&IBusControllerImpl::LaunchIBusDaemon,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 ibus_address));
+  DCHECK(success);
+  return true;
+}
 
+void IBusControllerImpl::LaunchIBusDaemon(const std::string& ibus_address) {
+  static const char kIBusDaemonPath[] = "/usr/bin/ibus-daemon";
   // TODO(zork): Send output to /var/log/ibus.log
   const std::string ibus_daemon_command_line =
-      base::StringPrintf("%s --panel=disable --cache=none --restart --replace",
-                         kIBusDaemonPath);
+      base::StringPrintf("%s --panel=disable --cache=none --restart --replace"
+                         " --address=%s",
+                         kIBusDaemonPath,
+                         ibus_address.c_str());
   if (!LaunchProcess(ibus_daemon_command_line,
                      &process_handle_,
                      reinterpret_cast<GChildWatchFunc>(OnIBusDaemonExit))) {
     DVLOG(1) << "Failed to launch " << ibus_daemon_command_line;
-    return false;
   }
-  return true;
 }
 
 bool IBusControllerImpl::LaunchProcess(const std::string& command_line,
@@ -939,6 +1034,18 @@ bool IBusControllerImpl::LaunchProcess(const std::string& command_line,
   return true;
 }
 
+void IBusControllerImpl::IBusDaemonInitializationDone() {
+  DCHECK_NE(IBUS_DAEMON_RUNNING, ibus_daemon_status_);
+  if (ibus_daemon_status_ != IBUS_DAEMON_INITIALIZING) {
+    // Corresponding to killing ibus-daemon under initializing, so do nothing.
+    // TODO(nona): Shutdown ibus bus connection.
+    return;
+  }
+  ibus_daemon_status_ = IBUS_DAEMON_RUNNING;
+  // Reset retry count

[Yusuke Sato, 2012/05/01 07:06:55]

nit: this looks obvious. remove

[Seigo Nonaka, 2012/05/02 05:12:10]

Removed retry count from CL.
Thanks.
On 2012/05/01 07:06:55, Yusuke Sato wrote:

+  ibus_launch_retry_count_ = kIBusDaemonRetryCount;
+}
+
 // static
 void IBusControllerImpl::SetInputMethodConfigCallback(GObject* source_object,
                                                       GAsyncResult* res,
@@ -963,16 +1070,47 @@ void IBusControllerImpl::SetInputMethodConfigCallback(GObject* source_object,
   g_object_unref(config);
 }
 
-// static
 void IBusControllerImpl::OnIBusDaemonExit(GPid pid,
                                           gint status,
                                           IBusControllerImpl* controller) {
-  if (controller->process_handle_ != base::kNullProcessHandle &&
-      base::GetProcId(controller->process_handle_) == pid) {
-    controller->process_handle_ = base::kNullProcessHandle;
+  const IBusDaemonStatus on_exit_state = controller->ibus_daemon_status_;
+  controller->ibus_daemon_status_ = IBUS_DAEMON_STOP;
+
+  if (controller->process_handle_ != base::kNullProcessHandle) {
+    if (base::GetProcId(controller->process_handle_) == pid) {
+      // Corresponding to shutting down ibus-daemon by exit message or crashing
+      // ibus-daemon. So reset handler and try re-launch if necessary.
+      controller->process_handle_ = base::kNullProcessHandle;
+    } else {
+      // Corresponding to ibus-daemon is re-launched during

[Yusuke Sato, 2012/05/01 07:06:55]

question: when exactly could this path be taken? probably I couldn't understand
your code comment here.

[Seigo Nonaka, 2012/05/02 05:12:10]

I added the this condition path in code comment.

On 2012/05/01 07:06:55, Yusuke Sato wrote:

+      // |ibus_daemon_status_| is IBUS_DAEMON_SHUTTING_DOWN. So this exit signal
+      // is corresponds to old ibus-daemon's one and do not try re-launch.
+      // TODO(nona): Shutdown ibus-bus connection.
+      return;

[Yusuke Sato, 2012/05/01 07:06:55]

should we assign STOP in this case? (see L.1077)

[Seigo Nonaka, 2012/05/02 05:12:10]

Thanks moved L.1077 to L.1092
On 2012/05/01 07:06:55, Yusuke Sato wrote:

+    }
+  }
+
+  if (on_exit_state == IBUS_DAEMON_SHUTTING_DOWN) {
+    // Normal exitting, so do nothing.
+    return;
+  }
+
+  DCHECK_NE(IBUS_DAEMON_STOP, on_exit_state);

[Yusuke Sato, 2012/05/01 07:06:55]

(note: still checking if this DCHECK is valid.)

[Seigo Nonaka, 2012/05/02 05:12:10]

Thanks, removed.
On 2012/05/01 07:06:55, Yusuke Sato wrote:

+
+  if (on_exit_state == IBUS_DAEMON_INITIALIZING) {
+    // TODO(nona): Remove current file watcher if exit on initializing state.

[Yusuke Sato, 2012/05/01 07:06:55]

I guess it's not actually safe. please consider this case:

- ibus-daemon starts.
- it creates an address file.
- right after that, it crashes.
- OnIBusDaemonExit is called first, then
MaybeResetIBusBusAndSetsInitializationDone() is called (via a message from FILE
thread).
- The status becomes RUNNING even though the daemon is not yet running.


(not 100% sure, could be wrong but) we can fix this problem by:
- add |input_method_address_| member variable to this class, and update it in
LaunchIBusDaemon().
- move MaybeResetIBusBusAndSetsInitializationDone() from anon namespace to this
class, and check if the address passed to the function matches
|input_method_address_|.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+    // It is safe to leave watcher insatnce if the ibus-daemon is crashed before
+    // making address file.
+  }
+
+  if (controller->ibus_launch_retry_count_ == 0) {
+    LOG(FATAL) << "Give up re-launching ibus-daemon";

[Yusuke Sato, 2012/05/01 07:06:55]

I don't think we need to reboot Chromebook here because rebooting will unlikely
fix the problem. LOG(ERROR) would suffice.

[Seigo Nonaka, 2012/05/02 05:12:10]

Done.

+    return;
+  } else {
+    LOG(ERROR) << "The ibus-daemon is crashed. Re-launching...";
+    --controller->ibus_launch_retry_count_;
+    controller->StartIBusDaemon();
   }
-  // Restart the daemon if needed.
-  controller->MaybeLaunchIBusDaemon();
 }
 #endif  // defined(HAVE_IBUS)