Process SelectionRequestor::PerformBlockingConvertSelection() requests one at a time

ui/base/x/selection_requestor.cc

Index: ui/base/x/selection_requestor.cc
diff --git a/ui/base/x/selection_requestor.cc b/ui/base/x/selection_requestor.cc
index 516e0e0398c558b5bb0de603fe6a175ab840af7a..ac092ea3257766af933312ebc6434fa787f3bcbf 100644
--- a/ui/base/x/selection_requestor.cc
+++ b/ui/base/x/selection_requestor.cc
@@ -15,10 +15,22 @@ namespace ui {
 
 namespace {
 
-const char kChromeSelection[] = "CHROME_SELECTION";
+// Properties to be used in XConvertSelection() requests. The number of
+// properties is arbitrary.
+const char* kChromeSelectionProperties[] = {
+    "CHROME_SELECTION0",
+    "CHROME_SELECTION1",
+    "CHROME_SELECTION2",
+    "CHROME_SELECTION3",
+    "CHROME_SELECTION4"
+};
 
 const char* kAtomsToCache[] = {
-  kChromeSelection,
+  kChromeSelectionProperties[0],
+  kChromeSelectionProperties[1],
+  kChromeSelectionProperties[2],
+  kChromeSelectionProperties[3],
+  kChromeSelectionProperties[4],
   NULL
 };
 
@@ -26,65 +38,61 @@ const char* kAtomsToCache[] = {
 
 SelectionRequestor::SelectionRequestor(Display* x_display,
                                        Window x_window,
-                                       Atom selection_name,
                                        PlatformEventDispatcher* dispatcher)
     : x_display_(x_display),
       x_window_(x_window),
-      selection_name_(selection_name),
       dispatcher_(dispatcher),
+      property_pool_(kChromeSelectionProperties,
+                     kChromeSelectionProperties
+                         + arraysize(kChromeSelectionProperties)),
       atom_cache_(x_display_, kAtomsToCache) {
 }
 
 SelectionRequestor::~SelectionRequestor() {}
 
 bool SelectionRequestor::PerformBlockingConvertSelection(
+    Atom selection,
     Atom target,
     scoped_refptr<base::RefCountedMemory>* out_data,
     size_t* out_data_items,
     Atom* out_type) {
-  // The name of the property that we are either:
-  // - Passing as a parameter with the XConvertSelection() request.
-  // OR
-  // - Asking the selection owner to set on |x_window_|.
-  Atom property = atom_cache_.GetAtom(kChromeSelection);
-
-  XConvertSelection(x_display_,
-                    selection_name_,
-                    target,
-                    property,
-                    x_window_,
-                    CurrentTime);
-
-  // Now that we've thrown our message off to the X11 server, we block waiting
-  // for a response.
-  PendingRequest pending_request(target);
-  BlockTillSelectionNotifyForRequest(&pending_request);
-
-  bool success = false;
-  if (pending_request.returned_property == property) {
-    success =  ui::GetRawBytesOfProperty(x_window_,
-                                         pending_request.returned_property,
-                                         out_data, out_data_items, out_type);
-  }
-  if (pending_request.returned_property != None)
-    XDeleteProperty(x_display_, x_window_, pending_request.returned_property);
+  // Find a property for XConvertSelection() to use. The selection owner will
+  // set |property| on |xwindow_| with the data for the requested target. If
+  // there is no available property, make the request fail.
+  if (property_pool_.empty())
+    return false;
+
+  const char* property_name = property_pool_.back();
+  property_pool_.pop_back();

[Daniel Erat, 2014/07/15 01:04:09]

with the disclaimer that i don't know much about selections, and i didn't know
anything about INCR until just now: what's the reason for having multiple atoms
here? it looks like you pop the back one off, use it synchronously, and then put
it back at the end of the list. if i'm not misreading the code, won't this
result in both this method and PerformBlockingConvertSelectionWithParameter()
always only using the last atom? if so, what's the reason for having multiple
atoms?

[pkotwicz, 2014/07/15 02:07:24]

The problem is that there are callers of clipboard functions from posted tasks.
So a second PerformBlockingConvertSelection() call can be made before the first
one completes.
This is the case with chrome.bookmarkManagerPrivate.canPaste

[Daniel Erat, 2014/07/15 03:32:01]

oh. is the nested message loop in BlockTillSelectionNotifyForRequest() where the
second call can get run?

+  bool success = PerformBlockingConvertSelectionImpl(selection, target,
+      property_name, out_data, out_data_items, out_type);
+  property_pool_.push_back(property_name);
   return success;
 }
 
 void SelectionRequestor::PerformBlockingConvertSelectionWithParameter(
+    Atom selection,
     Atom target,
     const std::vector< ::Atom>& parameter) {
-  SetAtomArrayProperty(x_window_, kChromeSelection, "ATOM", parameter);
-  PerformBlockingConvertSelection(target, NULL, NULL, NULL);
+  if (property_pool_.empty())
+    return;
+  const char* property_name = property_pool_.back();
+  property_pool_.pop_back();
+  SetAtomArrayProperty(x_window_, property_name, "ATOM", parameter);
+  PerformBlockingConvertSelectionImpl(selection, target, property_name, NULL,
+      NULL, NULL);
+  property_pool_.push_back(property_name);
 }
 
 SelectionData SelectionRequestor::RequestAndWaitForTypes(
+    Atom selection,
     const std::vector< ::Atom>& types) {
   for (std::vector< ::Atom>::const_iterator it = types.begin();
        it != types.end(); ++it) {
     scoped_refptr<base::RefCountedMemory> data;
     ::Atom type = None;
-    if (PerformBlockingConvertSelection(*it,
+    if (PerformBlockingConvertSelection(selection,
+                                        *it,
                                         &data,
                                         NULL,
                                         &type) &&
@@ -101,14 +109,12 @@ void SelectionRequestor::OnSelectionNotify(const XSelectionEvent& event) {
   // there are multiple pending requests on the same target, satisfy them in
   // FIFO order.
   PendingRequest* request_notified = NULL;
-  if (selection_name_ == event.selection) {
-    for (std::list<PendingRequest*>::iterator iter = pending_requests_.begin();
-         iter != pending_requests_.end(); ++iter) {
-      PendingRequest* request = *iter;
-      if (request->returned)
-        continue;
-      if (request->target != event.target)
-        continue;
+  for (std::list<PendingRequest*>::iterator iter = pending_requests_.begin();
+       iter != pending_requests_.end(); ++iter) {
+    PendingRequest* request = *iter;
+    if (!request->returned &&
+        request->selection == event.selection &&
+        request->target == event.target) {
       request_notified = request;
       break;
     }
@@ -133,6 +139,37 @@ void SelectionRequestor::OnSelectionNotify(const XSelectionEvent& event) {
     request_notified->quit_closure.Run();
 }
 
+bool SelectionRequestor::PerformBlockingConvertSelectionImpl(
+    Atom selection,
+    Atom target,
+    const char* property_name,
+    scoped_refptr<base::RefCountedMemory>* out_data,
+    size_t* out_data_items,
+    Atom* out_type) {
+  Atom property = atom_cache_.GetAtom(property_name);
+  XConvertSelection(x_display_,
+                    selection,
+                    target,
+                    property,
+                    x_window_,
+                    CurrentTime);
+
+  // Now that we've thrown our message off to the X11 server, we block waiting
+  // for a response.
+  PendingRequest pending_request(selection, target);
+  BlockTillSelectionNotifyForRequest(&pending_request);
+
+  bool success = false;
+  if (pending_request.returned_property == property) {
+    success = ui::GetRawBytesOfProperty(x_window_,
+                                        pending_request.returned_property,
+                                        out_data, out_data_items, out_type);
+  }
+  if (pending_request.returned_property != None)
+    XDeleteProperty(x_display_, x_window_, pending_request.returned_property);
+  return success;
+}
+
 void SelectionRequestor::BlockTillSelectionNotifyForRequest(
     PendingRequest* request) {
   pending_requests_.push_back(request);
@@ -171,8 +208,10 @@ void SelectionRequestor::BlockTillSelectionNotifyForRequest(
   pending_requests_.pop_back();
 }
 
-SelectionRequestor::PendingRequest::PendingRequest(Atom target)
-    : target(target),
+SelectionRequestor::PendingRequest::PendingRequest(Atom requested_selection,
+                                                   Atom requested_target)
+    : selection(requested_selection),
+      target(requested_target),
       returned_property(None),
       returned(false) {
 }