BrowserPlugin: Cleanup ChildFrameCompositingHelper and fix lifetime issue

content/renderer/child_frame_compositing_helper.cc

 // Copyright 2014 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 "content/renderer/child_frame_compositing_helper.h"
 
 #include "cc/layers/delegated_frame_provider.h"
 #include "cc/layers/delegated_frame_resource_collection.h"
 #include "cc/layers/delegated_renderer_layer.h"
 #include "cc/layers/solid_color_layer.h"
 #include "cc/layers/texture_layer.h"
 #include "cc/output/context_provider.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/output/copy_output_result.h"
 #include "cc/resources/single_release_callback.h"
 #include "content/common/browser_plugin/browser_plugin_messages.h"
 #include "content/common/frame_messages.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
+#include "content/renderer/browser_plugin/browser_plugin.h"
 #include "content/renderer/browser_plugin/browser_plugin_manager.h"
 #include "content/renderer/render_frame_impl.h"
 #include "content/renderer/render_thread_impl.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/WebKit/public/platform/WebGraphicsContext3D.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebPluginContainer.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "ui/gfx/size_conversions.h"
 #include "ui/gfx/skia_util.h"
 #include "webkit/renderer/compositor_bindings/web_layer_impl.h"
 
 namespace content {
 
 ChildFrameCompositingHelper::SwapBuffersInfo::SwapBuffersInfo()
     : route_id(0),
       output_surface_id(0),
       host_id(0),
       software_frame_id(0),
       shared_memory(NULL) {}
 
 ChildFrameCompositingHelper*
 ChildFrameCompositingHelper::CreateCompositingHelperForBrowserPlugin(
-    blink::WebPluginContainer* container,
-    BrowserPluginManager* manager,
-    int instance_id,
-    int host_routing_id) {
+    const base::WeakPtr<BrowserPlugin>& browser_plugin) {
   return new ChildFrameCompositingHelper(
-      container, NULL, manager, NULL, instance_id, host_routing_id);
+      browser_plugin, NULL, NULL, browser_plugin->render_view_routing_id());
 }
 
 ChildFrameCompositingHelper*
 ChildFrameCompositingHelper::CreateCompositingHelperForRenderFrame(
     blink::WebFrame* frame,
     RenderFrameImpl* render_frame,
     int host_routing_id) {
   return new ChildFrameCompositingHelper(
-      NULL, frame, NULL, render_frame, 0, host_routing_id);
+      base::WeakPtr<BrowserPlugin>(), frame, render_frame, host_routing_id);
 }
 
 ChildFrameCompositingHelper::ChildFrameCompositingHelper(
-    blink::WebPluginContainer* container,
+    const base::WeakPtr<BrowserPlugin>& browser_plugin,
     blink::WebFrame* frame,
-    BrowserPluginManager* manager,
     RenderFrameImpl* render_frame,
-    int instance_id,
     int host_routing_id)
-    : instance_id_(instance_id),
-      host_routing_id_(host_routing_id),
+    : host_routing_id_(host_routing_id),
       last_route_id_(0),
       last_output_surface_id_(0),
       last_host_id_(0),
       last_mailbox_valid_(false),
       ack_pending_(true),
       software_ack_pending_(false),
       opaque_(true),
-      container_(container),
       frame_(frame),
-      browser_plugin_manager_(manager),
+      browser_plugin_(browser_plugin),
       render_frame_(render_frame) {}
 
 ChildFrameCompositingHelper::~ChildFrameCompositingHelper() {}
 
+BrowserPluginManager* ChildFrameCompositingHelper::GetBrowserPluginManager() {
+  if (!browser_plugin_)

[kenrb, 2014/01/31 23:44:37]

nit: is this check necessary? If BrowerPlugin owns the compositing helper then
it shouldn't be possible to become NULL.

[Fady Samuel, 2014/02/03 18:04:20]

I just attempted this. There are a couple of callbacks that keep the
CompositingHelper alive:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/c...

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/c...

+    return NULL;
+
+  return browser_plugin_->browser_plugin_manager();
+}
+
+blink::WebPluginContainer* ChildFrameCompositingHelper::GetContainer() {
+  if (!browser_plugin_)
+    return NULL;
+
+  return browser_plugin_->container();
+}
+
+int ChildFrameCompositingHelper::GetInstanceID() {
+  if (!browser_plugin_)
+    return 0;
+
+  return browser_plugin_->guest_instance_id();
+}
+
 void ChildFrameCompositingHelper::SendCompositorFrameSwappedACKToBrowser(
     FrameHostMsg_CompositorFrameSwappedACK_Params& params) {
   // This function will be removed when BrowserPluginManager is removed and
   // BrowserPlugin is modified to use a RenderFrame.
-  if (browser_plugin_manager_) {
-    browser_plugin_manager_->Send(
+  if (GetBrowserPluginManager()) {
+    GetBrowserPluginManager()->Send(
         new BrowserPluginHostMsg_CompositorFrameSwappedACK(
-            host_routing_id_, instance_id_, params));
-  } else {
-    DCHECK(render_frame_);
+            host_routing_id_, GetInstanceID(), params));
+  } else if (render_frame_) {
     render_frame_->Send(
         new FrameHostMsg_CompositorFrameSwappedACK(host_routing_id_, params));
   }
 }
 
 void ChildFrameCompositingHelper::SendBuffersSwappedACKToBrowser(
     FrameHostMsg_BuffersSwappedACK_Params& params) {
   // This function will be removed when BrowserPluginManager is removed and
   // BrowserPlugin is modified to use a RenderFrame.
-  if (browser_plugin_manager_) {
-    browser_plugin_manager_->Send(new BrowserPluginHostMsg_BuffersSwappedACK(
-        host_routing_id_, instance_id_, params));
-  } else {
-    DCHECK(render_frame_);
+  if (GetBrowserPluginManager()) {
+    GetBrowserPluginManager()->Send(new BrowserPluginHostMsg_BuffersSwappedACK(
+        host_routing_id_, GetInstanceID(), params));
+  } else if (render_frame_) {
     render_frame_->Send(
         new FrameHostMsg_BuffersSwappedACK(host_routing_id_, params));
   }
 }
 
 void ChildFrameCompositingHelper::SendReclaimCompositorResourcesToBrowser(
     FrameHostMsg_ReclaimCompositorResources_Params& params) {
   // This function will be removed when BrowserPluginManager is removed and
   // BrowserPlugin is modified to use a RenderFrame.
-  if (browser_plugin_manager_) {
-    browser_plugin_manager_->Send(
+  if (GetBrowserPluginManager()) {
+    GetBrowserPluginManager()->Send(
         new BrowserPluginHostMsg_ReclaimCompositorResources(
-            host_routing_id_, instance_id_, params));
-  } else {
-    DCHECK(render_frame_);
+            host_routing_id_, GetInstanceID(), params));
+  } else if (render_frame_) {
     render_frame_->Send(
         new FrameHostMsg_ReclaimCompositorResources(host_routing_id_, params));
   }
 }
 
 void ChildFrameCompositingHelper::CopyFromCompositingSurface(
     int request_id,
     gfx::Rect source_rect,
     gfx::Size dest_size) {
   CHECK(background_layer_);
@@ skipping 39 matching lines @@
 
 void ChildFrameCompositingHelper::EnableCompositing(bool enable) {
   if (enable && !background_layer_.get()) {
     background_layer_ = cc::SolidColorLayer::Create();
     background_layer_->SetMasksToBounds(true);
     background_layer_->SetBackgroundColor(
         SkColorSetARGBInline(255, 255, 255, 255));
     web_layer_.reset(new webkit::WebLayerImpl(background_layer_));
   }
 
-  if (container_) {
-    container_->setWebLayer(enable ? web_layer_.get() : NULL);
-  } else {
+  if (GetContainer()) {
+    GetContainer()->setWebLayer(enable ? web_layer_.get() : NULL);
+  } else if (frame_) {
     frame_->setRemoteWebLayer(enable ? web_layer_.get() : NULL);
   }
 }
 
 void ChildFrameCompositingHelper::CheckSizeAndAdjustLayerProperties(
     const gfx::Size& new_size,
     float device_scale_factor,
     cc::Layer* layer) {
   if (buffer_size_ != new_size) {
     buffer_size_ = new_size;
@@ skipping 62 matching lines @@
       params.ack.gl_frame_data->sync_point = sync_point;
       SendCompositorFrameSwappedACKToBrowser(params);
       break;
     }
     case SOFTWARE_COMPOSITOR_FRAME:
       break;
   }
 }
 
 void ChildFrameCompositingHelper::OnContainerDestroy() {
-  if (container_)
-    container_->setWebLayer(NULL);
-  container_ = NULL;
+  if (GetContainer())
+    GetContainer()->setWebLayer(NULL);
 
   if (resource_collection_)
     resource_collection_->SetClient(NULL);
 
   ack_pending_ = false;
   software_ack_pending_ = false;
   resource_collection_ = NULL;
   frame_provider_ = NULL;
   texture_layer_ = NULL;
   delegated_layer_ = NULL;
@@ skipping 259 matching lines @@
     bitmap = result->TakeBitmap();
 
   SkBitmap resized_bitmap;
   if (bitmap) {
     resized_bitmap =
         skia::ImageOperations::Resize(*bitmap,
                                       skia::ImageOperations::RESIZE_BEST,
                                       dest_size.width(),
                                       dest_size.height());
   }
-  browser_plugin_manager_->Send(
-      new BrowserPluginHostMsg_CopyFromCompositingSurfaceAck(
-          host_routing_id_, instance_id_, request_id, resized_bitmap));
+  if (GetBrowserPluginManager()) {
+    GetBrowserPluginManager()->Send(
+        new BrowserPluginHostMsg_CopyFromCompositingSurfaceAck(
+            host_routing_id_, GetInstanceID(), request_id, resized_bitmap));
+  }
 }
 
 }  // namespace content