cc: Implement shared worker contexts. (v1)

content/common/gpu/client/context_provider_command_buffer.cc

 // Copyright (c) 2013 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/common/gpu/client/context_provider_command_buffer.h"
 
 #include <set>
 #include <vector>
 
 #include "base/callback_helpers.h"
 #include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
 #include "cc/output/managed_memory_policy.h"
 #include "content/common/gpu/client/grcontext_for_webgraphicscontext3d.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "third_party/skia/include/gpu/GrContext.h"
 
 namespace content {
 
 class ContextProviderCommandBuffer::LostContextCallbackProxy
     : public blink::WebGraphicsContext3D::WebGraphicsContextLostCallback {
  public:
   explicit LostContextCallbackProxy(ContextProviderCommandBuffer* provider)
       : provider_(provider) {
     provider_->context3d_->setContextLostCallback(this);
   }
 
   virtual ~LostContextCallbackProxy() {
     provider_->context3d_->setContextLostCallback(NULL);
   }
 
   virtual void onContextLost() {
     provider_->OnLostContext();
   }
 
  private:
   ContextProviderCommandBuffer* provider_;
 };
 
+void DestroyContext3d(
+    scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context3d) {
+  context3d.reset();
+}
+
 scoped_refptr<ContextProviderCommandBuffer>
 ContextProviderCommandBuffer::Create(
     scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context3d,
     CommandBufferContextType type) {
   if (!context3d)
     return NULL;
 
   return new ContextProviderCommandBuffer(context3d.Pass(), type);
 }
 
@@ skipping 15 matching lines @@
 
   base::AutoLock lock(main_thread_lock_);
 
   // Destroy references to the context3d_ before leaking it.
   if (context3d_->GetCommandBufferProxy()) {
     context3d_->GetCommandBufferProxy()->SetLock(nullptr);
     context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback(
         CommandBufferProxyImpl::MemoryAllocationChangedCallback());
   }
   lost_context_callback_proxy_.reset();
+
+  // Destroy the context3d_ on the same thread it was initialized on.
+  if (context3d_task_runner_.get() && base::ThreadTaskRunnerHandle::IsSet() &&

[danakj, 2015/09/02 18:19:28]

you don't need .get() when checking boolness

[reveman, 2015/09/02 18:53:08]

Done.

+      base::ThreadTaskRunnerHandle::Get() != context3d_task_runner_.get()) {

[danakj, 2015/09/02 18:19:27]

I dislike the pattern of "if not on the thread, post task" and would very much
prefer to just always post task.

But why do we need to do this in the ContextProvider? It feels a bit sketchy?

How about we don't keep a reference on the context provider on the worker
thread, and hold the reference on the compositor thread instead. That should
provide a pretty clear lifetime? And not require changing this class which is
used all over the place?

[reveman, 2015/09/02 18:53:08]

I agree and that's what I initially did but it breaks a huge amount of tests
that use this on one thread and expects synchronous tear down.
We need it because ContextProvider is thread-safe-ref-counted and
ContextProviderCommandBuffer::context3d_ is not but expects to be created and
destroyed on the same thread.
We don't keep a reference on any of the worker threads afaik. The problem is
that we keep a reference on the compositor thread and it ends up being destroyed
on that thread while it was initialized on the main thread. As long as the
ContextProvider is ref-counted, it will be hard to provide any guarantees for
where and when it's destroyed.

I think the alternative is to remove ref-counting from ContentProvider but I
don't know how hard that would be or if it's even possible. I'm assuming it's
ref-counted for a good reason.

[danakj, 2015/09/02 21:15:36]

I guess this all makes me kinda sad then. Maybe we could make this not
refcounted. Or solve all of this some other way. Idk. Not that I have solid
options here without spending the time to try them out, so sorry :( 
https://codereview.chromium.org/1311093002/ sure was a simpler fix that didn't
involve posting deletes. :/ Did it have other downsides?

[reveman, 2015/09/02 21:32:35]

I'm looking at making this not refcounted. In a separate patch but I'll update
this issue with the status. That other patch doesn't solve all issues. Worker
contexts might still be deleted on the wrong thread and that's blocking us from
using shared worker contexts in the renderer.

+    context3d_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&DestroyContext3d, base::Passed(&context3d_)));

[piman, 2015/09/02 18:26:08]

The potential issue is that it may just mask the problem and possibly leak the
context, if the target thread was destroyed.

Agreed that ContextProviderCommandBuffer being refcounted may be misleading - I
wonder if we still need that.

+  }
 }
 
 
 CommandBufferProxyImpl* ContextProviderCommandBuffer::GetCommandBufferProxy() {
   return context3d_->GetCommandBufferProxy();
 }
 
 WebGraphicsContext3DCommandBufferImpl*
 ContextProviderCommandBuffer::WebContext3D() {
   DCHECK(context3d_);
   DCHECK(lost_context_callback_proxy_);  // Is bound to thread.
   DCHECK(context_thread_checker_.CalledOnValidThread());
 
   return context3d_.get();
 }
 
 bool ContextProviderCommandBuffer::BindToCurrentThread() {
   // This is called on the thread the context will be used.
   DCHECK(context_thread_checker_.CalledOnValidThread());
 
   if (lost_context_callback_proxy_)
     return true;
 
   context3d_->SetContextType(context_type_);
   if (!context3d_->InitializeOnCurrentThread())
     return false;
 
+  // The context3d_ should be destroyed on the thread it was initialized.
+  if (base::ThreadTaskRunnerHandle::IsSet())
+    context3d_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+
   InitializeCapabilities();
 
   std::string unique_context_name =
       base::StringPrintf("%s-%p", debug_name_.c_str(), context3d_.get());
   context3d_->traceBeginCHROMIUM("gpu_toplevel", unique_context_name.c_str());
 
   lost_context_callback_proxy_.reset(new LostContextCallbackProxy(this));
   return true;
 }
 
@@ skipping 64 matching lines @@
 }
 
 void ContextProviderCommandBuffer::DeleteCachedResources() {
   DCHECK(context_thread_checker_.CalledOnValidThread());
 
   if (gr_context_)
     gr_context_->FreeGpuResources();
 }
 
 void ContextProviderCommandBuffer::OnLostContext() {
-  DCHECK(context_thread_checker_.CalledOnValidThread());
+  // Note: no thread check here as this should not change the thread for which
+  // this context is currently bound. e.g. a worker context might be unbound
+  // and this should not result in it being bound to the current thread.
   {
     base::AutoLock lock(main_thread_lock_);
     if (destroyed_)
       return;
     destroyed_ = true;
   }
   if (!lost_context_callback_.is_null())
     base::ResetAndReturn(&lost_context_callback_).Run();
   if (gr_context_)
     gr_context_->OnLostContext();
 }
 
 void ContextProviderCommandBuffer::OnMemoryAllocationChanged(
     const gpu::MemoryAllocation& allocation) {
-  DCHECK(context_thread_checker_.CalledOnValidThread());
-
+  // Note: no thread check here as this should not change the thread for which
+  // this context is currently bound. e.g. a worker context might be unbound
+  // and this should not result in it being bound to the current thread.
   if (memory_policy_changed_callback_.is_null())
     return;
 
   memory_policy_changed_callback_.Run(cc::ManagedMemoryPolicy(allocation));
 }
 
 void ContextProviderCommandBuffer::InitializeCapabilities() {
   Capabilities caps;
   caps.gpu = context3d_->GetImplementation()->capabilities();
 
@@ skipping 29 matching lines @@
 
   if (!memory_policy_changed_callback_.is_null()) {
     DCHECK(context3d_->GetCommandBufferProxy());
     context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback(
         base::Bind(&ContextProviderCommandBuffer::OnMemoryAllocationChanged,
                    base::Unretained(this)));
   }
 }
 
 }  // namespace content