Make SurfaceFactory lifetime match frame_sink_id_ registration in BlimpCompositor

blimp/client/core/compositor/blimp_compositor.cc

Index: blimp/client/core/compositor/blimp_compositor.cc
diff --git a/blimp/client/core/compositor/blimp_compositor.cc b/blimp/client/core/compositor/blimp_compositor.cc
index 4b94dbcfab23b67609f7b84d2dd4466f1e8ebbe5..e09d25c5e4382091b6ed6f6ba4985de4c5640f56 100644
--- a/blimp/client/core/compositor/blimp_compositor.cc
+++ b/blimp/client/core/compositor/blimp_compositor.cc
@@ -138,6 +138,7 @@ BlimpCompositor::BlimpCompositor(
       frame_sink_id_(compositor_dependencies_->GetEmbedderDependencies()
                          ->AllocateFrameSinkId()),
       proxy_client_(nullptr),
+      bound_to_proxy_(false),
       compositor_frame_sink_request_pending_(false),
       layer_(cc::Layer::Create()),
       remote_proto_channel_receiver_(nullptr),
@@ -149,6 +150,8 @@ BlimpCompositor::BlimpCompositor(
 void BlimpCompositor::Initialize() {
   surface_id_allocator_ = base::MakeUnique<cc::SurfaceIdAllocator>();
   GetEmbedderDeps()->GetSurfaceManager()->RegisterFrameSinkId(frame_sink_id_);
+  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
+      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
   host_ = CreateLayerTreeHost();
 
   if (use_threaded_layer_tree_host_) {
@@ -184,7 +187,7 @@ void BlimpCompositor::RequestCopyOfOutput(
     std::unique_ptr<cc::CopyOutputRequest> copy_request,
     bool flush_pending_update) {
   // If we don't have a FrameSink, fail right away.
-  if (!surface_factory_)
+  if (!bound_to_proxy_)
     return;
 
   if (!use_threaded_layer_tree_host_) {
@@ -255,7 +258,7 @@ void BlimpCompositor::ApplyViewportDeltas(
 }
 
 void BlimpCompositor::RequestNewCompositorFrameSink() {
-  DCHECK(!surface_factory_);
+  DCHECK(!bound_to_proxy_);
   DCHECK(!compositor_frame_sink_request_pending_);
 
   compositor_frame_sink_request_pending_ = true;
@@ -278,9 +281,10 @@ void BlimpCompositor::DidCommitAndDrawFrame() {
   for (auto it = pending_commit_trackers_.begin();
        it != pending_commit_trackers_.end();) {
     if (--it->first == 0) {
-      if (surface_factory_)
+      if (bound_to_proxy_) {
         surface_factory_->RequestCopyOfSurface(local_frame_id_,
                                                std::move(it->second));
+      }
       it = pending_commit_trackers_.erase(it);
     } else {
       ++it;
@@ -371,8 +375,8 @@ const base::WeakPtr<cc::InputHandler>& BlimpCompositor::GetInputHandler() {
 void BlimpCompositor::OnContextProvidersCreated(
     const scoped_refptr<cc::ContextProvider>& compositor_context_provider,
     const scoped_refptr<cc::ContextProvider>& worker_context_provider) {
-  DCHECK(!surface_factory_) << "Any connection to the old CompositorFrameSink "
-                               "should have been destroyed";
+  DCHECK(!bound_to_proxy_) << "Any connection to the old CompositorFrameSink "
+                              "should have been destroyed";
 
   // Make sure we still have a host and we're still expecting a
   // CompositorFrameSink. This can happen if the host dies while the request is
@@ -400,16 +404,15 @@ void BlimpCompositor::OnContextProvidersCreated(
 void BlimpCompositor::BindToProxyClient(
     base::WeakPtr<BlimpCompositorFrameSinkProxyClient> proxy_client) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!surface_factory_);
+  DCHECK(!bound_to_proxy_);
 
+  bound_to_proxy_ = true;
   proxy_client_ = proxy_client;
-  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
-      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
 }
 
 void BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(bound_to_proxy_);
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   gfx::Size surface_size = root_pass->output_rect.size();
@@ -451,7 +454,7 @@ void BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
 }
 
 void BlimpCompositor::SubmitCompositorFrameAck() {
-  DCHECK(surface_factory_);
+  DCHECK(bound_to_proxy_);

[Khushal, 2016/11/08 00:26:51]

Hmmm, should we still keep this one? Earlier an unbind would have destroyed the
surface factory so there was no way this callback could run. Is that still the
case now after calling Reset on the factory?

[piman, 2016/11/08 00:43:58]

Actually, the draw callback is run even after the factory is destroyed. There's
no change relative to that in this CL... Maybe the DCHECK was already invalid
previously?
It seems harmless to remove. If we're not bound, proxy_client_ will be NULL on
the compositor thread and
BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck will not run.

[Khushal, 2016/11/08 00:51:45]

Oh yes. Just looked through the code, you're right. Could've failed earlier
anyway. Removing it makes sense.

[piman, 2016/11/08 01:40:13]

Done.

   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(&BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck,
@@ -465,16 +468,17 @@ void BlimpCompositor::MakeCopyRequestOnNextSwap(
 
 void BlimpCompositor::UnbindProxyClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(bound_to_proxy_);
 
   DestroyDelegatedContent();
-  surface_factory_.reset();
+  surface_factory_->Reset();
+  bound_to_proxy_ = false;
   proxy_client_ = nullptr;
 }
 
 void BlimpCompositor::ReturnResources(
     const cc::ReturnedResourceArray& resources) {
-  DCHECK(surface_factory_);
+  DCHECK(bound_to_proxy_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(
@@ -560,7 +564,6 @@ void BlimpCompositor::DestroyLayerTreeHost() {
   // Tear down the output surface connection with the old LayerTreeHost
   // instance.
   DestroyDelegatedContent();
-  surface_factory_.reset();
 
   // Destroy the old LayerTreeHost state.
   host_.reset();