Pepper: always delete ResourceMessageFilter objects on the creation thread.

ppapi/host/resource_message_filter.cc

 // Copyright (c) 2012 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 "ppapi/host/resource_message_filter.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/task_runner.h"
 #include "ipc/ipc_message.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/host/ppapi_host.h"
 #include "ppapi/host/resource_host.h"
 
 namespace ppapi {
 namespace host {
 
-ResourceMessageFilter::ResourceMessageFilter()
-    : reply_thread_message_loop_proxy_(
+namespace internal {
+
+// static
+void ResourceMessageFilterDeleteTraits::Destruct(
+    const ResourceMessageFilter* filter) {
+  filter->InternalDestruct();
+}
+
+}  // namespace internal
+
+ResourceMessageFilter::ResourceMessageFilter(DeletionThread deletion_thread)
+    : deletion_message_loop_proxy_(
+          deletion_thread == DELETE_ON_CREATION_THREAD ?
+              base::MessageLoop::current()->message_loop_proxy() : 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),
       resource_host_(NULL) {
 }
 
+ResourceMessageFilter::ResourceMessageFilter(
+    DeletionThread deletion_thread,
+    scoped_refptr<base::MessageLoopProxy> reply_thread_message_loop_proxy)
+    : deletion_message_loop_proxy_(
+          deletion_thread == DELETE_ON_CREATION_THREAD ?
+              base::MessageLoop::current()->message_loop_proxy() : NULL),
+      reply_thread_message_loop_proxy_(reply_thread_message_loop_proxy),
+      resource_host_(NULL) {
+}
+
 ResourceMessageFilter::~ResourceMessageFilter() {
 }
 
 void ResourceMessageFilter::OnFilterAdded(ResourceHost* resource_host) {
   resource_host_ = resource_host;
 }
 
 void ResourceMessageFilter::OnFilterDestroyed() {
   resource_host_ = NULL;
 }
@@ skipping 32 matching lines @@
 scoped_refptr<base::TaskRunner>
 ResourceMessageFilter::OverrideTaskRunnerForMessage(const IPC::Message& msg) {
   return NULL;
 }
 
 void ResourceMessageFilter::DispatchMessage(const IPC::Message& msg,
                                             HostMessageContext context) {
   RunMessageHandlerAndReply(msg, &context);
 }
 
+void ResourceMessageFilter::InternalDestruct() const {
+  if (deletion_message_loop_proxy_ &&
+      !deletion_message_loop_proxy_->BelongsToCurrentThread()) {
+    // During shutdown the object may not be deleted, but it should be okay to
+    // leak in that case.
+    deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this);

[dmichael (off chromium), 2013/10/30 22:05:42]

Would there be any harm in just always deleting it on its creation thread?

We might even just DCHECK in the constructor that it's the IO thread, and
explicitly use DeleteSoon on the IO thread (instead of storing the
message_loop_proxy).

My *ideal* would be if these acted more like the Hosts, where they aren't
ref-counted at all, and the lifetime is very obvious. But I haven't thought hard
enough to see if that's at all practical...

[yzshen1, 2013/10/30 22:11:55]

It is not harmful for all the existing message filters. Just not necessary for
some of the message filters. They could be deleted on any thread. I think that
saves us some post tasks. But maybe that is not a big gain. :)
If a message filter is in the renderer process, the creation/deletion thread
will be the main thread, not the IO thread. That is different than filters in
the browser processes.
Because we post tasks between threads, I don't know a better way than
ref-counting it. :/
Weakptr doesn't work well for multi-threading scenario, IIRC.

I will also think harder.

Thanks!

[dmichael (off chromium), 2013/10/31 16:14:59]

I see what you mean, but I would lean in favor of keeping the code simpler, so
(a) the individual filters don't need to specify the deletion thread, and (b) we
don't have think hard to make sure a given filter really can be deleted on any
thread. I think it's just easier to understand if we say they're always created
and deleted on the same thread. I don't think the performance difference is
going to really be noticeable. WDYT?
Okay, I forgot about that. Thanks for explaining.
Yeah, I don't think it would be very easy to do this. We accomplish it sometimes
by having a nested sort of "core" class which is RefCountedThreadSafe, while the
outer class has a more clearly defined lifetime. It matters more for classes
that are providing an API in the process. Since these Filters mostly only
respond to IPC, there probably isn't much to be gained by doing all that work.

[yzshen1, 2013/10/31 21:28:04]

Yeah. Sounds good. I have made the change.
I think it is actually similar to the solution you mentioned above: ResourceHost
is acting as the outer, non-ref-counted class, while the ResourceMessageFilter
it contains is the inner, ref-counted class. 

Thanks!

+  } else {
+    delete this;
+  }
+}
+
 }  // namespace host
 }  // namespace ppapi