Create manager to track OffscreenCanvasSurfaceImpl instances

content/browser/renderer_host/offscreen_canvas_surface_impl.cc

 // Copyright 2016 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/browser/renderer_host/offscreen_canvas_surface_impl.h"
 
 #include "base/bind_helpers.h"
 #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(
     mojo::InterfaceRequest<blink::mojom::OffscreenCanvasSurface> request) {
   mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(),

[Fady Samuel, 2016/11/15 17:32:45]

I'm confused by the lifetime management here. It seems more complicated than
necessary.

What if instead, offscreenCanvasSurfaceManager owned OffscreenCavansSurface.
When OffscreenCanvasSurfaceImpl's binding_ gets a connection error then you tell
the manager and it deletes it from the map.

[xlai (Olivia), 2016/11/15 17:56:52]

I don't understand why this is a problem; this is exactly same as the create
function of every other mojo service in browser/UI. It's added in
RenderProcessHostImpl::RegisterMojoInterfaces() just as everyone else.

Letting OffscreenCanvasSurfaceManager owns OffscreenCanvasSurface may create
more troubles: in the case when user invokes transferControlToOffscreen() and
did not do commit(), we are creating an unnecessary instance of *Manager; also,
if it has a connection error, it won't register to *manager in the first place
so
why bother the deletion?

[Fady Samuel, 2016/11/15 18:06:43]

I guess the part that I find really confusing is the weak pointers in
OffscreenCanvasSurfaceManager. Will there ever be a point in time where
OffscreenCanvasSurfaceManager is holding on to a nullptr for a FrameSinkId? I
think the answer is no, but that's what the use of WeakPtr suggests.

[xlai (Olivia), 2016/11/15 19:12:18]

No I don't think OffscreenCanvasSurfaceManager will ever hold a nullptr because
when the instance is destructed it will always remember to unregister itself
from manager. I must use WeakPtr or raw pointer because obviously
OffscreenCanvasSurfaceManager is not the owner of those 
instances; WeakPtr look a better choice than raw pointer to me.

                           std::move(request));
 }
 
 void OffscreenCanvasSurfaceImpl::GetSurfaceId(
     const GetSurfaceIdCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (frame_sink_id_.is_valid()) {
+    // This IPC should be only called once for each HTMLCanvasElement. In this
+    // case, frame_sink_id_ is still unset.
+    // 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_);
+    mojo::ReportBadMessage(
+        "The same OffscreenCanvasSurfaceImpl is registered to "
+        "OffscreenCanvasSurfaceManager twice.");
+  }
 
-  cc::LocalFrameId local_frame_id = id_allocator_->GenerateId();
-  surface_id_ = cc::SurfaceId(AllocateFrameSinkId(), local_frame_id);
+  frame_sink_id_ = AllocateFrameSinkId();
+  cc::SurfaceId surface_id =
+      cc::SurfaceId(frame_sink_id_, id_allocator_->GenerateId());
 
-  callback.Run(surface_id_);
+  OffscreenCanvasSurfaceManager::GetInstance()
+      ->RegisterOffscreenCanvasSurfaceInstance(frame_sink_id_, this);
+
+  callback.Run(surface_id);
 }
 
 void OffscreenCanvasSurfaceImpl::Require(const cc::SurfaceId& surface_id,
                                          const cc::SurfaceSequence& sequence) {
   cc::SurfaceManager* manager = GetSurfaceManager();
   cc::Surface* surface = manager->GetSurfaceForId(surface_id);
   if (!surface) {
     DLOG(ERROR) << "Attempting to require callback on nonexistent surface";
     return;
   }
   surface->AddDestructionDependency(sequence);
 }
 
 void OffscreenCanvasSurfaceImpl::Satisfy(const cc::SurfaceSequence& sequence) {
   std::vector<uint32_t> sequences;
   sequences.push_back(sequence.sequence);
   cc::SurfaceManager* manager = GetSurfaceManager();
   manager->DidSatisfySequences(sequence.frame_sink_id, &sequences);
 }
 
 }  // namespace content