cc: Implement shared worker contexts. (v1)

content/common/gpu/client/context_provider_command_buffer.cc

Index: content/common/gpu/client/context_provider_command_buffer.cc
diff --git a/content/common/gpu/client/context_provider_command_buffer.cc b/content/common/gpu/client/context_provider_command_buffer.cc
index ba9f0ebfbed6a6f75196c29a7bf89da933557773..4bb788835ee9983119820c796b51d0e700708ba9 100644
--- a/content/common/gpu/client/context_provider_command_buffer.cc
+++ b/content/common/gpu/client/context_provider_command_buffer.cc
@@ -9,6 +9,7 @@
 
 #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"
@@ -36,6 +37,11 @@ class ContextProviderCommandBuffer::LostContextCallbackProxy
   ContextProviderCommandBuffer* provider_;
 };
 
+void DestroyContext3d(
+    scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context3d) {
+  context3d.reset();
+}
+
 scoped_refptr<ContextProviderCommandBuffer>
 ContextProviderCommandBuffer::Create(
     scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context3d,
@@ -71,6 +77,13 @@ ContextProviderCommandBuffer::~ContextProviderCommandBuffer() {
         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.

+  }
 }
 
 
@@ -98,6 +111,10 @@ bool ContextProviderCommandBuffer::BindToCurrentThread() {
   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 =
@@ -182,7 +199,9 @@ void ContextProviderCommandBuffer::DeleteCachedResources() {
 }
 
 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_)
@@ -197,8 +216,9 @@ void ContextProviderCommandBuffer::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;