Split WebGraphicsContext3DCommandBufferImpl::initialize() into two stages.

content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc

Index: content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc
===================================================================
--- content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc	(revision 98270)
+++ content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc	(working copy)
@@ -21,6 +21,7 @@
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/synchronization/lock.h"
 #include "content/common/content_switches.h"
 #include "content/renderer/gpu/gpu_channel_host.h"
 #include "content/renderer/render_thread.h"
@@ -32,6 +33,8 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
 #include "webkit/glue/gl_bindings_skia_cmd_buffer.h"
 
+static base::LazyInstance<base::Lock>
+    g_all_contexts_lock(base::LINKER_INITIALIZED);
 static base::LazyInstance<std::set<WebGraphicsContext3DCommandBufferImpl*> >
     g_all_contexts(base::LINKER_INITIALIZED);
 
@@ -54,7 +57,10 @@
 
 WebGraphicsContext3DCommandBufferImpl::
     ~WebGraphicsContext3DCommandBufferImpl() {
-  g_all_contexts.Pointer()->erase(this);
+  {
+    base::AutoLock lock(g_all_contexts_lock.Get());
+    g_all_contexts.Pointer()->erase(this);
+  }
   delete context_;
 }
 
@@ -70,25 +76,61 @@
     WebGraphicsContext3D::Attributes attributes,
     WebKit::WebView* web_view,
     bool render_directly_to_web_view) {
+  DCHECK(!context_);
   TRACE_EVENT0("gpu", "WebGfxCtx3DCmdBfrImpl::initialize");
   webkit_glue::BindSkiaToCommandBufferGL();
   RenderThread* render_thread = RenderThread::current();
   if (!render_thread)
     return false;
-  GpuChannelHost* host = render_thread->EstablishGpuChannelSync(
+  host_ = render_thread->EstablishGpuChannelSync(
     content::
       CAUSE_FOR_GPU_LAUNCH_WEBGRAPHICSCONTEXT3DCOMMANDBUFFERIMPL_INITIALIZE);
-  if (!host)
+  if (!host_)
     return false;
-  DCHECK(host->state() == GpuChannelHost::kConnected);
+  DCHECK(host_->state() == GpuChannelHost::kConnected);
 
+  const GPUInfo& gpu_info = host_->gpu_info();
+  UMA_HISTOGRAM_ENUMERATION(
+      "GPU.WebGraphicsContext3D_Init_CanLoseContext",
+      attributes.canRecoverFromContextLoss * 2 + gpu_info.can_lose_context,
+      4);
+  if (attributes.canRecoverFromContextLoss == false) {
+    if (gpu_info.can_lose_context)
+      return false;
+  }
+
+  if (web_view && web_view->mainFrame())
+    active_url_ = GURL(web_view->mainFrame()->document().url());
+
+  attributes_ = attributes;
+  render_directly_to_web_view_ = render_directly_to_web_view;
+  if (render_directly_to_web_view_) {
+    RenderView* renderview = RenderView::FromWebView(web_view);
+    if (!renderview)
+      return false;
+    render_view_routing_id_ = renderview->routing_id(),
+#ifndef WTF_USE_THREADED_COMPOSITING
+    web_view_ = web_view;
+  } else {
+    web_view_ = NULL;

[jamesr, 2011/08/25 20:05:53]

web_view_ is set to NULL in the c'tor so no need to set it here

[lain Merrick, 2011/08/25 20:11:17]

Done.

+#endif
+  }
+  return true;
+}
+
+bool WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL() {
+  if (context_) {
+    return true;
+  }
+  TRACE_EVENT0("gpu", "WebGfxCtx3DCmdBfrImpl::MaybeInitializeGL");
+
   // Convert WebGL context creation attributes into RendererGLContext / EGL size
   // requests.
-  const int alpha_size = attributes.alpha ? 8 : 0;
-  const int depth_size = attributes.depth ? 24 : 0;
-  const int stencil_size = attributes.stencil ? 8 : 0;
-  const int samples = attributes.antialias ? 4 : 0;
-  const int sample_buffers = attributes.antialias ? 1 : 0;
+  const int alpha_size = attributes_.alpha ? 8 : 0;
+  const int depth_size = attributes_.depth ? 24 : 0;
+  const int stencil_size = attributes_.stencil ? 8 : 0;
+  const int samples = attributes_.antialias ? 4 : 0;
+  const int sample_buffers = attributes_.antialias ? 1 : 0;
   const int32 attribs[] = {
     RendererGLContext::ALPHA_SIZE, alpha_size,
     RendererGLContext::DEPTH_SIZE, depth_size,
@@ -98,49 +140,29 @@
     RendererGLContext::NONE,
   };
 
-  const char* preferred_extensions = attributes.noExtensions ?
-      kWebGLPreferredGLExtensions : "*";
-
-  const GPUInfo& gpu_info = host->gpu_info();
-  UMA_HISTOGRAM_ENUMERATION(
-      "GPU.WebGraphicsContext3D_Init_CanLoseContext",
-      attributes.canRecoverFromContextLoss * 2 + gpu_info.can_lose_context,
-      4);
-  if (attributes.canRecoverFromContextLoss == false) {
-    if (gpu_info.can_lose_context)
-      return false;
-  }
-
-  GURL active_url;
-  if (web_view && web_view->mainFrame())
-    active_url = GURL(web_view->mainFrame()->document().url());
-
   // HACK: Assume this is a WebGL context by looking for the noExtensions
   // attribute. WebGL contexts must not go in the share group because they
   // rely on destruction of the context to clean up owned resources. Putting
   // them in a share group would prevent this from happening.
+  base::AutoLock lock(g_all_contexts_lock.Get());

[jamesr, 2011/08/25 20:05:53]

this lock is held way too long, please tighten it up. I think you only need it
until line 151, and you don't need to take it at all unless the if on 149
succeeds

[lain Merrick, 2011/08/25 20:11:17]

I was thinking of commenting this. I think it's needed because if the context
lives in another thread, it could be destroyed between here and line 158.

Unlock on line 180, maybe? Or if it's safe (but suboptimal) to use a dead
context as the share group, I could just unlock on 151, yeah.

   RendererGLContext* share_group = NULL;
-  if (!attributes.noExtensions) {
+  if (!attributes_.noExtensions) {
     share_group = g_all_contexts.Pointer()->empty() ?
         NULL : (*g_all_contexts.Pointer()->begin())->context_;
   }
 
-  render_directly_to_web_view_ = render_directly_to_web_view;
-  if (render_directly_to_web_view) {
-#ifndef WTF_USE_THREADED_COMPOSITING
-    RenderView* renderview = RenderView::FromWebView(web_view);
-    if (!renderview)
-      return false;
-    web_view_ = web_view;
-#endif
+  const char* preferred_extensions = attributes_.noExtensions ?
+      kWebGLPreferredGLExtensions : "*";
+
+  if (render_directly_to_web_view_) {
     context_ = RendererGLContext::CreateViewContext(
-        host,
-        renderview->routing_id(),
-        !attributes.noExtensions,
+        host_,
+        render_view_routing_id_,
+        !attributes_.noExtensions,
         share_group,
         preferred_extensions,
         attribs,
-        active_url);
+        active_url_);
     if (context_) {
       context_->SetSwapBuffersCallback(
           NewCallback(this,
@@ -148,16 +170,13 @@
     }
   } else {
     context_ = RendererGLContext::CreateOffscreenContext(
-        host,
+        host_,
         gfx::Size(1, 1),
-        !attributes.noExtensions,
+        !attributes_.noExtensions,
         share_group,
         preferred_extensions,
         attribs,
-        active_url);
-#ifndef WTF_USE_THREADED_COMPOSITING
-    web_view_ = NULL;
-#endif
+        active_url_);
   }
   if (!context_)
     return false;
@@ -175,7 +194,6 @@
 
   // Set attributes_ from created offscreen context.
   {
-    attributes_ = attributes;
     GLint alpha_bits = 0;
     getIntegerv(GL_ALPHA_BITS, &alpha_bits);
     attributes_.alpha = alpha_bits > 0;
@@ -190,13 +208,15 @@
     attributes_.antialias = samples > 0;
   }
 
-  if (!attributes.noExtensions)
+  if (!attributes_.noExtensions)
     g_all_contexts.Pointer()->insert(this);

[jamesr, 2011/08/25 20:05:53]

you forgot a lock here

[lain Merrick, 2011/08/25 20:11:17]

Still inside the lock on line 147 (but maybe that's way too long)

 
   return true;
 }
 
 bool WebGraphicsContext3DCommandBufferImpl::makeContextCurrent() {
+  if (!MaybeInitializeGL())
+    return false;
   return RendererGLContext::MakeCurrent(context_);
 }
 
@@ -221,7 +241,6 @@
 }
 
 WebGLId WebGraphicsContext3DCommandBufferImpl::getPlatformTextureId() {
-  DCHECK(context_);
   return context_->GetParentTextureId();
 }