Do not execute two or more setxkbmap commands in parallel.

chrome/browser/chromeos/input_method/xkeyboard.cc

Index: chrome/browser/chromeos/input_method/xkeyboard.cc
diff --git a/chrome/browser/chromeos/input_method/xkeyboard.cc b/chrome/browser/chromeos/input_method/xkeyboard.cc
index aaf1444d63d38f12a75c544d7a8dd85af96ab3cb..0bf20ead5cff34db374ebab362a7ab59edb33603 100644
--- a/chrome/browser/chromeos/input_method/xkeyboard.cc
+++ b/chrome/browser/chromeos/input_method/xkeyboard.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/input_method/xkeyboard.h"
 
+#include <queue>
 #include <utility>
 
 #include <X11/XKBlib.h>
@@ -18,6 +19,7 @@
 #include "base/process_util.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/input_method/input_method_util.h"
+#include "content/browser/browser_thread.h"
 
 namespace chromeos {
 namespace input_method {
@@ -228,17 +230,17 @@ class XKeyboard {
       return false;
     }
 
-    const std::string layouts_to_set = CreateFullXkbLayoutName(
+    const std::string layout_to_set = CreateFullXkbLayoutName(
         layout_name, modifier_map);
-    if (layouts_to_set.empty()) {
+    if (layout_to_set.empty()) {
       return false;
     }
 
     if (!current_layout_name_.empty()) {
       const std::string current_layout = CreateFullXkbLayoutName(
           current_layout_name_, current_modifier_map_);
-      if (!force && (current_layout == layouts_to_set)) {
-        DLOG(INFO) << "The requested layout is already set: " << layouts_to_set;
+      if (!force && (current_layout == layout_to_set)) {
+        DLOG(INFO) << "The requested layout is already set: " << layout_to_set;
         return true;
       }
     }
@@ -251,28 +253,41 @@ class XKeyboard {
 
     // TODO(yusukes): Revert to VLOG(1) when crosbug.com/15851 is resolved.
     LOG(WARNING) << (force ? "Reapply" : "Set")
-                 << " layout: " << layouts_to_set;
+                 << " layout: " << layout_to_set;
 
-    ExecuteSetLayoutCommand(layouts_to_set);
+    const bool start_execution = execute_queue_.empty();
+    execute_queue_.push(layout_to_set);
+    if (start_execution) {

[satorux1, 2011/06/07 05:41:29]

I thought we could move the queue management logic to
MaybeExecuteSetLayoutCommand();

Something like below may be a bit cleaner.

  execute_queue_.push(layout_to_set);
  MaybeExecuteSetLayoutCommand();


void MaybeExecuteSetLayoutCommand() {
  // We execute the setxkbmap command one at a time.
  // OnSetLayoutFinish() dequeues and retries as needed.
  if (execute_queue_.size() != 1)
    return;

[Yusuke Sato, 2011/06/07 05:51:43]

I think it will not work if SetLayout() is rapidly called more than 3 times.

SetLayout(A);  // queue.size() == 1, call setxkbmap
SetLayout(B);  // queue.size() == 2, do not call setxkbmap since the size != 1
SetLayout(C);  // queue.size() == 3, do not call setxkbmap since the size != 1
OnSetLayoutFinish() for A; // queue.pop() is called first and queue.size()
becomes 2, the next setxkbmap will NOT be called since the size != 1

On 2011/06/07 05:41:29, satorux1 wrote:

[satorux1, 2011/06/07 05:59:13]

Thank you for the explanation. The code wasn't obvious to me. Would be nice to
add a comment why you are doing this here.


On 2011/06/07 05:51:43, Yusuke Sato wrote:

[Yusuke Sato, 2011/06/07 06:53:28]

Done.

+      // No setxkbmap command is in flight. Start the first one.
+      MaybeExecuteSetLayoutCommand();
+    }
     return true;
   }
 
-  // Executes 'setxkbmap -layout ...' command asynchronously.
+  // Executes 'setxkbmap -layout ...' command asynchronously using a layout name
+  // in the |execute_queue_|. Do nothing if the queue is empty.
   // TODO(yusukes): Use libxkbfile.so instead of the command (crosbug.com/13105)
-  void ExecuteSetLayoutCommand(const std::string& layouts_to_set) {
+  void MaybeExecuteSetLayoutCommand() {
+    if (execute_queue_.empty()) {
+      return;
+    }
+    const std::string layout_to_set = execute_queue_.front();
+
     std::vector<std::string> argv;
     base::file_handle_mapping_vector fds_to_remap;
     base::ProcessHandle handle = base::kNullProcessHandle;
 
     argv.push_back(kSetxkbmapCommand);
     argv.push_back("-layout");
-    argv.push_back(layouts_to_set);
+    argv.push_back(layout_to_set);
+    argv.push_back("-synch");
     const bool result = base::LaunchApp(argv,
                                         fds_to_remap,  // No remapping.
                                         false,  // Don't wait.
                                         &handle);
     if (!result) {
-      LOG(ERROR) << "Failed to execute setxkbmap: " << layouts_to_set;
+      LOG(ERROR) << "Failed to execute setxkbmap: " << layout_to_set;
+      execute_queue_ = std::queue<std::string>();  // clear the queue.
       return;
     }
 
@@ -282,16 +297,27 @@ class XKeyboard {
     g_child_watch_add(pid,
                       reinterpret_cast<GChildWatchFunc>(OnSetLayoutFinish),
                       this);
+    VLOG(1) << "ExecuteSetLayoutCommand: " << layout_to_set << ": pid=" << pid;
   }
 
   static void OnSetLayoutFinish(GPid pid, gint status, XKeyboard* self) {
-    DLOG(INFO) << "OnSetLayoutFinish: pid=" << pid;
+    CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    VLOG(1) << "OnSetLayoutFinish: pid=" << pid;
+    if (self->execute_queue_.empty()) {
+      LOG(ERROR) << "OnSetLayoutFinish: execute_queue_ is empty. "
+                 << "base::LaunchApp failed? pid=" << pid;
+      return;
+    }
+    self->execute_queue_.pop();
+    self->MaybeExecuteSetLayoutCommand();
   }
 
   // The XKB layout name which we set last time like "us" and "us(dvorak)".
   std::string current_layout_name_;
   // The mapping of modifier keys we set last time.
   ModifierMap current_modifier_map_;
+  // A queue for executing setxkbmap one by one.
+  std::queue<std::string> execute_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(XKeyboard);
 };