content: Fix Context creation logic in ContextProviderFactoryImpl.

content/browser/renderer_host/context_provider_factory_impl_android.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/context_provider_factory_impl_android.h"
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/memory/singleton.h"
@@ skipping 21 matching lines @@
     case ui::ContextProviderFactory::ContextType::
         BLIMP_RENDER_COMPOSITOR_CONTEXT:
       return command_buffer_metrics::BLIMP_RENDER_COMPOSITOR_CONTEXT;
     case ui::ContextProviderFactory::ContextType::BLIMP_RENDER_WORKER_CONTEXT:
       return command_buffer_metrics::BLIMP_RENDER_WORKER_CONTEXT;
   }
   NOTREACHED();
   return command_buffer_metrics::CONTEXT_TYPE_UNKNOWN;
 }
 
+bool IsGpuChannelHostFactoryAvailable() {
+  return BrowserGpuChannelHostFactory::instance() != nullptr;
+}
+
+gpu::GpuChannelHost* IsGpuChannelAvailable() {
+  BrowserGpuChannelHostFactory* factory =
+      BrowserGpuChannelHostFactory::instance();
+  DCHECK(factory);
+
+  if (factory->GetGpuChannel())
+    return factory->GetGpuChannel();

[no sievers, 2016/08/17 18:30:10]

nit: This is maybe a bit verbose vs. just returning factory->GetGpuChannel().
Also maybe call the function GetGpuChannel() (the current name sounds like it
returns a bool)?

[Khushal, 2016/08/17 21:24:55]

Yeah, that's better. Done.

+  else
+    return nullptr;
+}
+
 }  // namespace
 
 // static
 ContextProviderFactoryImpl* ContextProviderFactoryImpl::GetInstance() {
   return base::Singleton<ContextProviderFactoryImpl>::get();
 }
 
 ContextProviderFactoryImpl::ContextProviderFactoryImpl()
     : in_handle_pending_requests_(false),
       surface_client_id_(0),
@@ skipping 86 matching lines @@
   context_request.context_type = context_type;
   context_request.surface_handle = surface_handle;
   context_request.shared_memory_limits = shared_memory_limits;
   context_request.attributes = attributes;
   context_request.support_locking = support_locking;
   context_request.automatic_flushes = automatic_flushes;
   context_request.shared_context_provider = shared_context_provider;
   context_request.result_callback = result_callback;
 
   context_provider_requests_.push_back(context_request);
-  HandlePendingRequests();
+  CheckCanHandlePendingRequests();
 }
 
-void ContextProviderFactoryImpl::HandlePendingRequests() {
+void ContextProviderFactoryImpl::CheckCanHandlePendingRequests() {

[no sievers, 2016/08/17 18:30:10]

can this outer logic be merged into HandlePendingRequests()?

[Khushal, 2016/08/17 21:24:55]

We call HandlePendingRequests from places where we know what the result is, for
instance when the GpuChannelHostFactory gives us a null host in the callback,
and we pass the result back to the caller who made the context request. I added
one to the dtor too so we clear out all the requests when the factory dies.

In CheckCanHandlePendingRequests, if we don't have a gpu channel already, we
would make a request to the factory to establish the channel instead.

+  DCHECK(!context_provider_requests_.empty())
+      << "We don't have any pending requests?";
+  if (!IsGpuChannelHostFactoryAvailable()) {
+    HandlePendingRequests(nullptr,
+                          ContextCreationFailureReason::BROWSER_SHUTDOWN);
+    return;
+  }
+
+  scoped_refptr<gpu::GpuChannelHost> gpu_channel_host(IsGpuChannelAvailable());
+  if (gpu_channel_host) {
+    HandlePendingRequests(gpu_channel_host,
+                          ContextCreationFailureReason::FAILURE_NONE);
+  } else {
+    EstablishGpuChannel();
+  }
+}
+
+void ContextProviderFactoryImpl::HandlePendingRequests(
+    scoped_refptr<gpu::GpuChannelHost> gpu_channel_host,
+    ContextCreationFailureReason reason) {
   DCHECK(!context_provider_requests_.empty())
       << "We don't have any pending requests?";
 
   // Failure to initialize the context could result in new requests. Handle
   // them after going through the current list.
   if (in_handle_pending_requests_)
     return;
 
   {
     base::AutoReset<bool> auto_reset_in_handle_requests(
         &in_handle_pending_requests_, true);
 
-    scoped_refptr<gpu::GpuChannelHost> gpu_channel_host(
-        EnsureGpuChannelEstablished());
-
-    // If we don't have a Gpu Channel Host, we will come back here when the Gpu
-    // channel is established, since OnGpuChannelEstablished triggers handling
-    // of the requests we couldn't process right now.
-    if (!gpu_channel_host)
-      return;
-
     std::list<ContextProvidersRequest> context_requests =
         context_provider_requests_;
     context_provider_requests_.clear();
 
     for (ContextProvidersRequest& context_request : context_requests) {
       scoped_refptr<cc::ContextProvider> context_provider;
 
       const bool create_onscreen_context =
           context_request.surface_handle != gpu::kNullSurfaceHandle;
 
       // Is the request for an onscreen context? Make sure the surface is
-      // still valid in that case. DO NOT run the callback if we don't have a
-      // valid surface.
+      // still valid in that case.
       if (create_onscreen_context &&
           !GpuSurfaceTracker::GetInstance()->IsValidSurfaceHandle(
               context_request.surface_handle)) {
-        continue;
+        // GPU Surface handle loss trumps other context creation failure
+        // reasons.
+        context_request.result_callback.Run(
+            nullptr, ContextCreationFailureReason::GPU_SURFACE_HANDLE_LOST);
+      } else if (!gpu_channel_host) {
+        context_request.result_callback.Run(nullptr, reason);
+      } else {
+        DCHECK_EQ(ContextCreationFailureReason::FAILURE_NONE, reason);
+
+        context_provider = new ContextProviderCommandBuffer(
+            gpu_channel_host, gpu::GPU_STREAM_DEFAULT,
+            gpu::GpuStreamPriority::NORMAL, context_request.surface_handle,
+            GURL(std::string("chrome://gpu/ContextProviderFactoryImpl::") +
+                 std::string("CompositorContextProvider")),
+            context_request.automatic_flushes, context_request.support_locking,
+            context_request.shared_memory_limits, context_request.attributes,
+            static_cast<ContextProviderCommandBuffer*>(
+                context_request.shared_context_provider),
+            context_request.context_type);
+        context_request.result_callback.Run(context_provider, reason);
       }
-
-      context_provider = new ContextProviderCommandBuffer(
-          gpu_channel_host, gpu::GPU_STREAM_DEFAULT,
-          gpu::GpuStreamPriority::NORMAL, context_request.surface_handle,
-          GURL(std::string("chrome://gpu/ContextProviderFactoryImpl::") +
-               std::string("CompositorContextProvider")),
-          context_request.automatic_flushes, context_request.support_locking,
-          context_request.shared_memory_limits, context_request.attributes,
-          static_cast<ContextProviderCommandBuffer*>(
-              context_request.shared_context_provider),
-          context_request.context_type);
-      context_request.result_callback.Run(context_provider);
     }
   }
 
   if (!context_provider_requests_.empty())
-    HandlePendingRequests();
+    CheckCanHandlePendingRequests();
 }
 
-gpu::GpuChannelHost* ContextProviderFactoryImpl::EnsureGpuChannelEstablished() {
+void ContextProviderFactoryImpl::EstablishGpuChannel() {
 #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) || \
     defined(SYZYASAN) || defined(CYGPROFILE_INSTRUMENTATION)
   const int64_t kGpuChannelTimeoutInSeconds = 40;
 #else
   const int64_t kGpuChannelTimeoutInSeconds = 10;
 #endif
 
   BrowserGpuChannelHostFactory* factory =
       BrowserGpuChannelHostFactory::instance();
-
-  if (factory->GetGpuChannel())
-    return factory->GetGpuChannel();
+  DCHECK(factory);
 
   factory->EstablishGpuChannel(
       base::Bind(&ContextProviderFactoryImpl::OnGpuChannelEstablished,
                  weak_factory_.GetWeakPtr()));
   establish_gpu_channel_timeout_.Start(
       FROM_HERE, base::TimeDelta::FromSeconds(kGpuChannelTimeoutInSeconds),
       this, &ContextProviderFactoryImpl::OnGpuChannelTimeout);
-
-  return nullptr;
 }
 
 void ContextProviderFactoryImpl::OnGpuChannelEstablished(
     scoped_refptr<gpu::GpuChannelHost> gpu_channel) {
   establish_gpu_channel_timeout_.Stop();
 
-  // This should happen only during shutdown. So early out instead of queuing
-  // more requests with the factory.
-  if (!gpu_channel)
-    return;
-
   // We can queue the Gpu Channel initialization requests multiple times as
   // we get context requests. So we might have already handled any pending
   // requests when this callback runs.
-  if (!context_provider_requests_.empty())
-    HandlePendingRequests();
+  if (context_provider_requests_.empty())
+    return;
+
+  if (gpu_channel) {
+    HandlePendingRequests(std::move(gpu_channel),
+                          ContextCreationFailureReason::FAILURE_NONE);
+  } else if (IsGpuChannelHostFactoryAvailable()) {
+    HandlePendingRequests(
+        nullptr,
+        ContextCreationFailureReason::GPU_PROCESS_INITIALIZATION_FAILURE);
+  } else {
+    HandlePendingRequests(nullptr,
+                          ContextCreationFailureReason::BROWSER_SHUTDOWN);
+  }
 }
 
 void ContextProviderFactoryImpl::OnGpuChannelTimeout() {
   LOG(FATAL) << "Timed out waiting for GPU channel.";
 }
 
 }  // namespace content