Fix RectangleUpdateDecoder to reference the FrameConsumerProxy.

RectangleUpdateDecoder runs on a separate thread from the FrameConsumer in the PPAPI plugin client, requiring FrameConsumer calls to be proxied to the correct thread.  Previously it was passed a bare FrameConsumer pointer, with the underlying FrameConsumerProxy lifetime managed by the ChromotingInstance.  Since the RectangleUpdateDecoder is itself ref-counted it can outlive the ChromotingInstance, and thereby the FrameConsumerProxy, if it is still processing queued messages at the time.

This CL:
* Changes RectangleUpdateDecoder() to take scoped_refptr<>s to the message-loop-proxy and consumer.
* Has RectangleUpdateDecoder() take a FrameConsumerProxy, since FrameConsumer is not ref-counted.  Ideally it should take a scoped_ptr<FrameConsumer>, and leave ref-counting to be an internal detail of FrameConsumerProxy. 

Also:
* FrameConsumerProxy now accepts a WeakPtr<FrameConsumer>, removing the need for an explicit Detach(), which feels safer.  The WeakPtr must have been created on the thread the FrameConsumerProxy will punt calls to, which shouldn't be a problem.
* PepperView now SupportsWeakPtr rather than containing a WeakPtrFactory, to make it easy to get a WeakPtr to it.

BUG=118110


Review URL: http://codereview.chromium.org/9703006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@127035 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 30 insertions, 45 deletions

diff --git a/remoting/client/frame_consumer_proxy.cc b/remoting/client/frame_consumer_proxy.cc
index 47bc9cd..7344b36 100644
--- a/remoting/client/frame_consumer_proxy.cc
+++ b/remoting/client/frame_consumer_proxy.cc
@@ -11,12 +11,12 @@
 namespace remoting {
 
 FrameConsumerProxy::FrameConsumerProxy(
-    base::MessageLoopProxy* frame_consumer_message_loop)
-    : frame_consumer_(NULL),
-      frame_consumer_message_loop_(frame_consumer_message_loop) {
+    scoped_refptr<base::MessageLoopProxy> frame_consumer_message_loop)
+    : frame_consumer_message_loop_(frame_consumer_message_loop) {
 }
 
 FrameConsumerProxy::~FrameConsumerProxy() {
+  DCHECK(frame_consumer_message_loop_->BelongsToCurrentThread());
 }
 
 void FrameConsumerProxy::ApplyBuffer(const SkISize& view_size,
@@ -56,15 +56,11 @@ void FrameConsumerProxy::SetSourceSize(const SkISize& source_size) {
     frame_consumer_->SetSourceSize(source_size);
 }
 
-void FrameConsumerProxy::Attach(FrameConsumer* frame_consumer) {
+void FrameConsumerProxy::Attach(
+    const base::WeakPtr<FrameConsumer>& frame_consumer) {
   DCHECK(frame_consumer_message_loop_->BelongsToCurrentThread());
   DCHECK(frame_consumer_ == NULL);
   frame_consumer_ = frame_consumer;
 }
 
-void FrameConsumerProxy::Detach() {
-  DCHECK(frame_consumer_message_loop_->BelongsToCurrentThread());
-  frame_consumer_ = NULL;
-}
-
 }  // namespace remoting
diff --git a/remoting/client/frame_consumer_proxy.h b/remoting/client/frame_consumer_proxy.h
index 039f042..7e62f2e 100644
--- a/remoting/client/frame_consumer_proxy.h
+++ b/remoting/client/frame_consumer_proxy.h
@@ -11,6 +11,7 @@
 #define REMOTING_CLIENT_FRAME_CONSUMER_PROXY_H_
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "remoting/client/frame_consumer.h"
 
 namespace base {
@@ -25,7 +26,8 @@ class FrameConsumerProxy
  public:
   // Constructs a proxy for |frame_consumer| which will trampoline invocations
   // to |frame_consumer_message_loop|.
-  FrameConsumerProxy(base::MessageLoopProxy* frame_consumer_message_loop);
+  FrameConsumerProxy(
+      scoped_refptr<base::MessageLoopProxy> frame_consumer_message_loop);
   virtual ~FrameConsumerProxy();
 
   // FrameConsumer implementation.
@@ -38,15 +40,10 @@ class FrameConsumerProxy
 
   // Attaches to |frame_consumer_|.
   // This must only be called from |frame_consumer_message_loop_|.
-  void Attach(FrameConsumer* frame_consumer);
-
-  // Detaches from |frame_consumer_|, ensuring no further calls reach it.
-  // This must only be called from |frame_consumer_message_loop_|.
-  void Detach();
+  void Attach(const base::WeakPtr<FrameConsumer>& frame_consumer);
 
  private:
-  FrameConsumer* frame_consumer_;
-
+  base::WeakPtr<FrameConsumer> frame_consumer_;
   scoped_refptr<base::MessageLoopProxy> frame_consumer_message_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameConsumerProxy);
diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index fac4d75..35fee2f 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -13,7 +13,6 @@
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
-#include "base/message_loop.h"
 #include "base/stringprintf.h"
 #include "base/string_split.h"
 #include "base/synchronization/lock.h"
@@ -163,12 +162,6 @@ ChromotingInstance::~ChromotingInstance() {
   // Stopping the context shuts down all chromoting threads.
   context_.Stop();
 
-  // Detach the |consumer_proxy_|, so that any queued tasks don't touch |view_|
-  // after we've deleted it.
-  if (consumer_proxy_.get()) {
-    consumer_proxy_->Detach();
-  }
-
   // Delete |thread_proxy_| before we detach |plugin_message_loop_|,
   // otherwise ScopedThreadProxy may DCHECK when being destroyed.
   thread_proxy_.reset();
@@ -195,13 +188,14 @@ bool ChromotingInstance::Init(uint32_t argc,
   context_.Start();
 
   // Create the chromoting objects that don't depend on the network connection.
-  // Because we decode on a separate thread we need a FrameConsumerProxy to
-  // bounce calls from the RectangleUpdateDecoder back to the plugin thread.
-  consumer_proxy_ = new FrameConsumerProxy(plugin_message_loop_);
+  // RectangleUpdateDecoder runs on a separate thread so for now we wrap
+  // PepperView with a ref-counted proxy object.
+  scoped_refptr<FrameConsumerProxy> consumer_proxy =
+      new FrameConsumerProxy(plugin_message_loop_);
   rectangle_decoder_ = new RectangleUpdateDecoder(
-      context_.decode_message_loop(), consumer_proxy_.get());
+      context_.decode_message_loop(), consumer_proxy);
   view_.reset(new PepperView(this, &context_, rectangle_decoder_.get()));
-  consumer_proxy_->Attach(view_.get());
+  consumer_proxy->Attach(view_->AsWeakPtr());
 
   return true;
 }
diff --git a/remoting/client/plugin/chromoting_instance.h b/remoting/client/plugin/chromoting_instance.h
index 734c574..b4ec662 100644
--- a/remoting/client/plugin/chromoting_instance.h
+++ b/remoting/client/plugin/chromoting_instance.h
@@ -177,7 +177,6 @@ class ChromotingInstance :
   scoped_ptr<protocol::ConnectionToHost> host_connection_;
   scoped_ptr<PepperView> view_;
 
-  scoped_refptr<FrameConsumerProxy> consumer_proxy_;
   scoped_refptr<RectangleUpdateDecoder> rectangle_decoder_;
   scoped_ptr<MouseInputFilter> mouse_input_filter_;
   scoped_ptr<protocol::KeyEventTracker> key_event_tracker_;
diff --git a/remoting/client/plugin/pepper_view.cc b/remoting/client/plugin/pepper_view.cc
index 06eb08e..39e3ab4 100644
--- a/remoting/client/plugin/pepper_view.cc
+++ b/remoting/client/plugin/pepper_view.cc
@@ -75,8 +75,7 @@ PepperView::PepperView(ChromotingInstance* instance,
     clip_area_(SkIRect::MakeEmpty()),
     source_size_(SkISize::Make(0, 0)),
     flush_pending_(false),
-    is_initialized_(false),
-    ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+    is_initialized_(false) {
 }
 
 PepperView::~PepperView() {
@@ -110,8 +109,6 @@ void PepperView::TearDown() {
   while (!buffers_.empty()) {
     FreeBuffer(buffers_.front());
   }
-
-  weak_factory_.InvalidateWeakPtrs();
 }
 
 void PepperView::SetConnectionState(protocol::ConnectionToHost::State state,
@@ -315,8 +312,8 @@ void PepperView::FlushBuffer(const SkIRect& clip_area,
   // Flush the updated areas to the screen.
   scoped_ptr<base::Closure> task(
       new base::Closure(
-          base::Bind(&PepperView::OnFlushDone, weak_factory_.GetWeakPtr(),
-                     start_time, buffer)));
+          base::Bind(&PepperView::OnFlushDone, AsWeakPtr(), start_time,
+                     buffer)));
 
   // Flag needs to be set here in order to get a proper error code for Flush().
   // Otherwise Flush() will always return PP_OK_COMPLETIONPENDING and the error
diff --git a/remoting/client/plugin/pepper_view.h b/remoting/client/plugin/pepper_view.h
index ec1caf9..8db4753 100644
--- a/remoting/client/plugin/pepper_view.h
+++ b/remoting/client/plugin/pepper_view.h
@@ -24,7 +24,9 @@ class ClientContext;
 class FrameProducer;
 
 class PepperView : public ChromotingView,
-                   public FrameConsumer {
+                   public FrameConsumer,
+                   public base::SupportsWeakPtr<PepperView>
+{
  public:
   // Constructs a PepperView for the |instance|. The |instance|, |context|
   // and |producer| must outlive this class.
@@ -114,8 +116,6 @@ class PepperView : public ChromotingView,
   // True after Initialize() has been called, until TearDown().
   bool is_initialized_;
 
-  base::WeakPtrFactory<PepperView> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(PepperView);
 };
 
diff --git a/remoting/client/rectangle_update_decoder.cc b/remoting/client/rectangle_update_decoder.cc
index d829576..8eeebd5 100644
--- a/remoting/client/rectangle_update_decoder.cc
+++ b/remoting/client/rectangle_update_decoder.cc
@@ -25,7 +25,8 @@ using remoting::protocol::SessionConfig;
 namespace remoting {
 
 RectangleUpdateDecoder::RectangleUpdateDecoder(
-    base::MessageLoopProxy* message_loop, FrameConsumer* consumer)
+    scoped_refptr<base::MessageLoopProxy> message_loop,
+    scoped_refptr<FrameConsumerProxy> consumer)
     : message_loop_(message_loop),
       consumer_(consumer),
       source_size_(SkISize::Make(0, 0)),
diff --git a/remoting/client/rectangle_update_decoder.h b/remoting/client/rectangle_update_decoder.h
index daa6dfd..0b0b0c1 100644
--- a/remoting/client/rectangle_update_decoder.h
+++ b/remoting/client/rectangle_update_decoder.h
@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "remoting/base/decoder.h"
+#include "remoting/client/frame_consumer_proxy.h"
 #include "remoting/client/frame_producer.h"
 
 namespace base {
@@ -23,7 +24,6 @@ class ImageData;
 
 namespace remoting {
 
-class FrameConsumer;
 class VideoPacket;
 
 namespace protocol {
@@ -37,8 +37,10 @@ class RectangleUpdateDecoder :
     public base::RefCountedThreadSafe<RectangleUpdateDecoder>,
     public FrameProducer {
  public:
-  RectangleUpdateDecoder(base::MessageLoopProxy* message_loop,
-                         FrameConsumer* consumer);
+  // Creates an update decoder on |message_loop_|, outputting to |consumer|.
+  // TODO(wez): Replace the ref-counted proxy with an owned FrameConsumer.
+  RectangleUpdateDecoder(scoped_refptr<base::MessageLoopProxy> message_loop,
+                         scoped_refptr<FrameConsumerProxy> consumer);
 
   // Initializes decoder with the information from the protocol config.
   void Initialize(const protocol::SessionConfig& config);
@@ -67,8 +69,7 @@ class RectangleUpdateDecoder :
   void DoPaint();
 
   scoped_refptr<base::MessageLoopProxy> message_loop_;
-  FrameConsumer* consumer_;
-
+  scoped_refptr<FrameConsumerProxy> consumer_;
   scoped_ptr<Decoder> decoder_;
 
   // Remote screen size in pixels.