aw: Avoid uncontrolled video context destruction

content/browser/android/in_process/synchronous_compositor_factory_impl.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/browser/android/in_process/synchronous_compositor_factory_impl.h"
 
 #include "content/browser/android/in_process/synchronous_compositor_output_surface.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/renderer/gpu/frame_swap_message_queue.h"
 #include "gpu/command_buffer/client/gl_in_process_context.h"
@@ skipping 214 matching lines @@
 
 void SynchronousCompositorFactoryImpl::CompositorInitializedHardwareDraw() {
   base::AutoLock lock(num_hardware_compositor_lock_);
   num_hardware_compositors_++;
 }
 
 void SynchronousCompositorFactoryImpl::CompositorReleasedHardwareDraw() {
   base::AutoLock lock(num_hardware_compositor_lock_);
   DCHECK_GT(num_hardware_compositors_, 0u);
   num_hardware_compositors_--;
-  if (num_hardware_compositors_ == 0) {
-    // Nullify the video_context_provider_ now so that it is not null only if
-    // there is at least 1 hardware compositor
-    video_context_provider_ = NULL;
-  }
 }
 
 bool SynchronousCompositorFactoryImpl::CanCreateMainThreadContext() {
   base::AutoLock lock(num_hardware_compositor_lock_);
   return num_hardware_compositors_ > 0;
 }
 
 scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>
 SynchronousCompositorFactoryImpl::TryCreateStreamTextureFactory() {
-  scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>
-      context_provider;
-  // This check only guarantees the main thread context is created after
-  // a compositor did successfully initialize hardware draw in the past.
-  // When all compositors have released hardware draw, main thread context
-  // creation is guaranteed to fail.
-  if (CanCreateMainThreadContext() && !video_context_provider_) {
+  // Always fail creation even if |video_context_provider_| is not NULL.
+  // This is to avoid synchronous calls that may deadlock. Setting
+  // |video_context_provider_| to null is also not safe since it makes
+  // synchronous destruction uncontrolled and possibly deadlock.
+  if (!CanCreateMainThreadContext()) {
+    return
+        scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>();

[hush (inactive), 2014/08/22 17:49:38]

can we just return null here?

[boliu, 2014/08/22 17:54:17]

Hmm, compiler seems happy, but not sure if it fits chromium style

[boliu, 2014/08/22 18:14:30]

Took a cursory look and didn't find any instance of "return NULL" used on a
ref-counted type.

[hush (inactive), 2014/08/22 18:26:43]

alright.
lgtm
sorry that I broke this thing...
On 2014/08/22 18:14:30, boliu wrote:

+  }
+
+  if (!video_context_provider_) {
     DCHECK(service_);
     DCHECK(share_context_.get());
 
     video_context_provider_ = new VideoContextProvider(
         CreateContext(service_,
                       share_context_.get(),
                       gpu::GLInProcessContextSharedMemoryLimits()));
   }
   return video_context_provider_;
 }
 
 void SynchronousCompositorFactoryImpl::SetDeferredGpuService(
     scoped_refptr<gpu::InProcessCommandBuffer::Service> service) {
   DCHECK(!service_);
   service_ = service;
 }
 
 void SynchronousCompositorFactoryImpl::SetRecordFullDocument(
     bool record_full_document) {
   record_full_layer_ = record_full_document;
 }
 
 }  // namespace content