GPU: Flush in glBind* to avoid being executed after future glDelete*

gpu/command_buffer/client/gles2_implementation.cc

Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index 51c63707421276e368442b4ba22fe6761b64bf9c..b859a388625fa4dbb89cd4d663e6442897a10317 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -2321,6 +2321,15 @@ bool GLES2Implementation::BindBufferHelper(
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used even though it's marked it as used here.
   GetIdHandler(id_namespaces::kBuffers)->MarkAsUsedForBind(buffer_id);
+
+  // In Pepper, BindBuffer() may need generate resource in GPU process, so we
+  // have to flush command buffer to make the BindBuffer() being executed in
+  // GPU process before any DeleteBuffer() which are issused in future from
+  // other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
@@ -2361,7 +2370,17 @@ bool GLES2Implementation::BindFramebufferHelper(
       SetGLErrorInvalidEnum("glBindFramebuffer", target, "target");
       return false;
   }
+
   GetIdHandler(id_namespaces::kFramebuffers)->MarkAsUsedForBind(framebuffer);
+
+  // In Pepper, BindFramebuffer() may need generate resource in GPU process,
+  // so we have to flush command buffer to make the BindFramebuffer() being
+  // executed in GPU process before any DeleteFramebuffer() which are issused in
+  // future from other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
@@ -2383,6 +2402,15 @@ bool GLES2Implementation::BindRenderbufferHelper(
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used even though it's marked it as used here.
   GetIdHandler(id_namespaces::kRenderbuffers)->MarkAsUsedForBind(renderbuffer);
+
+  // In Pepper, BindRenderbuffer() may need generate resource in GPU process,
+  // so we have to flush command buffer to make the BindRenderbuffer() being
+  // executed in GPU process before any DeleteRenderbuffer() which are issused
+  // in future from other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();
+
   return changed;
 }
 
@@ -2418,6 +2446,15 @@ bool GLES2Implementation::BindTextureHelper(GLenum target, GLuint texture) {
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used. even though it's marked it as used here.
   GetIdHandler(id_namespaces::kTextures)->MarkAsUsedForBind(texture);
+
+  // In Pepper, BindTexture() may need generate resource in GPU process, so we
+  // have to flush command buffer to make the BindTexture() being executed in
+  // GPU process before any DeleteTexture() which are issused in future from
+  // other contexts.
+  // TODO(penghuang): Get rid of the Flush().
+  if (changed && share_group_->bind_generates_resource())
+    helper_->CommandBufferHelper::Flush();

[piman, 2014/12/01 21:41:53]

This is racy, it needs to be done under the lock in IdHandler. Otherwise a
DeleteTexture on another thread could happen after the MarkAsUsedForBind
(therefore clearing the "in use" bit for the id), but before the Flush here,
therefore causing a mismatch between client and service.

[Peng, 2014/12/02 19:32:36]

For your mentioned case, it should be fine, unless other threads call
DeleteTextures() and Flush() together, and DeleteTextures() & Flush() are both
happen between MarkAsUsedForBind() & Flush(). It is unlikely, almost impossible.
So for short term, this change should be OK. At least, the change does not make
it worse.

I also looked code of GenTextures, GenBuffers, etc. They have the same issue.
They use Flush() without holding the lock in the id allocator too. 

For long term, I think a solution could be to add the 'sync point' concept for
context shared group. All GL commands need read the status of a shared group
will attach a 'sync point' with it. GPU process will not execute those commands
until the sync point being passed. For GL commands which modify status, a sync
point will be attached with it too (The sync points are generated by the shared
group in client side without any IPC). When the GPU process completes the
command, the GPU process will update the shared group sync point with the
attached sync point, and executes any pending commands which are waiting for
that sync point. WDYT?

[Peng, 2014/12/04 17:45:01]

Done. Flush() the command buffer under the lock in IdHandler.

+
   return changed;
 }