Avoid holding locks across sync IPCs in command buffer

gpu/command_buffer/client/share_group.cc

Index: gpu/command_buffer/client/share_group.cc
diff --git a/gpu/command_buffer/client/share_group.cc b/gpu/command_buffer/client/share_group.cc
index 96ab8d181be01ab52edda453043ad4bbd451bf78..46413d17a8c07e0faf1b159049d14d156d284e88 100644
--- a/gpu/command_buffer/client/share_group.cc
+++ b/gpu/command_buffer/client/share_group.cc
@@ -49,18 +49,21 @@ class IdHandler : public IdHandlerInterface {
   virtual bool FreeIds(
       GLES2Implementation* gl_impl,
       GLsizei n, const GLuint* ids, DeleteFn delete_fn) OVERRIDE {
-    base::AutoLock auto_lock(lock_);
-
-    for (GLsizei ii = 0; ii < n; ++ii) {
-      id_allocator_.FreeID(ids[ii]);
-    }
-
+    // Possibly may do a synchronous ipc, so can't hold a lock across it on
+    // pepper: http://crbug.com/418651
     (gl_impl->*delete_fn)(n, ids);
     // We need to ensure that the delete call is evaluated on the service side
     // before any other contexts issue commands using these client ids.
     // TODO(vmiura): Can remove this by virtualizing internal ids, however
     // this code only affects PPAPI for now.
     gl_impl->helper()->CommandBufferHelper::Flush();
+
+    base::AutoLock auto_lock(lock_);
+
+    for (GLsizei ii = 0; ii < n; ++ii) {
+      id_allocator_.FreeID(ids[ii]);
+    }

[piman, 2014/10/02 21:44:02]

I think this change would cause a potential race if bind_generates_resource is
true, the race leading to a command buffer loss.

Thread 1 does glDeleteTextures(1, &id)
Thread 2 does glBindTexture(GL_TEXTURE_2D, id), glFlush()

Where id is the same.

If the ordering is such as the bind + flush in Thread 2 happens after the l.59
(the CommandBufferHelper::Flush) on thread 1, but before the lock+FreeId, then
we will send a bind command to the GPU process, which will recreate the client
id on the service side (since it happened after the delete), but even though it
marks it as used on the client side (on thread 2), it will be freed right away
(on thread 1). So after that point we have an inconsistency between the service
and client side, and the next glGenTextures that reuses the client id (since
it's freed on the client side) will lose the context on the service side
(GLES2DecoderImpl::GenTexturesHelper returns false because the client id is in
use, so GLES2DecoderImpl::HandleGenTexturesImmediate raises the
kInvalidArguments error).

We really need the sending of the Delete command and the FreeID to be atomic
when bind_generates_resource is true, unless we add one more level of
virtualization.

[Peng, 2014/12/05 18:16:42]

I think delete_fn and Flush() do not use sync IPC currently. So it is fine to
call them with the lock locked (will not cause deadlock in pepper). Without the
change in this file, does other part of the CL LGTY?

[piman, 2014/12/05 20:51:49]

delete_fn will use a sync IPC in some cases, e.g. if the command buffer is full.

+
     return true;
   }