Plumbing explicit share groups through context creation

content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc

Index: content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
diff --git a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
index 52e99bc21546408ace761a6098daef49a0b268e6..97d79bef2b8a905b5854bc79e8347e4110911d33 100644
--- a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
+++ b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
@@ -22,7 +22,6 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
-#include "base/synchronization/lock.h"
 #include "content/common/gpu/client/gpu_channel_host.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/content_switches.h"
@@ -202,13 +201,21 @@ WebGraphicsContext3DCommandBufferImpl::SharedMemoryLimits::SharedMemoryLimits()
       max_transfer_buffer_size(kDefaultMaxTransferBufferSize),
       mapped_memory_reclaim_limit(gpu::gles2::GLES2Implementation::kNoLimit) {}
 
+WebGraphicsContext3DCommandBufferImpl::ShareGroup::ShareGroup() {
+}
+
+WebGraphicsContext3DCommandBufferImpl::ShareGroup::~ShareGroup() {
+  DCHECK(contexts_.empty());
+}
+
 WebGraphicsContext3DCommandBufferImpl::WebGraphicsContext3DCommandBufferImpl(
     int surface_id,
     const GURL& active_url,
     GpuChannelHost* host,
     const Attributes& attributes,
     bool bind_generates_resources,
-    const SharedMemoryLimits& limits)
+    const SharedMemoryLimits& limits,
+    WebGraphicsContext3DCommandBufferImpl* share_context)
     : initialize_failed_(false),
       visible_(false),
       host_(host),
@@ -226,6 +233,12 @@ WebGraphicsContext3DCommandBufferImpl::WebGraphicsContext3DCommandBufferImpl(
       bind_generates_resources_(bind_generates_resources),
       mem_limits_(limits),
       flush_id_(0) {
+  if (share_context) {
+    share_group_ = share_context->share_group_;
+  } else {
+    share_group_ = attributes_.shareResources ? NULL
+                                              : new ShareGroup();
+  }
 }
 
 WebGraphicsContext3DCommandBufferImpl::
@@ -295,19 +308,26 @@ bool WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL() {
 }
 
 bool WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
-    bool onscreen) {
+    bool onscreen, WebGraphicsContext3DCommandBufferImpl* share_context) {
   if (!host_.get())
     return false;
+
+  CommandBufferProxyImpl* share_group_command_buffer = NULL;
+
+  if (share_context) {
+    share_group_command_buffer = share_context->command_buffer_.get();
+  }
+
   // We need to lock g_all_shared_contexts to ensure that the context we picked
   // for our share group isn't deleted.
   // (There's also a lock in our destructor.)
   base::AutoLock lock(g_all_shared_contexts_lock.Get());
-  CommandBufferProxyImpl* share_group = NULL;
-  if (attributes_.shareResources) {
+
+  if (attributes_.shareResources && !share_group_command_buffer) {
     ContextMap& all_contexts = g_all_shared_contexts.Get();
     ContextMap::const_iterator it = all_contexts.find(host_.get());
     if (it != all_contexts.end())
-      share_group = it->second->command_buffer_.get();
+      share_group_command_buffer = it->second->command_buffer_.get();
   }
 
   std::vector<int32> attribs;
@@ -329,14 +349,14 @@ bool WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
   if (onscreen) {
     command_buffer_.reset(host_->CreateViewCommandBuffer(
         surface_id_,
-        share_group,
+        share_group_command_buffer,
         attribs,
         active_url_,
         gpu_preference_));
   } else {
     command_buffer_.reset(host_->CreateOffscreenCommandBuffer(
         gfx::Size(1, 1),
-        share_group,
+        share_group_command_buffer,
         attribs,
         active_url_,
         gpu_preference_));
@@ -349,14 +369,29 @@ bool WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
   return command_buffer_->Initialize();
 }
 
-bool WebGraphicsContext3DCommandBufferImpl::CreateContext(
-    bool onscreen) {
+bool WebGraphicsContext3DCommandBufferImpl::CreateContext(bool onscreen) {
   // Ensure the gles2 library is initialized first in a thread safe way.
   g_gles2_initializer.Get();
 
-  if (!command_buffer_ &&
-      !InitializeCommandBuffer(onscreen)) {
-    return false;
+  scoped_refptr<gpu::gles2::ShareGroup> gles2_share_group;
+
+  if (!command_buffer_) {
+    scoped_ptr<base::AutoLock> share_group_lock;
+    WebGraphicsContext3DCommandBufferImpl* share_context = NULL;
+
+    if (share_group_) {
+      share_group_lock.reset(new base::AutoLock(share_group_->lock()));
+      share_context = share_group_->GetAnyContext();
+    }
+
+    if (!InitializeCommandBuffer(onscreen, share_context))
+      return false;
+
+    if (share_context)
+      gles2_share_group = share_context->GetImplementation()->share_group();
+
+    if (share_group_)
+      share_group_->AddContext(this);
   }

[piman, 2013/12/12 23:56:39]

So, there is a race risk here.
After you release the share_group_lock, the share_context could be deleted (on
another thread), and could be the last context in gles2_share_group.
If that's the case, on the service side, we will destroy the corresponding
ContextGroup, and free all remaining texture ids and friends.
However, because you keep the gles2::ShareGroup and will use it in the
GLES2Implementation, you get into the state where the ShareGroup doesn't match
the ContextGroup any more (it could have ids allocated on the client side that
don't exist on the service side), leading to problems later on.

I think you need to keep the lock held between when you grab the context and
both:
- you create the service-side context (InitializeCommandBuffer)
- you create the client-side context (GLES2Implementation).

At that point, the interaction between share_group_'s lock and the
g_all_shared_contexts_lock is starting to become complex.

At the same time, g_all_shared_contexts is really just like another ShareGroup.
Maybe we could combine both? Have a per-GpuChannelHost ShareGroup for
WebGraphicsContext3DCommandBufferImpl that have Attributes::shareResources set
and no explicit ShareGroup, and pick that in the constructor, making the rest of
the code common. WDYT?

[bajones, 2013/12/13 00:17:20]

I like it! I was thinking about replacing g_all_shared_contexts with a
ShareGroup, but hadn't considered attaching it to the GpuChannelHost, which
simplifies object lifetime management significantly.

Thanks for the suggestion!

 
   // Create the GLES2 helper, which writes the command buffer protocol.
@@ -373,16 +408,16 @@ bool WebGraphicsContext3DCommandBufferImpl::CreateContext(
 
   DCHECK(host_.get());
   scoped_ptr<base::AutoLock> lock;
-  scoped_refptr<gpu::gles2::ShareGroup> share_group;
-  if (attributes_.shareResources) {
+
+  if (attributes_.shareResources && !gles2_share_group) {
     // Make sure two clients don't try to create a new ShareGroup
     // simultaneously.
     lock.reset(new base::AutoLock(g_all_shared_contexts_lock.Get()));
     ContextMap& all_contexts = g_all_shared_contexts.Get();
     ContextMap::const_iterator it = all_contexts.find(host_.get());
     if (it != all_contexts.end()) {
-      share_group = it->second->GetImplementation()->share_group();
-      DCHECK(share_group);
+      gles2_share_group = it->second->GetImplementation()->share_group();
+      DCHECK(gles2_share_group);
     }
   }
 
@@ -393,7 +428,7 @@ bool WebGraphicsContext3DCommandBufferImpl::CreateContext(
   // Create the object exposing the OpenGL API.
   real_gl_.reset(new gpu::gles2::GLES2Implementation(
       gles2_helper_.get(),
-      share_group,
+      gles2_share_group,
       transfer_buffer_.get(),
       bind_generates_resources_,
       free_command_buffer_when_invisible,
@@ -440,6 +475,10 @@ uint32_t WebGraphicsContext3DCommandBufferImpl::lastFlushID() {
 DELEGATE_TO_GL_R(insertSyncPoint, InsertSyncPointCHROMIUM, unsigned int)
 
 void WebGraphicsContext3DCommandBufferImpl::Destroy() {
+  if (share_group_) {
+    share_group_->RemoveContext(this);
+  }
+
   if (host_.get()) {
     base::AutoLock lock(g_all_shared_contexts_lock.Get());
     ContextMap& all_contexts = g_all_shared_contexts.Get();
@@ -1192,15 +1231,24 @@ WebGraphicsContext3DCommandBufferImpl::CreateOffscreenContext(
     GpuChannelHost* host,
     const WebGraphicsContext3D::Attributes& attributes,
     const GURL& active_url,
-    const SharedMemoryLimits& limits) {
+    const SharedMemoryLimits& limits,
+    WebGraphicsContext3D* share_context) {

[piman, 2013/12/12 23:56:39]

nit: pass a WebGraphicsContext3DCommandBufferImpl here, making it explicit to
the caller that it's responsible for ensuring the type correctness.

   if (!host)
     return NULL;
+
+  WebGraphicsContext3DCommandBufferImpl* share_context_impl = NULL;
+  if (share_context) {
+    share_context_impl =
+        static_cast<WebGraphicsContext3DCommandBufferImpl*>(share_context);
+  }
+
   return new WebGraphicsContext3DCommandBufferImpl(0,
                                                    active_url,
                                                    host,
                                                    attributes,
                                                    false,
-                                                   limits);
+                                                   limits,
+                                                   share_context_impl);
 }
 
 DELEGATE_TO_GL_5(texImageIOSurface2DCHROMIUM, TexImageIOSurface2DCHROMIUM,
@@ -1370,6 +1418,9 @@ void WebGraphicsContext3DCommandBufferImpl::OnGpuChannelLost() {
     base::AutoLock lock(g_all_shared_contexts_lock.Get());
     g_all_shared_contexts.Get().erase(host_.get());
   }
+
+  if (share_group_)
+    share_group_->RemoveAllContexts();
 }
 
 void WebGraphicsContext3DCommandBufferImpl::OnErrorMessage(