Add PepperViewProxy to protect PepperView and ChromotingInstance on shutdown

Adding a refcounted PepperViewProxy so that we can detach PepperView when
ChromotingInstance is destroyed. PepperViewProxy also performs the task of
thread delegation for PepperView so we can assume PepperView is used only on
pepper thread.

BUG=65696
TEST=None

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

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

6 files changed, 271 insertions, 51 deletions

diff --git a/remoting/client/plugin/chromoting_instance.cc b/remoting/client/plugin/chromoting_instance.cc
index b11dae7..e7af6c0 100644
--- a/remoting/client/plugin/chromoting_instance.cc
+++ b/remoting/client/plugin/chromoting_instance.cc
@@ -20,6 +20,7 @@
 #include "remoting/client/plugin/chromoting_scriptable_object.h"
 #include "remoting/client/plugin/pepper_input_handler.h"
 #include "remoting/client/plugin/pepper_view.h"
+#include "remoting/client/plugin/pepper_view_proxy.h"
 #include "remoting/jingle_glue/jingle_thread.h"
 #include "remoting/protocol/connection_to_host.h"
 #include "remoting/protocol/jingle_connection_to_host.h"
@@ -38,11 +39,11 @@ ChromotingInstance::~ChromotingInstance() {
     client_->Stop();
   }
 
-  // TODO(ajwong): We need to ensure all objects have actually stopped posting
-  // to the message loop before this point.  Right now, we don't have a well
-  // defined stop for the plugin process, and the thread shutdown is likely a
-  // race condition.
+  // Stopping the context shutdown all chromoting threads. This is a requirement
+  // before we can call Detach() on |view_proxy_|.
   context_.Stop();
+
+  view_proxy_->Detach();
 }
 
 bool ChromotingInstance::Init(uint32_t argc,
@@ -70,11 +71,13 @@ bool ChromotingInstance::Init(uint32_t argc,
   host_connection_.reset(new protocol::JingleConnectionToHost(
       context_.jingle_thread()));
   view_.reset(new PepperView(this, &context_));
+  view_proxy_ = new PepperViewProxy(this, view_.get());
   rectangle_decoder_.reset(
-      new RectangleUpdateDecoder(context_.decode_message_loop(), view_.get()));
+      new RectangleUpdateDecoder(context_.decode_message_loop(),
+                                 view_proxy_));
   input_handler_.reset(new PepperInputHandler(&context_,
                                               host_connection_.get(),
-                                              view_.get()));
+                                              view_proxy_));
 
   // Default to a medium grey.
   view_->SetSolidFill(0xFFCDCDCD);
@@ -88,7 +91,7 @@ void ChromotingInstance::Connect(const ClientConfig& config) {
   client_.reset(new ChromotingClient(config,
                                      &context_,
                                      host_connection_.get(),
-                                     view_.get(),
+                                     view_proxy_,
                                      rectangle_decoder_.get(),
                                      input_handler_.get(),
                                      NULL));
diff --git a/remoting/client/plugin/chromoting_instance.h b/remoting/client/plugin/chromoting_instance.h
index ead3b56..94e4100 100644
--- a/remoting/client/plugin/chromoting_instance.h
+++ b/remoting/client/plugin/chromoting_instance.h
@@ -43,6 +43,7 @@ class ClientContext;
 class InputHandler;
 class JingleThread;
 class PepperView;
+class PepperViewProxy;
 class RectangleUpdateDecoder;
 
 struct ClientConfig;
@@ -85,6 +86,15 @@ class ChromotingInstance : public pp::Instance {
   ClientContext context_;
   scoped_ptr<protocol::ConnectionToHost> host_connection_;
   scoped_ptr<PepperView> view_;
+
+  // PepperViewProxy is refcounted and used to interface between shromoting
+  // objects and PepperView and perform thread switching. It wraps around
+  // |view_| and receives method calls on chromoting threads. These method
+  // calls are then delegates on the pepper thread. During destruction of
+  // ChromotingInstance we need to detach PepperViewProxy from PepperView since
+  // both ChromotingInstance and PepperView are destroyed and there will be
+  // outstanding tasks on the pepper message loo.
+  scoped_refptr<PepperViewProxy> view_proxy_;
   scoped_ptr<RectangleUpdateDecoder> rectangle_decoder_;
   scoped_ptr<InputHandler> input_handler_;
   scoped_ptr<ChromotingClient> client_;
diff --git a/remoting/client/plugin/pepper_view.cc b/remoting/client/plugin/pepper_view.cc
index a02ffed..0031439 100644
--- a/remoting/client/plugin/pepper_view.cc
+++ b/remoting/client/plugin/pepper_view.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -24,7 +24,8 @@ PepperView::PepperView(ChromotingInstance* instance, ClientContext* context)
     viewport_width_(0),
     viewport_height_(0),
     is_static_fill_(false),
-    static_fill_color_(0) {
+    static_fill_color_(0),
+    ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
 }
 
 PepperView::~PepperView() {
@@ -35,13 +36,13 @@ bool PepperView::Initialize() {
 }
 
 void PepperView::TearDown() {
+  DCHECK(instance_->CurrentlyOnPluginThread());
+
+  task_factory_.RevokeAll();
 }
 
 void PepperView::Paint() {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(NewTracedMethod(this, &PepperView::Paint));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   TraceContext::tracer()->PrintString("Start Paint.");
   // TODO(ajwong): We're assuming the native format is BGRA_PREMUL below. This
@@ -67,7 +68,7 @@ void PepperView::Paint() {
     // size!  Otherwise, this will just silently do nothing.
     graphics2d_.ReplaceContents(&image);
     graphics2d_.Flush(TaskToCompletionCallback(
-        NewTracedMethod(this, &PepperView::OnPaintDone)));
+        task_factory_.NewRunnableMethod(&PepperView::OnPaintDone)));
   } else {
     // TODO(ajwong): We need to keep a backing store image of the viewport that
     // has the data here which can be redrawn.
@@ -109,38 +110,26 @@ void PepperView::PaintFrame(media::VideoFrame* frame, UpdatedRects* rects) {
   // size!  Otherwise, this will just silently do nothing.
   graphics2d_.ReplaceContents(&image);
   graphics2d_.Flush(TaskToCompletionCallback(
-      NewTracedMethod(this, &PepperView::OnPaintDone)));
+      task_factory_.NewRunnableMethod(&PepperView::OnPaintDone)));
 
   TraceContext::tracer()->PrintString("End Paint Frame.");
 }
 
 void PepperView::SetSolidFill(uint32 color) {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(
-        NewTracedMethod(this, &PepperView::SetSolidFill, color));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   is_static_fill_ = true;
   static_fill_color_ = color;
 }
 
 void PepperView::UnsetSolidFill() {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(
-        NewTracedMethod(this, &PepperView::UnsetSolidFill));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   is_static_fill_ = false;
 }
 
 void PepperView::SetConnectionState(ConnectionState state) {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(
-        NewRunnableMethod(this, &PepperView::SetConnectionState, state));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   ChromotingScriptableObject* scriptable_obj = instance_->GetScriptableObject();
   switch (state) {
@@ -167,11 +156,7 @@ void PepperView::SetConnectionState(ConnectionState state) {
 }
 
 void PepperView::SetViewport(int x, int y, int width, int height) {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(NewTracedMethod(this, &PepperView::SetViewport,
-                                          x, y, width, height));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   // TODO(ajwong): Should we ignore x & y updates?  What do those even mean?
 
@@ -197,6 +182,8 @@ void PepperView::AllocateFrame(media::VideoFrame::Format format,
                                base::TimeDelta duration,
                                scoped_refptr<media::VideoFrame>* frame_out,
                                Task* done) {
+  DCHECK(instance_->CurrentlyOnPluginThread());
+
   // TODO(ajwong): Implement this to be backed by an pp::ImageData rather than
   // generic memory.
   media::VideoFrame::CreateFrame(media::VideoFrame::RGB32,
@@ -211,6 +198,8 @@ void PepperView::AllocateFrame(media::VideoFrame::Format format,
 }
 
 void PepperView::ReleaseFrame(media::VideoFrame* frame) {
+  DCHECK(instance_->CurrentlyOnPluginThread());
+
   if (frame) {
     LOG(WARNING) << "Frame released.";
     frame->Release();
@@ -220,12 +209,7 @@ void PepperView::ReleaseFrame(media::VideoFrame* frame) {
 void PepperView::OnPartialFrameOutput(media::VideoFrame* frame,
                                       UpdatedRects* rects,
                                       Task* done) {
-  if (!instance_->CurrentlyOnPluginThread()) {
-    RunTaskOnPluginThread(
-        NewTracedMethod(this, &PepperView::OnPartialFrameOutput,
-                        make_scoped_refptr(frame), rects, done));
-    return;
-  }
+  DCHECK(instance_->CurrentlyOnPluginThread());
 
   TraceContext::tracer()->PrintString("Calling PaintFrame");
   // TODO(ajwong): Clean up this API to be async so we don't need to use a
@@ -236,6 +220,8 @@ void PepperView::OnPartialFrameOutput(media::VideoFrame* frame,
 }
 
 void PepperView::OnPaintDone() {
+  DCHECK(instance_->CurrentlyOnPluginThread());
+
   // TODO(ajwong):Probably should set some variable to allow repaints to
   // actually paint.
   TraceContext::tracer()->PrintString("Paint flushed");
diff --git a/remoting/client/plugin/pepper_view.h b/remoting/client/plugin/pepper_view.h
index 1213533..f02a26b 100644
--- a/remoting/client/plugin/pepper_view.h
+++ b/remoting/client/plugin/pepper_view.h
@@ -1,15 +1,11 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
 // This class is an implementation of the ChromotingView using Pepper devices
-// as the backing stores.  The public APIs to this class are thread-safe.
-// Calls will dispatch any interaction with the pepper API onto the pepper
-// main thread.
-//
-// TODO(ajwong): We need to better understand the threading semantics of this
-// class.  Currently, we're just going to always run everything on the pepper
-// main thread.  Is this smart?
+// as the backing stores.  This class is used only on pepper thread.
+// Chromoting objects access this object through PepperViewProxy which
+// delegates method calls on the pepper thread.
 
 #ifndef REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_H_
 #define REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_H_
@@ -79,11 +75,11 @@ class PepperView : public ChromotingView,
   bool is_static_fill_;
   uint32 static_fill_color_;
 
+  ScopedRunnableMethodFactory<PepperView> task_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PepperView);
 };
 
 }  // namespace remoting
 
-DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::PepperView);
-
 #endif  // REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_H_
diff --git a/remoting/client/plugin/pepper_view_proxy.cc b/remoting/client/plugin/pepper_view_proxy.cc
new file mode 100644
index 0000000..4ee72d0
--- /dev/null
+++ b/remoting/client/plugin/pepper_view_proxy.cc
@@ -0,0 +1,147 @@
+// Copyright (c) 2011 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 "remoting/client/plugin/pepper_view_proxy.h"
+
+#include "base/message_loop.h"
+#include "remoting/base/tracer.h"
+#include "remoting/client/client_context.h"
+#include "remoting/client/plugin/chromoting_instance.h"
+#include "remoting/client/plugin/pepper_util.h"
+
+namespace remoting {
+
+PepperViewProxy::PepperViewProxy(ChromotingInstance* instance, PepperView* view)
+  : instance_(instance),
+    view_(view) {
+}
+
+PepperViewProxy::~PepperViewProxy() {
+}
+
+bool PepperViewProxy::Initialize() {
+  // This method needs a return value so we can't post a task and process on
+  // another thread so just return true since PepperView doesn't do anything
+  // either.
+  return true;
+}
+
+void PepperViewProxy::TearDown() {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(NewTracedMethod(this, &PepperViewProxy::TearDown));
+    return;
+  }
+
+  if (view_)
+    view_->TearDown();
+}
+
+void PepperViewProxy::Paint() {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(NewTracedMethod(this, &PepperViewProxy::Paint));
+    return;
+  }
+
+  if (view_)
+    view_->Paint();
+}
+
+void PepperViewProxy::SetSolidFill(uint32 color) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewTracedMethod(this, &PepperViewProxy::SetSolidFill, color));
+    return;
+  }
+
+  if (view_)
+    view_->SetSolidFill(color);
+}
+
+void PepperViewProxy::UnsetSolidFill() {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewTracedMethod(this, &PepperViewProxy::UnsetSolidFill));
+    return;
+  }
+
+  if (view_)
+    view_->UnsetSolidFill();
+}
+
+void PepperViewProxy::SetConnectionState(ConnectionState state) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewRunnableMethod(this, &PepperViewProxy::SetConnectionState, state));
+    return;
+  }
+
+  if (view_)
+    view_->SetConnectionState(state);
+}
+
+void PepperViewProxy::SetViewport(int x, int y, int width, int height) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(NewTracedMethod(this, &PepperViewProxy::SetViewport,
+                                          x, y, width, height));
+    return;
+  }
+
+  if (view_)
+    view_->SetViewport(x, y, width, height);
+}
+
+void PepperViewProxy::AllocateFrame(
+    media::VideoFrame::Format format,
+    size_t width,
+    size_t height,
+    base::TimeDelta timestamp,
+    base::TimeDelta duration,
+    scoped_refptr<media::VideoFrame>* frame_out,
+    Task* done) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewTracedMethod(this, &PepperViewProxy::AllocateFrame, format, width,
+                        height, timestamp, duration, frame_out, done));
+    return;
+  }
+
+  if (view_) {
+    view_->AllocateFrame(format, width, height, timestamp, duration, frame_out,
+                         done);
+  }
+}
+
+void PepperViewProxy::ReleaseFrame(media::VideoFrame* frame) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewTracedMethod(this, &PepperViewProxy::ReleaseFrame,
+                        make_scoped_refptr(frame)));
+    return;
+  }
+
+  if (view_)
+    view_->ReleaseFrame(frame);
+}
+
+void PepperViewProxy::OnPartialFrameOutput(media::VideoFrame* frame,
+                                           UpdatedRects* rects,
+                                           Task* done) {
+  if (instance_ && !instance_->CurrentlyOnPluginThread()) {
+    RunTaskOnPluginThread(
+        NewTracedMethod(this, &PepperViewProxy::OnPartialFrameOutput,
+                        make_scoped_refptr(frame), rects, done));
+    return;
+  }
+
+  if (view_)
+    view_->OnPartialFrameOutput(frame, rects, done);
+}
+
+void PepperViewProxy::Detach() {
+  DCHECK(instance_->CurrentlyOnPluginThread());
+  instance_ = NULL;
+  view_ = NULL;
+}
+
+}  // namespace remoting
diff --git a/remoting/client/plugin/pepper_view_proxy.h b/remoting/client/plugin/pepper_view_proxy.h
new file mode 100644
index 0000000..4c294e7
--- /dev/null
+++ b/remoting/client/plugin/pepper_view_proxy.h
@@ -0,0 +1,78 @@
+// Copyright (c) 2011 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.
+
+// PepperViewProxy is used to invoke PepperView object on pepper thread. It
+// has the same interface as PepperView. When a method calls is received on
+// any chromoting threads it delegates the method call to pepper thread.
+// It also provide a detach mechanism so that when PepperView object is
+// destroyed PepperViewProxy will not call it anymore. This is important in
+// providing a safe shutdown of ChromotingInstance and PepperView.
+
+// This object is accessed on chromoting threads and pepper thread. The internal
+// PepperView object is only accessed on pepper thread so as the Detach() method
+// call.
+
+#ifndef REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_PROXY_H_
+#define REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_PROXY_H_
+
+#include "base/ref_counted.h"
+#include "remoting/client/plugin/pepper_view.h"
+
+namespace remoting {
+
+class ChromotingInstance;
+class ClientContext;
+
+class PepperViewProxy : public base::RefCountedThreadSafe<PepperViewProxy>,
+                        public ChromotingView,
+                        public FrameConsumer {
+ public:
+  PepperViewProxy(ChromotingInstance* instance, PepperView* view);
+  virtual ~PepperViewProxy();
+
+  // ChromotingView implementation.
+  virtual bool Initialize();
+  virtual void TearDown();
+  virtual void Paint();
+  virtual void SetSolidFill(uint32 color);
+  virtual void UnsetSolidFill();
+  virtual void SetConnectionState(ConnectionState state);
+  virtual void SetViewport(int x, int y, int width, int height);
+
+  // FrameConsumer implementation.
+  virtual void AllocateFrame(media::VideoFrame::Format format,
+                             size_t width,
+                             size_t height,
+                             base::TimeDelta timestamp,
+                             base::TimeDelta duration,
+                             scoped_refptr<media::VideoFrame>* frame_out,
+                             Task* done);
+  virtual void ReleaseFrame(media::VideoFrame* frame);
+  virtual void OnPartialFrameOutput(media::VideoFrame* frame,
+                                    UpdatedRects* rects,
+                                    Task* done);
+
+  // Remove the reference to |instance_| and |view_| by setting the value to
+  // NULL.
+  // This method should only be called on pepper thread.
+  void Detach();
+
+ private:
+  // This variable is accessed on chromoting threads and pepper thread.
+  // This is initialized when this object is constructed. Its value is reset
+  // to NULL on pepper thread when Detach() is called and there will be no
+  // other threads accessing this variable at the same time. Given the above
+  // conditions locking this variable is not necessary.
+  ChromotingInstance* instance_;
+
+  // This variable is only accessed on the pepper thread. Locking is not
+  // necessary.
+  PepperView* view_;
+
+  DISALLOW_COPY_AND_ASSIGN(PepperViewProxy);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_CLIENT_PLUGIN_PEPPER_VIEW_PROXY_H_