content/ui[Android]: Remove ContextProviderFactory.

content/browser/gpu/browser_gpu_channel_host_factory.cc

 // Copyright (c) 2012 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/gpu/browser_gpu_channel_host_factory.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/profiler/scoped_tracker.h"
@@ skipping 233 matching lines @@
           FROM_HERE,
           base::Bind(
               &BrowserGpuChannelHostFactory::InitializeShaderDiskCacheOnIO,
               gpu_client_id_, cache_dir));
     }
   }
 }
 
 BrowserGpuChannelHostFactory::~BrowserGpuChannelHostFactory() {
   DCHECK(IsMainThread());
+  DCHECK(!in_shutdown_);
+
+  in_shutdown_ = true;
   if (pending_request_.get())
     pending_request_->Cancel();
   for (size_t n = 0; n < established_callbacks_.size(); n++)
     established_callbacks_[n].Run(nullptr);
   shutdown_event_->Signal();
   if (gpu_channel_) {
     gpu_channel_->DestroyChannel();
     gpu_channel_ = NULL;
   }
 }
@@ skipping 11 matching lines @@
 BrowserGpuChannelHostFactory::AllocateSharedMemory(size_t size) {
   std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
   if (!shm->CreateAnonymous(size))
     return std::unique_ptr<base::SharedMemory>();
   return shm;
 }
 
 void BrowserGpuChannelHostFactory::EstablishGpuChannel(
     const gpu::GpuChannelEstablishedCallback& callback) {
   DCHECK(!service_manager::ServiceManagerIsRemote());
+
+  // Don't queue any more requests if the factory is shutting down.
+  if (in_shutdown_)

[boliu, 2017/02/10 22:23:02]

Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in the
callbacks?

[Khushal, 2017/02/10 22:42:39]

Another thought was to set |instance_| to null in Terminate before destroying
it. But then you have to do a null check for the factory at all those call
sites. I don't know if that would be better.

Tbh, while writing this I figured we could simply skip running the callbacks in
the dtor, since that is effectively what is happening now with the return here.

We could also add that enum from ContextProviderFactory specifying the reason
for a null host to the GpuChannelEstablishFactory interface itself.

[boliu, 2017/02/10 22:45:18]

I'm actually worried about accidental infinite loops that's outside shutdown.

[Khushal, 2017/02/10 22:51:30]

Maye be we should always post to the message loop here instead of running the
callback synchronously?

[boliu, 2017/02/10 23:05:22]

All depends on what happens in those callbacks today I guess.

This is cross-platform code, so you probably need to check a whole bunch of
places

[Khushal, 2017/02/10 23:24:30]

I had actually looked around when thinking about putting an enum on the
callback. So there are 3 call-sites for it:

1) GpuProcessTransportFactory, which I think is used for context creation in the
browser on all platforms except android. Quick glance looks like we would make
limited attempts to initialize the Gpu process and then fallback to software.

2) CompositorImpl: Context creation for android and we don't fallback to
software. But I think in practice this re-entrancy has never been an issue at
shutdown because we stop making more requests after becoming invisible. For the
case where the initialization fails, I think we'll keep looping here.

3) VideoCaptureGpuJpegDecoder, which fails silently if a null host is given in
the callback.

So ideally most places should re-try a few times and then fallback to software
if they can, except during termination. If software is not possible, then it
should be fatal. Only android doesn't have a fallback path, so we need to know
whether to re-try or not in the callback. And may be even just crash after a
couple of attempts.

[Khushal, 2017/02/11 02:10:22]

So I think the simplest solution would be to just not run the callbacks in the
dtor, since we do it with a null host and the call-sites can't know if it is a
persistent failure. The cases where we don't fallback to software (android and
MUS?), consecutive failures like this would crash. I don't think there is any
code assuming that for every request the callback will always run anyway.

The calling code should ensure that it does not go into an infinite loop by
tolerating a limited number of consecutive failures and stop making requests
after that. Afaict, all call sites other than android are already doing this, so
we should change that in CompositorImpl. How does this sound?
+jbauman for gpu stuff.

[Khushal, 2017/02/13 21:07:13]

Done.

+    return;
+
   if (gpu_channel_.get() && gpu_channel_->IsLost()) {
     DCHECK(!pending_request_.get());
     // Recreate the channel if it has been lost.
     gpu_channel_->DestroyChannel();
     gpu_channel_ = NULL;
   }
 
   if (!gpu_channel_.get() && !pending_request_.get()) {
     // We should only get here if the context was lost.
     pending_request_ = EstablishRequest::Create(
@@ skipping 81 matching lines @@
 }
 
 // static
 void BrowserGpuChannelHostFactory::InitializeShaderDiskCacheOnIO(
     int gpu_client_id,
     const base::FilePath& cache_dir) {
   GetShaderCacheFactorySingleton()->SetCacheInfo(gpu_client_id, cache_dir);
 }
 
 }  // namespace content