Create manager to track OffscreenCanvasSurfaceImpl instances

content/browser/renderer_host/offscreen_canvas_surface_impl.cc

Index: content/browser/renderer_host/offscreen_canvas_surface_impl.cc
diff --git a/content/browser/renderer_host/offscreen_canvas_surface_impl.cc b/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
index bac06970c2382f730a49d63858dfaa89953644e6..d78401a2d99e18b16231c101a5cf79a4e6f29e07 100644
--- a/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
+++ b/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
@@ -8,15 +8,21 @@
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_manager.h"
 #include "content/browser/compositor/surface_utils.h"
+#include "content/browser/renderer_host/offscreen_canvas_surface_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 
 namespace content {
 
 OffscreenCanvasSurfaceImpl::OffscreenCanvasSurfaceImpl()
-    : id_allocator_(new cc::SurfaceIdAllocator()) {}
+    : id_allocator_(new cc::SurfaceIdAllocator()), weak_factory_(this) {}
 
-OffscreenCanvasSurfaceImpl::~OffscreenCanvasSurfaceImpl() {}
+OffscreenCanvasSurfaceImpl::~OffscreenCanvasSurfaceImpl() {
+  if (frame_sink_id_.is_valid()) {
+    OffscreenCanvasSurfaceManager::GetInstance()
+        ->UnregisterOffscreenCanvasSurfaceInstance(frame_sink_id_);
+  }
+}
 
 // static
 void OffscreenCanvasSurfaceImpl::Create(
@@ -28,11 +34,27 @@ void OffscreenCanvasSurfaceImpl::Create(
 void OffscreenCanvasSurfaceImpl::GetSurfaceId(
     const GetSurfaceIdCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // This IPC should be only called once for each HTMLCanvasElement. In this
+  // case, frame_sink_id_ is still unset.
+  DCHECK(!frame_sink_id_.is_valid());
+  if (frame_sink_id_.is_valid()) {

[kinuko, 2016/11/15 01:45:48]

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

"you should not handle DCHECK() failures"

We should either only do DCHECK or handle the error case.

[dcheng, 2016/11/15 07:28:29]

Since this comes from the renderer, I think we have to handle this correctly.

[xlai (Olivia), 2016/11/15 17:16:18]

Done. I remove the DCHECK and only handle the error case.

+    // As the browser makes no assumption of correct behavior of renderer, in
+    // an unwanted situation when this function is invoked twice, we need to
+    // unregister the instance from manager.
+    OffscreenCanvasSurfaceManager::GetInstance()
+        ->UnregisterOffscreenCanvasSurfaceInstance(frame_sink_id_);
+    LOG(ERROR) << "The same OffscreenCanvasSurfaceImpl is registered to"
+               << "OffscreenCanvasSurfaceManager twice.";

[kinuko, 2016/11/15 01:45:48]

Should this report a bad message?  Does it mean the renderer is compromised (and
should be killed if possible)?

[dcheng, 2016/11/15 07:28:29]

Yeah, this should call mojo::ReportBadMessage.

[xlai (Olivia), 2016/11/15 17:16:18]

Done.

+  }
+
+  frame_sink_id_ = AllocateFrameSinkId();
+  cc::SurfaceId surface_id =
+      cc::SurfaceId(frame_sink_id_, id_allocator_->GenerateId());
 
-  cc::LocalFrameId local_frame_id = id_allocator_->GenerateId();
-  surface_id_ = cc::SurfaceId(AllocateFrameSinkId(), local_frame_id);
+  OffscreenCanvasSurfaceManager::GetInstance()
+      ->RegisterOffscreenCanvasSurfaceInstance(frame_sink_id_, this);
 
-  callback.Run(surface_id_);
+  callback.Run(surface_id);
 }
 
 void OffscreenCanvasSurfaceImpl::Require(const cc::SurfaceId& surface_id,