Avoid holding locks across sync IPCs in command buffer

gpu/command_buffer/client/share_group.cc

 // Copyright (c) 2012 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 <stack>
 #include <vector>
 
 #include "gpu/command_buffer/client/share_group.h"
 
 #include "base/logging.h"
@@ skipping 31 matching lines @@
         ids[ii] = id_allocator_.AllocateIDAtOrAbove(id_offset);
         id_offset = ids[ii] + 1;
       }
     }
   }
 
   // Overridden from IdHandlerInterface.
   virtual bool FreeIds(
       GLES2Implementation* gl_impl,
       GLsizei n, const GLuint* ids, DeleteFn delete_fn) OVERRIDE {
+    // 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.

 
-    (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();
     return true;
   }
 
   // Overridden from IdHandlerInterface.
   virtual bool MarkAsUsedForBind(GLuint id) OVERRIDE {
     if (id == 0)
       return true;
     base::AutoLock auto_lock(lock_);
     return id_allocator_.MarkAsUsed(id);
   }
@@ skipping 176 matching lines @@
 }
 
 void ShareGroup::set_program_info_manager(ProgramInfoManager* manager) {
   program_info_manager_.reset(manager);
 }
 
 ShareGroup::~ShareGroup() {}
 
 }  // namespace gles2
 }  // namespace gpu