Move GLHelper ownership to ImageTransportFactory, and make it thread safe

There were several problems with previous implementation:
- there was a use-after-free in CopyTextureToImpl. ReadBackFramebufferComplete was responsible for deleting the texture, but in some cases it could be called after the GLHelper was destroyed, accessing a deleted field, which could have been overwritten (i.e. we'll delete the wrong texture).
- When creating thumbnails we would create a GLHelper per page, which creates a context (and some resources). That context doesn't go away until we close the page, it's wasteful.
- If we create the thumbnail on closing the page, we would block

Instead, this patch changes it to create a single GLHelper and fixes the cancelling semantics so that we don't have use-after-frees

BUG=123933
TEST=check for no leak

Review URL: https://chromiumcodereview.appspot.com/10290007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@135764 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 63 insertions, 62 deletions

diff --git a/content/browser/renderer_host/image_transport_factory.cc b/content/browser/renderer_host/image_transport_factory.cc
index 9fb5fbc..1a9dc44 100644
--- a/content/browser/renderer_host/image_transport_factory.cc
+++ b/content/browser/renderer_host/image_transport_factory.cc
@@ -14,6 +14,7 @@
 #include "content/browser/gpu/browser_gpu_channel_host_factory.h"
 #include "content/browser/gpu/gpu_surface_tracker.h"
 #include "content/browser/renderer_host/image_transport_client.h"
+#include "content/common/gpu/client/gl_helper.h"
 #include "content/common/gpu/client/gpu_channel_host.h"
 #include "content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h"
 #include "content/common/gpu/gpu_process_launch_causes.h"
@@ -30,6 +31,7 @@
 #include "webkit/gpu/webgraphicscontext3d_in_process_impl.h"
 
 using content::BrowserGpuChannelHostFactory;
+using content::GLHelper;
 
 namespace {
 
@@ -56,17 +58,16 @@ class DefaultTransportFactory
       gfx::GLSurfaceHandle surface) OVERRIDE {
   }
 
-  virtual WebKit::WebGraphicsContext3D* GetSharedContext(
-      ui::Compositor* compositor) OVERRIDE {
-    return NULL;
-  }
-
   virtual scoped_refptr<ImageTransportClient> CreateTransportClient(
       const gfx::Size& size,
       uint64* transport_handle) OVERRIDE {
     return NULL;
   }
 
+  virtual GLHelper* GetGLHelper(ui::Compositor* compositor) OVERRIDE {
+    return NULL;
+  }
+
   virtual gfx::ScopedMakeCurrent* GetScopedMakeCurrent() OVERRIDE {
     return NULL;
   }
@@ -141,12 +142,6 @@ class InProcessTransportFactory : public DefaultTransportFactory {
     return gfx::GLSurfaceHandle(gfx::kNullPluginWindow, true);
   }
 
-  virtual WebKit::WebGraphicsContext3D* GetSharedContext(
-      ui::Compositor* compositor) OVERRIDE {
-    // TODO(mazda): Implement this (http://crbug.com/118546).
-    return NULL;
-  }
-
   virtual scoped_refptr<ImageTransportClient> CreateTransportClient(
       const gfx::Size& size,
       uint64* transport_handle) OVERRIDE {
@@ -159,6 +154,11 @@ class InProcessTransportFactory : public DefaultTransportFactory {
     return surface;
   }
 
+  virtual GLHelper* GetGLHelper(ui::Compositor* compositor) OVERRIDE {
+    // TODO(mazda): Implement this (http://crbug.com/118546).
+    return NULL;
+  }
+
   // We don't generate lost context events, so we don't need to keep track of
   // observers
   virtual void AddObserver(ImageTransportFactoryObserver* observer) OVERRIDE {
@@ -308,15 +308,6 @@ class GpuProcessTransportFactory : public ui::ContextFactory,
     }
   }
 
-  virtual WebKit::WebGraphicsContext3D* GetSharedContext(
-      ui::Compositor* compositor) OVERRIDE {
-    PerCompositorDataMap::const_iterator itr =
-        per_compositor_data_.find(compositor);
-    if (itr == per_compositor_data_.end())
-      return NULL;
-    return itr->second->shared_context.get();
-  }
-
   virtual scoped_refptr<ImageTransportClient> CreateTransportClient(
       const gfx::Size& size,
       uint64* transport_handle) {
@@ -326,6 +317,21 @@ class GpuProcessTransportFactory : public ui::ContextFactory,
     return image;
   }
 
+  virtual GLHelper* GetGLHelper(ui::Compositor* compositor) {
+    PerCompositorData* data = per_compositor_data_[compositor];
+    if (!data)
+      data = CreatePerCompositorData(compositor);
+    if (!data->gl_helper.get()) {
+      WebKit::WebGraphicsContext3D* context_for_thread =
+          CreateContextCommon(compositor, true);
+      if (!context_for_thread)
+        return NULL;
+      data->gl_helper.reset(new GLHelper(data->shared_context.get(),
+                                         context_for_thread));
+    }
+    return data->gl_helper.get();
+  }
+
   virtual gfx::ScopedMakeCurrent* GetScopedMakeCurrent() { return NULL; }
 
   virtual void AddObserver(ImageTransportFactoryObserver* observer) {
@@ -341,6 +347,12 @@ class GpuProcessTransportFactory : public ui::ContextFactory,
     PerCompositorData* data = per_compositor_data_[compositor];
     DCHECK(data);
 
+    // Keep old resources around while we call the observers, but ensure that
+    // new resources are created if needed.
+    scoped_ptr<WebGraphicsContext3DCommandBufferImpl> old_shared_context(
+        data->shared_context.release());
+    scoped_ptr<GLHelper> old_helper(data->gl_helper.release());
+
     // Note: this has the effect of recreating the swap_client, which means we
     // won't get more reports of lost context from the same gpu process. It's a
     // good thing.
@@ -357,6 +369,7 @@ class GpuProcessTransportFactory : public ui::ContextFactory,
     int surface_id;
     scoped_ptr<CompositorSwapClient> swap_client;
     scoped_ptr<WebGraphicsContext3DCommandBufferImpl> shared_context;
+    scoped_ptr<GLHelper> gl_helper;
   };
 
   PerCompositorData* CreatePerCompositorData(ui::Compositor* compositor) {
diff --git a/content/browser/renderer_host/image_transport_factory.h b/content/browser/renderer_host/image_transport_factory.h
index 9416afd..600f314 100644
--- a/content/browser/renderer_host/image_transport_factory.h
+++ b/content/browser/renderer_host/image_transport_factory.h
@@ -9,6 +9,10 @@
 #include "base/memory/ref_counted.h"
 #include "ui/gfx/native_widget_types.h"
 
+namespace content {
+class GLHelper;
+}
+
 namespace gfx {
 class Size;
 class ScopedMakeCurrent;
@@ -33,6 +37,10 @@ class ImageTransportFactoryObserver {
 
   // Notifies that the surface handles generated by ImageTransportFactory for a
   // given compositor were lost.
+  // When this is called, the old resources (e.g. shared context, GL helper)
+  // still exist, but are about to be destroyed. Getting a reference to those
+  // resources from the ImageTransportFactory (e.g. through GetGLHelper) will
+  // return newly recreated, valid resources.
   virtual void OnLostResources(ui::Compositor* compositor) = 0;
 };
 
@@ -67,19 +75,16 @@ class ImageTransportFactory {
   // Destroys a shared surface handle.
   virtual void DestroySharedSurfaceHandle(gfx::GLSurfaceHandle surface) = 0;
 
-  // Returns a shared context associated with the compositor. Returns NULL if
-  // no context is associated with the compositor.
-  // Note: the context may get lost at any time, a state that an
-  // ImageTransportFactoryObserver gets notified of.
-  virtual WebKit::WebGraphicsContext3D* GetSharedContext(
-      ui::Compositor* compositor) = 0;
-
   // Creates a transport client of a given size, and using the opaque handle
   // sent by the GPU process.
   virtual scoped_refptr<ImageTransportClient> CreateTransportClient(
       const gfx::Size& size,
       uint64* transport_handle) = 0;
 
+  // Gets a GLHelper instance, associated with the compositor context. This
+  // GLHelper will get destroyed whenever the context is lost (
+  virtual content::GLHelper* GetGLHelper(ui::Compositor* compositor) = 0;
+
   // Returns a ScopedMakeCurrent that can be used to make current a context that
   // is shared with the compositor context, e.g. to create a texture in its
   // namespace. The caller gets ownership of the object.
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 8f16ab9..8e2aa25 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "content/browser/renderer_host/backing_store_skia.h"
 #include "content/browser/renderer_host/image_transport_client.h"
@@ -104,18 +105,6 @@ bool CanRendererHandleEvent(const aura::MouseEvent* event) {
   return true;
 }
 
-// Creates a content::GLHelper for the given compositor.
-content::GLHelper* CreateGLHelper(ui::Compositor* compositor) {
-  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
-  WebKit::WebGraphicsContext3D* context =
-      factory->GetSharedContext(compositor);
-  DCHECK(context);
-  WebKit::WebGraphicsContext3D* context_for_thread =
-      factory->AsContextFactory()->CreateOffscreenContext(compositor);
-  DCHECK(context_for_thread);
-  return new content::GLHelper(context, context_for_thread);
-}
-
 void GetScreenInfoForWindow(WebKit::WebScreenInfo* results,
                             aura::Window* window) {
   const gfx::Monitor monitor = window ?
@@ -449,15 +438,17 @@ bool RenderWidgetHostViewAura::CopyFromCompositingSurface(
   if (!output->initialize(size.width(), size.height(), true))
     return false;
 
-  if (!gl_helper_.get())
-    gl_helper_.reset(CreateGLHelper(compositor));
+  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+  content::GLHelper* gl_helper = factory->GetGLHelper(compositor);
+  if (!gl_helper)
+    return false;
 
   unsigned char* addr = static_cast<unsigned char*>(
       output->getTopDevice()->accessBitmap(true).getPixels());
-  return gl_helper_->CopyTextureTo(container->texture_id(),
-                                   container->size(),
-                                   size,
-                                   addr);
+  return gl_helper->CopyTextureTo(container->texture_id(),
+                                  container->size(),
+                                  size,
+                                  addr);
 }
 
 void RenderWidgetHostViewAura::AsyncCopyFromCompositingSurface(
@@ -476,17 +467,19 @@ void RenderWidgetHostViewAura::AsyncCopyFromCompositingSurface(
   if (!output->initialize(size.width(), size.height(), true))
     return;
 
-  if (!gl_helper_.get())
-    gl_helper_.reset(CreateGLHelper(compositor));
+  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+  content::GLHelper* gl_helper = factory->GetGLHelper(compositor);
+  if (!gl_helper)
+    return;
 
   scoped_callback_runner.Release();
   unsigned char* addr = static_cast<unsigned char*>(
       output->getTopDevice()->accessBitmap(true).getPixels());
-  gl_helper_->AsyncCopyTextureTo(container->texture_id(),
-                                 container->size(),
-                                 size,
-                                 addr,
-                                 callback);
+  gl_helper->AsyncCopyTextureTo(container->texture_id(),
+                                container->size(),
+                                size,
+                                addr,
+                                callback);
 }
 
 void RenderWidgetHostViewAura::OnAcceleratedCompositingStateChange() {
@@ -1178,13 +1171,6 @@ void RenderWidgetHostViewAura::OnLostResources(ui::Compositor* compositor) {
   shared_surface_handle_ = factory->CreateSharedSurfaceHandle(compositor);
   host_->CompositingSurfaceUpdated();
   host_->ScheduleComposite();
-
-  if (gl_helper_.get()) {
-    // Detach the resources to avoid deleting them using the invalid context.
-    // They've been released anyway when the GPU process died.
-    gl_helper_->Detach();
-    gl_helper_.reset(CreateGLHelper(compositor));
-  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.h b/content/browser/renderer_host/render_widget_host_view_aura.h
index 15857e8..b4bcef1 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.h
+++ b/content/browser/renderer_host/render_widget_host_view_aura.h
@@ -23,7 +23,6 @@
 #include "webkit/glue/webcursor.h"
 
 namespace content {
-class GLHelper;
 class RenderWidgetHostImpl;
 class RenderWidgetHostView;
 }
@@ -225,8 +224,6 @@ class RenderWidgetHostViewAura
   // The model object.
   content::RenderWidgetHostImpl* host_;
 
-  scoped_ptr<content::GLHelper> gl_helper_;
-
   aura::Window* window_;
 
   scoped_ptr<WindowObserver> window_observer_;