cc: Split ThreadProxy into ProxyMain and ProxyImpl

cc/trees/threaded_channel.h

Index: cc/trees/threaded_channel.h
diff --git a/cc/trees/threaded_channel.h b/cc/trees/threaded_channel.h
index 1a4b8b7027ad2acf883fc730def38235f6701a94..edd1444e6112be72b85285b586e76eb73895df4f 100644
--- a/cc/trees/threaded_channel.h
+++ b/cc/trees/threaded_channel.h
@@ -13,7 +13,6 @@
 #include "cc/trees/proxy_common.h"
 #include "cc/trees/proxy_impl.h"
 #include "cc/trees/proxy_main.h"
-#include "cc/trees/thread_proxy.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -22,9 +21,9 @@ class SingleThreadTaskRunner;
 namespace cc {
 class ChannelImpl;
 class ChannelMain;
+class LayerTreeHost;
 class ProxyImpl;
 class ProxyMain;
-class ThreadProxy;
 
 // An implementation of ChannelMain and ChannelImpl that sends commands between
 // ProxyMain and ProxyImpl across thread boundaries.
@@ -69,9 +68,7 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
       // TODO(khushalsagar): Make this ProxyMain* and write the initialization

[Wez, 2015/11/10 01:41:51]

nit: Looks like it _is_ now ProxyMain* ?

[Khushal, 2015/11/11 04:17:47]

Done.

       // sequence. Currently ThreadProxy implements both so we pass the pointer
       // and set ProxyImpl.
-      ThreadProxy* thread_proxy,
-      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner);
+      ProxyMain* proxy_main);
 
   ~ThreadedChannel() override;
 
@@ -99,9 +96,13 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
   void StartCommitOnImpl(CompletionEvent* completion,
                          LayerTreeHost* layer_tree_host,
                          bool hold_commit_for_activation) override;
-  void InitializeImplOnImpl(CompletionEvent* completion,
-                            LayerTreeHost* layer_tree_host) override;
-  void LayerTreeHostClosedOnImpl(CompletionEvent* completion) override;
+

[Wez, 2015/11/10 01:41:51]

nit: Why is there a blank line here? Are these part of the ChannelMain
implementation, ChannelImpl, or something else?

[Khushal, 2015/11/11 04:17:47]

These are all ChannelMain implementation. Fixed.

+  void InitializeImpl(
+      CompletionEvent* completion,
+      LayerTreeHost* layer_tree_host,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source) override;
+  void CloseImpl(CompletionEvent* completion) override;
+  bool IsInitialized() const override;
 
   // ChannelImpl Implementation
   void DidCompleteSwapBuffers() override;
@@ -123,28 +124,31 @@ class CC_EXPORT ThreadedChannel : public ChannelMain, public ChannelImpl {
   void BeginMainFrame(
       scoped_ptr<BeginMainFrameAndCommitState> begin_main_frame_state) override;
 
+  ProxyImpl* proxy_impl() const { return proxy_impl_.get(); }
+
  protected:
-  ThreadedChannel(ThreadProxy* thread_proxy,
-                  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
-                  scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner);
+  explicit ThreadedChannel(ProxyMain* proxy_main);
+
+  // virtual for testing.
+  virtual scoped_ptr<ProxyImpl> CreateProxyImpl(
+      ChannelImpl* channel_impl,
+      LayerTreeHost* layer_tree_host,
+      TaskRunnerProvider* task_runner_provider,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source);
 
  private:
+  void InitializeImplOnImpl(
+      CompletionEvent* completion,
+      LayerTreeHost* layer_tree_host,
+      scoped_ptr<BeginFrameSource> external_begin_frame_source);
+  void CloseImplOnImpl(CompletionEvent* completion);
+
   base::SingleThreadTaskRunner* MainThreadTaskRunner() const;
   base::SingleThreadTaskRunner* ImplThreadTaskRunner() const;
 
   ProxyMain* proxy_main_;
 
-  ProxyImpl* proxy_impl_;
-
-  // TODO(khushalsagar): Temporary variable to access proxy for assertion checks
-  // Remove this once the proxy class is split and the complete
-  // implementation for controlling communication across threads is moved to
-  // ThreadedChannel.
-  Proxy* proxy_;
-
-  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
-
-  scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner_;
+  scoped_ptr<ProxyImpl> proxy_impl_;

[Wez, 2015/11/10 01:41:51]

This doesn't look right; you're creating ThreadChannel on the main thread, I
think, and then ProxyImpl on that thread as well, then posting tasks to call
ProxyImpl on the impl thread, but then letting it get deleted back on the
ThreadChannel thread, i.e. on the main thread?

For this to work correctly you'll need to post a task to the impl thread to
delete the ProxyImpl on it, otherwise you may delete it on the main thread while
a task is processing a task on it on the impl thread - WeakPtr has DCHECKs
against that, so I'm surprised this works.

I'd also recommend storing the WeakPtr to the impl when you create the
ProxyImpl, to make the code a little more robust to any future refactoring that
might affect its lifetime.

[Khushal, 2015/11/11 04:17:47]

ProxyImpl is created and destroyed only on the impl thread. ProxyImpl itself has
DCHECKS to ensure that. We post tasks to the impl thread for both these
operations. Added comments for where that is being done.

Regarding the weak ptr, I'm not sure I completely understood that. Do you mean
we should store only the weak ptr for ProxyImpl here?

The class which implements ChannelImpl has to take ownership for ProxyImpl. In
this case it is the ThreadedChannel. In case of a network channel, the
ChannelImpl implementation which lives on the client will create and control the
lifetime of ProxyImpl.

[Wez, 2015/11/12 22:30:18]

Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().

So, I think a DCHECK(!proxy_impl_) would make sense in ~ThreadedChannel, since
having had CloseOnImplThread() called on the impl thread before the dtor runs on
the main thread is a pre-requisite for correct teardown.
Not quite; my point was mainly about avoiding calling
ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads, since
referring to the factory, which lives on thread A, from thread B inherently
risks accessing it after it's been deleted, which rather defeats the object -
taking and storing a WeakPtr on thread B to use to post tasks is safer.

However, looking through the ProxyImpl & ThreadedChannel:
- ProxyImpl can dispense WeakPtrs, but it doesn't actually use them itself.
- Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to auto-orphan
pending tasks on the ProxyImpl when it gets torn down.
- But the ThreadedChannel is actually the owner of the ProxyImpl.

So you should be able to simplify all this:
1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out into
ThreadedChannel.
2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl> to wrap it,
and use that in your PostTask() binds.
3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it immediately
beforehand.

This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff, and keeps it
all in the class that actually uses it.
SGTM

[Khushal, 2015/11/13 04:02:17]

Yeah, Sorry I missed that. It should be there. It seemed like a bad idea to have
IsInitialized access proxy_impl_ from the main thread. While it only checks if
ProxyImpl exists, and the initialization and destruction of proxy impl are
blocking operations, I figured its better to restrict access to variables to the
thread they belong to. Added a variable on the main thread which makes sure that
the channel is not initialized for correct teardown.
Oh. I actually kept the factory in ProxyImpl since I assumed if ProxyImpl owns
the factory, it lowers the risk of accessing it after its been deleted since the
lifetime of the factory and its pointers is tied to the lifetime of and
controlled by ProxyImpl itself. It explicitly invalidates all the weak pointers
in its dtor.

I think you're right that it would be safer if we avoided calling
weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to make
sure that the caller asks for a weak pointer on the correct thread and we save
it in the channel. Is that better?
I was a bit unsure about moving the weak factory for ProxyImpl to
ThreadedChannel since the pattern for using weakptrfactory is to always have the
class be composed of a factory for itself. I guess this is to make sure that the
factory never outlives the object and the class has control over how it exposes
weak ptrs to itself.

We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure if that
would be better. Wdyt?

[Wez, 2015/11/18 01:50:35]

Not quite - it's absolutely fine to check whether |proxy_impl_| is set, from
code on the main thread, _so long as that is the thread that controls the
lifetime of the ProxyImpl object_, which is the case here - the ThreadedChannel
owns the ProxyImpl and controls its lifetime, so so long as ThreadedChannel
correctly Pass()es or release()s it when it's time to post to the impl thread to
delete it, everything should be fine - the only thing you wouldn't want to do is
actually call into the ProxyImpl from the main thread, since it's not designed
to be called from multiple threads.
SupportsWeakPtr should ideally go away! The problem is that, unlike
WeakPtrFactory, the dtor only runs _after_ the derived class' members have been
torn down - so some part of the teardown causes a WeakPtr to the object to be
dereferenced, it will still dereference even while the object is half gone -
normally not an issue, but also no fun to have to diagnose... The other problem
is simply that it tempts callers to think they can take WeakPtrs, which means
they may create implicit threading dependencies without meaning to.

Having WeakPtrFactory inside the ProxyImpl benefits from implicitly invalidating
WeakPtrs on deletion, but it also means polluting ProxyImpl with GetWeakPtr()
even though it's really a ThreadedChannel implementation detail. There's a
similar risk of callers (ab)using that in confusing ways as with
SupportsWeakPtr.

(This is a case where it'd actually be ideal to have a scoped_ptr<>
specialization that embeds a WeakPtrFactory, to benefit from automatic
invalidation without requiring objects themselves be polluted.)

 
   DISALLOW_COPY_AND_ASSIGN(ThreadedChannel);
 };