Pepper: always delete ResourceMessageFilter objects on the creation thread.

BUG=312504
TEST=None

Review URL: https://codereview.chromium.org/53153002

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

3 files changed, 120 insertions, 60 deletions

diff --git a/ppapi/host/resource_message_filter.cc b/ppapi/host/resource_message_filter.cc
index ff56cac..1757ad5 100644
--- a/ppapi/host/resource_message_filter.cc
+++ b/ppapi/host/resource_message_filter.cc
@@ -16,14 +16,35 @@
 namespace ppapi {
 namespace host {
 
+namespace internal {
+
+// static
+void ResourceMessageFilterDeleteTraits::Destruct(
+    const ResourceMessageFilter* filter) {
+  if (!filter->deletion_message_loop_proxy_->BelongsToCurrentThread()) {
+    // During shutdown the object may not be deleted, but it should be okay to
+    // leak in that case.
+    filter->deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, filter);
+  } else {
+    delete filter;
+  }
+}
+
+}  // namespace internal
+
 ResourceMessageFilter::ResourceMessageFilter()
-    : reply_thread_message_loop_proxy_(
+    : deletion_message_loop_proxy_(
           base::MessageLoop::current()->message_loop_proxy()),
-      resource_host_(NULL) {}
+      reply_thread_message_loop_proxy_(
+          base::MessageLoop::current()->message_loop_proxy()),
+      resource_host_(NULL) {
+}
 
 ResourceMessageFilter::ResourceMessageFilter(
     scoped_refptr<base::MessageLoopProxy> reply_thread_message_loop_proxy)
-    : reply_thread_message_loop_proxy_(reply_thread_message_loop_proxy),
+    : deletion_message_loop_proxy_(
+          base::MessageLoop::current()->message_loop_proxy()),
+      reply_thread_message_loop_proxy_(reply_thread_message_loop_proxy),
       resource_host_(NULL) {
 }
 
diff --git a/ppapi/host/resource_message_filter.h b/ppapi/host/resource_message_filter.h
index ef00cf7..b72a7df 100644
--- a/ppapi/host/resource_message_filter.h
+++ b/ppapi/host/resource_message_filter.h
@@ -24,6 +24,15 @@ namespace ppapi {
 namespace host {
 
 class ResourceHost;
+class ResourceMessageFilter;
+
+namespace internal {
+
+struct PPAPI_HOST_EXPORT ResourceMessageFilterDeleteTraits {
+  static void Destruct(const ResourceMessageFilter* filter);
+};
+
+}  // namespace internal
 
 // This is the base class of resource message filters that can handle resource
 // messages on another thread. ResourceHosts can handle most messages
@@ -65,13 +74,15 @@ class ResourceHost;
 // AddFilter(make_scoped_refptr(new MyMessageFilter));
 class PPAPI_HOST_EXPORT ResourceMessageFilter
     : public ResourceMessageHandler,
-      public base::RefCountedThreadSafe<ResourceMessageFilter> {
+      public base::RefCountedThreadSafe<
+          ResourceMessageFilter, internal::ResourceMessageFilterDeleteTraits> {
  public:
   // This object must be constructed on the same thread that a reply message
   // should be sent, i.e. the IO thread when constructed in the browser process
   // or the main thread when constructed in the renderer process. Since
   // ResourceMessageFilters are usually constructed in the constructor of the
   // owning ResourceHost, this will almost always be the case anyway.
+  // The object will be deleted on the creation thread.
   ResourceMessageFilter();
   // Test constructor. Allows you to specify the message loop which will be used
   // to dispatch replies on.
@@ -93,7 +104,6 @@ class PPAPI_HOST_EXPORT ResourceMessageFilter
       const IPC::Message& msg) OVERRIDE;
 
  protected:
-  friend class base::RefCountedThreadSafe<ResourceMessageFilter>;
   virtual ~ResourceMessageFilter();
 
   // If you want the message to be handled on another thread, return a non-null
@@ -102,10 +112,17 @@ class PPAPI_HOST_EXPORT ResourceMessageFilter
       const IPC::Message& message);
 
  private:
+  friend class base::DeleteHelper<ResourceMessageFilter>;
+  friend class base::RefCountedThreadSafe<
+      ResourceMessageFilter, internal::ResourceMessageFilterDeleteTraits>;
+  friend struct internal::ResourceMessageFilterDeleteTraits;
+
   // This method is posted to the target thread and runs the message handler.
   void DispatchMessage(const IPC::Message& msg,
                        HostMessageContext context);
 
+  scoped_refptr<base::MessageLoopProxy> deletion_message_loop_proxy_;
+
   // Message loop to send resource message replies on. This will be the message
   // loop proxy of the IO thread for the browser process or the main thread for
   // the renderer process.
diff --git a/ppapi/host/resource_message_filter_unittest.cc b/ppapi/host/resource_message_filter_unittest.cc
index 78fc23c..e26d164 100644
--- a/ppapi/host/resource_message_filter_unittest.cc
+++ b/ppapi/host/resource_message_filter_unittest.cc
@@ -2,8 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/run_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "ipc/ipc_message.h"
@@ -15,9 +18,6 @@
 
 namespace ppapi {
 namespace host {
-
-typedef testing::Test ResourceMessageFilterTest;
-
 namespace {
 
 base::WaitableEvent g_handler_completion(true, false);
@@ -141,61 +141,83 @@ class MyResourceFilter : public ResourceMessageFilter {
 
 }  // namespace
 
+class ResourceMessageFilterTest : public testing::Test {
+ public:
+  void TestHandleMessageImpl() {
+    base::Thread io_thread("test_io_thread");
+    ASSERT_TRUE(io_thread.Start());
+
+    base::Thread bg_thread1("test_background_thread1");
+    ASSERT_TRUE(bg_thread1.Start());
+    scoped_refptr<MyResourceFilter> filter1 =
+        new MyResourceFilter(io_thread, bg_thread1, MSG1_TYPE, REPLY_MSG1_TYPE);
+
+    base::Thread bg_thread2("test_background_thread2");
+    ASSERT_TRUE(bg_thread2.Start());
+    scoped_refptr<MyResourceFilter> filter2 =
+        new MyResourceFilter(io_thread, bg_thread2, MSG2_TYPE, REPLY_MSG2_TYPE);
+
+    PP_Instance instance = 12345;
+    PP_Resource resource = 67890;
+    MyResourceHost host(NULL, instance, resource, MSG3_TYPE, REPLY_MSG3_TYPE);
+    host.AddMessageFilter(filter1);
+    host.AddMessageFilter(filter2);
+
+    proxy::ResourceMessageCallParams params(resource, 1);
+    params.set_has_callback();
+    HostMessageContext context(params);
+    IPC::Message message1(0, MSG1_TYPE, IPC::Message::PRIORITY_NORMAL);
+    IPC::Message message2(0, MSG2_TYPE, IPC::Message::PRIORITY_NORMAL);
+    IPC::Message message3(0, MSG3_TYPE, IPC::Message::PRIORITY_NORMAL);
+
+    // Message 1 handled by the first filter.
+    host.HandleMessage(message1, &context);
+    g_handler_completion.Wait();
+    EXPECT_EQ(filter1->last_handled_msg().type(), message1.type());
+    EXPECT_EQ(filter1->last_message_loop(), bg_thread1.message_loop());
+    EXPECT_EQ(host.last_reply_msg().type(),
+              static_cast<uint32>(REPLY_MSG1_TYPE));
+    EXPECT_EQ(host.last_reply_message_loop(), io_thread.message_loop());
+    g_handler_completion.Reset();
+
+    // Message 2 handled by the second filter.
+    host.HandleMessage(message2, &context);
+    g_handler_completion.Wait();
+    EXPECT_EQ(filter2->last_handled_msg().type(), message2.type());
+    EXPECT_EQ(filter2->last_message_loop(), bg_thread2.message_loop());
+    EXPECT_EQ(host.last_reply_msg().type(),
+              static_cast<uint32>(REPLY_MSG2_TYPE));
+    EXPECT_EQ(host.last_reply_message_loop(), io_thread.message_loop());
+    g_handler_completion.Reset();
+
+    // Message 3 handled by the resource host.
+    host.HandleMessage(message3, &context);
+    EXPECT_EQ(host.last_handled_msg().type(), message3.type());
+    EXPECT_EQ(host.last_reply_msg().type(),
+              static_cast<uint32>(REPLY_MSG3_TYPE));
+
+    io_thread.Stop();
+    bg_thread1.Stop();
+    bg_thread2.Stop();
+  }
+};
+
 // Test that messages are filtered correctly and handlers are run on the correct
 // threads.
 TEST_F(ResourceMessageFilterTest, TestHandleMessage) {
-  base::Thread io_thread("test_io_thread");
-  ASSERT_TRUE(io_thread.Start());
-
-  base::Thread bg_thread1("test_background_thread1");
-  ASSERT_TRUE(bg_thread1.Start());
-  scoped_refptr<MyResourceFilter> filter1 =
-      new MyResourceFilter(io_thread, bg_thread1, MSG1_TYPE, REPLY_MSG1_TYPE);
-
-  base::Thread bg_thread2("test_background_thread2");
-  ASSERT_TRUE(bg_thread2.Start());
-  scoped_refptr<MyResourceFilter> filter2 =
-      new MyResourceFilter(io_thread, bg_thread2, MSG2_TYPE, REPLY_MSG2_TYPE);
-
-  PP_Instance instance = 12345;
-  PP_Resource resource = 67890;
-  MyResourceHost host(NULL, instance, resource, MSG3_TYPE, REPLY_MSG3_TYPE);
-  host.AddMessageFilter(filter1);
-  host.AddMessageFilter(filter2);
-
-  proxy::ResourceMessageCallParams params(resource, 1);
-  params.set_has_callback();
-  HostMessageContext context(params);
-  IPC::Message message1(0, MSG1_TYPE, IPC::Message::PRIORITY_NORMAL);
-  IPC::Message message2(0, MSG2_TYPE, IPC::Message::PRIORITY_NORMAL);
-  IPC::Message message3(0, MSG3_TYPE, IPC::Message::PRIORITY_NORMAL);
-
-  // Message 1 handled by the first filter.
-  host.HandleMessage(message1, &context);
-  g_handler_completion.Wait();
-  EXPECT_EQ(filter1->last_handled_msg().type(), message1.type());
-  EXPECT_EQ(filter1->last_message_loop(), bg_thread1.message_loop());
-  EXPECT_EQ(host.last_reply_msg().type(), static_cast<uint32>(REPLY_MSG1_TYPE));
-  EXPECT_EQ(host.last_reply_message_loop(), io_thread.message_loop());
-  g_handler_completion.Reset();
-
-  // Message 2 handled by the second filter.
-  host.HandleMessage(message2, &context);
-  g_handler_completion.Wait();
-  EXPECT_EQ(filter2->last_handled_msg().type(), message2.type());
-  EXPECT_EQ(filter2->last_message_loop(), bg_thread2.message_loop());
-  EXPECT_EQ(host.last_reply_msg().type(), static_cast<uint32>(REPLY_MSG2_TYPE));
-  EXPECT_EQ(host.last_reply_message_loop(), io_thread.message_loop());
-  g_handler_completion.Reset();
-
-  // Message 3 handled by the resource host.
-  host.HandleMessage(message3, &context);
-  EXPECT_EQ(host.last_handled_msg().type(), message3.type());
-  EXPECT_EQ(host.last_reply_msg().type(), static_cast<uint32>(REPLY_MSG3_TYPE));
-
-  io_thread.Stop();
-  bg_thread1.Stop();
-  bg_thread2.Stop();
+  // ResourceMessageFilter instances need to be created on a thread with message
+  // loop. Therefore, we create a message loop and run the testing logic as a
+  // task on it.
+  base::MessageLoop main_message_loop;
+
+  // It should be safe to use base::Unretained() because the object won't be
+  // destroyed before the task is run.
+  main_message_loop.PostTask(
+      FROM_HERE,
+      base::Bind(&ResourceMessageFilterTest::TestHandleMessageImpl,
+                 base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
 }
 
 }  // namespace proxy