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..0b4c5e12626be8a8800fa36b32b4de7e7247cf7c 100644
--- a/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
+++ b/content/browser/renderer_host/offscreen_canvas_surface_impl.cc
@@ -8,13 +8,14 @@
 #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() {}
 
@@ -26,11 +27,20 @@ void OffscreenCanvasSurfaceImpl::Create(
 }
 
 void OffscreenCanvasSurfaceImpl::GetSurfaceId(
+    uint32_t canvas_id,
     const GetSurfaceIdCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   cc::LocalFrameId local_frame_id = id_allocator_->GenerateId();
+
+  // This IPC should be only called once for each HTMLCanvasElement. In this
+  // case, surface_id_ is still unset.
+  DCHECK(surface_id_.is_null());

[dcheng, 2016/11/08 18:30:23]

This can't DCHECK, since the renderer could do this an arbitrary number of
times.

Will we violate an invariant if we allow this to be called multiple times?

[xlai (Olivia), 2016/11/10 15:45:11]

This IPC call is supposed to be called only once for the initial set up of
Surface Layer, for each HTMLCanvasElement that has been transferred to
OffscreenCanvas. It is called twice, it means the caller does it wrongly. It's
more like an implementation check to prevent future developers from making a
mistake.

[dcheng, 2016/11/11 08:36:40]

Right, but this is browser code being invoked by the renderer via a remote Mojo
IPC call. The browser can make no assumptions that the renderer is behaving
correctly. Given that, is it safe for this to be a DCHECK, or will this break
important invariants?

   surface_id_ = cc::SurfaceId(AllocateFrameSinkId(), local_frame_id);
+  canvas_id_ = canvas_id;
+
+  OffscreenCanvasSurfaceManager::GetInstance()
+      ->RegisterOffscreenCanvasSurfaceInstance(canvas_id_, this);
 
   callback.Run(surface_id_);
 }