Support using TrackedCallbacks as hints to determine the handling thread of resource reply messages

Support using TrackedCallbacks as hints to determine the handling thread of resource reply messages.

BUG=269737
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240282

[yzshen1, 2013-12-10 01:18:09]

Hi, David and Antoine.

Would you please take a look?

David: everything
Antoine: OWNER review for content/ppapi_plugin/*

Thanks!

[piman, 2013-12-10 01:45:46]

LGTM for the content parts.

[dmichael (off chromium), 2013-12-10 23:38:00]

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.h:168: scoped_refptr<ResourceReplyThreadRegistrar>
resource_reply_thread_registrar_;
Is there any reason to not just hold it by value?

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_resou...
File ppapi/proxy/plugin_resource.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_resou...
ppapi/proxy/plugin_resource.h:109: // on the target thread of the callback;
otherwises, it will be handled on the
s/otherwises/otherwise

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
File ppapi/proxy/resource_reply_thread_registrar.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:27: //
ResourceReplyThreadRegistrar records handling thread for
nit: records +the+ handling

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:41:
scoped_refptr<base::MessageLoopProxy> GetTargetThreadAndUnregister(
As I understand it, Unregister and this method should be called *without* the
ProxyLock, while Register has to have it. Not clear about the constructor and
destructor; presumably they don't care (and it's probably not locked when they
run).

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:54: base::Lock lock_;
I find this a little bit scary, like we might mess up and either deadlock or
have a race condition. (Though I think your current code is correct). It might
be nice if we only used the ProxyLock, though I'm guessing you are worried about
too much contention.

Have you benchmarked to see how much difference there is? In terms of raw work,
there's not much difference. If you rely only on the proxy lock, then Register
will only have to lock one mutex instead of two. Unregister would lock the proxy
lock instead of this one. But the PRoxyLock will obviously be more contended...


At a minimum, this should be documented better, w.r.t. the locking expectations
of each method and noting which lock(s) protect the members.

[yzshen1, 2013-12-11 00:21:49]

Thanks David!

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.h:168: scoped_refptr<ResourceReplyThreadRegistrar>
resource_reply_thread_registrar_;
On 2013/12/10 23:38:01, dmichael wrote:
> Is there any reason to not just hold it by value?

Mostly because the resources and the message filter on the IO thread will hold
pointers to this object.

We could make it owned by this class if we make sure this object outlive:
- all resources
- the channel on the IO thread

I could double check and make it non-ref-counted. I was just a little bit
concerned about adding an implicit constraint that this object must outlive the
message filter on the IO thread. But maybe it is okay because we are already
passing non-ref-counted pointer of the instance set to the message filter.

What do you think?

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_resou...
File ppapi/proxy/plugin_resource.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_resou...
ppapi/proxy/plugin_resource.h:109: // on the target thread of the callback;
otherwises, it will be handled on the
On 2013/12/10 23:38:01, dmichael wrote:
> s/otherwises/otherwise

Done.

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
File ppapi/proxy/resource_reply_thread_registrar.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:27: //
ResourceReplyThreadRegistrar records handling thread for
On 2013/12/10 23:38:01, dmichael wrote:
> nit: records +the+ handling

Done.

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:41:
scoped_refptr<base::MessageLoopProxy> GetTargetThreadAndUnregister(
On 2013/12/10 23:38:01, dmichael wrote:
> As I understand it, Unregister and this method should be called *without* the
> ProxyLock, while Register has to have it. Not clear about the constructor and
> destructor; presumably they don't care (and it's probably not locked when they
> run).

All methods except Register() don't care about whether ProxyLock is held or not,
I think.

I added a comment to Register that it must be called while the proxy lock is
held. (And I also have a debug check in the impl.)

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.h:54: base::Lock lock_;
On 2013/12/10 23:38:01, dmichael wrote:
> I find this a little bit scary, like we might mess up and either deadlock or
> have a race condition. (Though I think your current code is correct). It might
> be nice if we only used the ProxyLock, though I'm guessing you are worried
about
> too much contention.
Yeah, GetTargetThreadAndUnregister() will be called on the IO thread for every
incoming reply, it seems better if we can use a separate lock.

I think it should be safe, because this lock only protects the internal state of
the class, and while holding the lock, we never try to acquire the pepper proxy
lock.

> 
> Have you benchmarked to see how much difference there is? In terms of raw
work,
> there's not much difference. If you rely only on the proxy lock, then Register
> will only have to lock one mutex instead of two. Unregister would lock the
proxy
> lock instead of this one. But the PRoxyLock will obviously be more
contended...
I am writing an test, and will test and see how much is the difference.
 
> At a minimum, this should be documented better, w.r.t. the locking
expectations
> of each method and noting which lock(s) protect the members.
I added a comment for Register() and a comment for |lock_|. Please take a look.

[dmichael (off chromium), 2013-12-11 21:30:36]

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
File ppapi/proxy/plugin_message_filter.h (right):

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
ppapi/proxy/plugin_message_filter.h:31: // The input is a pointer to a set that
will be used to uniquify PP_Instances
nit: this documentation is a little out of date, with the new parameter.

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
ppapi/proxy/plugin_message_filter.h:63: // pointer, it will be managed by the
later that creates this class.
Not part of your CL, but...
"later"->"delegate"?

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/resource_rep...
File ppapi/proxy/resource_reply_thread_registrar.cc (right):

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.cc:27: // TODO(yzshen): add
comments.
Would be good to add the comments before checking in ^^^

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.h:168: scoped_refptr<ResourceReplyThreadRegistrar>
resource_reply_thread_registrar_;
On 2013/12/11 00:21:49, yzshen1 wrote:
> On 2013/12/10 23:38:01, dmichael wrote:
> > Is there any reason to not just hold it by value?
> 
> Mostly because the resources and the message filter on the IO thread will hold
> pointers to this object.
> 
> We could make it owned by this class if we make sure this object outlive:
> - all resources
> - the channel on the IO thread
> 
> I could double check and make it non-ref-counted. I was just a little bit
> concerned about adding an implicit constraint that this object must outlive
the
> message filter on the IO thread. But maybe it is okay because we are already
> passing non-ref-counted pointer of the instance set to the message filter.
> 
> What do you think?
> 
Hmm, yeah... it's a little unclear to me what the lifetimes are.

PluginDelegate already assumes that PluginGlobals outlives it
(PluginDelegate::delegate_ is the PpapiThread, which owns PluginGlobals).
Similarly, PluginDelegate's lifetime is scoped to its channel. It appears to be
deleted only in OnChannelError:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
...
And during shutdown, it will clear the channel...  which will (inside of
IPC::ChannelProxy) post a task to the IO thread to delete the MessageFilter...

So I guess at shutdown it might actually be possible that the
PluginMessageFilter does outlive PluginGlobals.

Okay, I guess we have to do it that way.

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:111: new ppapi::proxy::PluginMessageFilter(
Why do we need this filter *and* the one the dispatcher makes? I think I'm not
understanding something.

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:112: NULL,
globals->resource_reply_thread_registrar()));
Why are you passing a NULL GloballyInstanceIDSet now?

https://codereview.chromium.org/46433002/diff/280001/ppapi/proxy/plugin_main_...
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/46433002/diff/280001/ppapi/proxy/plugin_main_...
ppapi/proxy/plugin_main_nacl.cc:116: NULL,
PluginGlobals::Get()->resource_reply_thread_registrar()));
Why do we need this *and* the one that PluginDispatcher registers?

[yzshen1, 2013-12-11 22:22:07]

Thanks David!

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
File ppapi/proxy/plugin_message_filter.h (right):

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
ppapi/proxy/plugin_message_filter.h:31: // The input is a pointer to a set that
will be used to uniquify PP_Instances
I think you might be reading an old patchset.
Please take a look at the latest one. :)

On 2013/12/11 21:30:36, dmichael wrote:
> nit: this documentation is a little out of date, with the new parameter.

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/plugin_messa...
ppapi/proxy/plugin_message_filter.h:63: // pointer, it will be managed by the
later that creates this class.
Done.

I changed to:
"It is managed by PluginDispatcher::PluginDelegate."

On 2013/12/11 21:30:36, dmichael wrote:
> Not part of your CL, but...
> "later"->"delegate"?

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/resource_rep...
File ppapi/proxy/resource_reply_thread_registrar.cc (right):

https://codereview.chromium.org/46433002/diff/240001/ppapi/proxy/resource_rep...
ppapi/proxy/resource_reply_thread_registrar.cc:27: // TODO(yzshen): add
comments.
This is an old patchset. Please see the latest one.
On 2013/12/11 21:30:36, dmichael wrote:
> Would be good to add the comments before checking in ^^^

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.h (right):

https://codereview.chromium.org/46433002/diff/260001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.h:168: scoped_refptr<ResourceReplyThreadRegistrar>
resource_reply_thread_registrar_;
On 2013/12/11 21:30:36, dmichael wrote:
> On 2013/12/11 00:21:49, yzshen1 wrote:
> > On 2013/12/10 23:38:01, dmichael wrote:
> > > Is there any reason to not just hold it by value?
> > 
> > Mostly because the resources and the message filter on the IO thread will
hold
> > pointers to this object.
> > 
> > We could make it owned by this class if we make sure this object outlive:
> > - all resources
> > - the channel on the IO thread
> > 
> > I could double check and make it non-ref-counted. I was just a little bit
> > concerned about adding an implicit constraint that this object must outlive
> the
> > message filter on the IO thread. But maybe it is okay because we are already
> > passing non-ref-counted pointer of the instance set to the message filter.
> > 
> > What do you think?
> > 
> Hmm, yeah... it's a little unclear to me what the lifetimes are.
> 
> PluginDelegate already assumes that PluginGlobals outlives it
> (PluginDelegate::delegate_ is the PpapiThread, which owns PluginGlobals).
> Similarly, PluginDelegate's lifetime is scoped to its channel. It appears to
be
> deleted only in OnChannelError:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
> ...
> And during shutdown, it will clear the channel...  which will (inside of
> IPC::ChannelProxy) post a task to the IO thread to delete the MessageFilter...
> 
> So I guess at shutdown it might actually be possible that the
> PluginMessageFilter does outlive PluginGlobals.
> 
> Okay, I guess we have to do it that way.

Yeah, in ChildProcess, the main_thread_ member (in our case ppapi_thread) is
destroyed before io_thread_. Therefore it is possible that PluginGlobals is gone
while the IO thread is still processing messages.

(That makes me think that we should probably also change
globally_seen_instance_ids_ to be ref-counted.)

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:111: new ppapi::proxy::PluginMessageFilter(
On 2013/12/11 21:30:36, dmichael wrote:
> Why do we need this filter *and* the one the dispatcher makes? I think I'm not
> understanding something.
This is the channel between the browser and the plugin. (plugin_main_nacl.cc
also has such a channel.)
Dispatchers manage channels between renderers and the plugin.

https://codereview.chromium.org/46433002/diff/280001/content/ppapi_plugin/ppa...
content/ppapi_plugin/ppapi_thread.cc:112: NULL,
globals->resource_reply_thread_registrar()));
On 2013/12/11 21:30:36, dmichael wrote:
> Why are you passing a NULL GloballyInstanceIDSet now?

Because between the browser and the plugin we don't have to do this
make-pp-instance-unique work. The browser never creates plugin instances.

(In plugin_message_filter, I have got a comment for the constructor about this.)

https://codereview.chromium.org/46433002/diff/280001/ppapi/proxy/plugin_main_...
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/46433002/diff/280001/ppapi/proxy/plugin_main_...
ppapi/proxy/plugin_main_nacl.cc:116: NULL,
PluginGlobals::Get()->resource_reply_thread_registrar()));
On 2013/12/11 21:30:36, dmichael wrote:
> Why do we need this *and* the one that PluginDispatcher registers?

Please see my comment in ppapi_thread.cc.

[dmichael (off chromium), 2013-12-11 22:25:31]

Oops, thanks for reminding me about the browser channel.

lgtm

[commit-bot: I haz the power, 2013-12-11 22:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/46433002/300001

[commit-bot: I haz the power, 2013-12-11 22:30:52]

Failed to apply patch for content/ppapi_plugin/ppapi_thread.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/ppapi_plugin/ppapi_thread.cc
  Hunk #1 FAILED at 36.
  Hunk #2 FAILED at 103.
  Hunk #3 succeeded at 104 (offset -31 lines).
  Hunk #4 succeeded at 354 (offset -31 lines).
  2 out of 4 hunks FAILED -- saving rejects to file
content/ppapi_plugin/ppapi_thread.cc.rej

Patch:       content/ppapi_plugin/ppapi_thread.cc
Index: content/ppapi_plugin/ppapi_thread.cc
diff --git a/content/ppapi_plugin/ppapi_thread.cc
b/content/ppapi_plugin/ppapi_thread.cc
index
a29a63903b4e7e36cf57bd3f1ad87d7d1edb2d61..1040e252e956ff29f04c534cdaf8cad3d8d21852
100644
--- a/content/ppapi_plugin/ppapi_thread.cc
+++ b/content/ppapi_plugin/ppapi_thread.cc
@@ -36,7 +36,9 @@
 #include "ppapi/c/ppp.h"
 #include "ppapi/proxy/interface_list.h"
 #include "ppapi/proxy/plugin_globals.h"
+#include "ppapi/proxy/plugin_message_filter.h"
 #include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/proxy/resource_reply_thread_registrar.h"
 #include "ppapi/shared_impl/api_id.h"
 #include "third_party/WebKit/public/web/WebKit.h"
 #include "ui/base/ui_base_switches.h"
@@ -103,6 +105,12 @@ PpapiThread::PpapiThread(const CommandLine& command_line,
bool is_broker)
   // note that only those InterfaceProxy-based ones require registration.
   AddRoute(ppapi::API_ID_PPB_HOSTRESOLVER_PRIVATE,
            &dispatcher_message_listener_);
+
+  if (!is_broker_) {
+    channel()->AddFilter(
+        new ppapi::proxy::PluginMessageFilter(
+            NULL, globals->resource_reply_thread_registrar()));
+  }
 }
 
 PpapiThread::~PpapiThread() {
@@ -135,7 +143,6 @@ bool PpapiThread::OnControlMessageReceived(const
IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(PpapiMsg_SetNetworkState, OnSetNetworkState)
     IPC_MESSAGE_HANDLER(PpapiMsg_Crash, OnCrash)
     IPC_MESSAGE_HANDLER(PpapiMsg_Hang, OnHang)
-    IPC_MESSAGE_HANDLER(PpapiPluginMsg_ResourceReply, OnResourceReply)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -386,13 +393,6 @@ void PpapiThread::OnCreateChannel(base::ProcessId
renderer_pid,
   Send(new PpapiHostMsg_ChannelCreated(channel_handle));
 }
 
-void PpapiThread::OnResourceReply(
-    const ppapi::proxy::ResourceMessageReplyParams& reply_params,
-    const IPC::Message& nested_msg) {
-  ppapi::proxy::PluginDispatcher::DispatchResourceReply(reply_params,
-                                                        nested_msg);
-}
-
 void PpapiThread::OnSetNetworkState(bool online) {
   // Note the browser-process side shouldn't send us these messages in the
   // first unless the plugin has dev permissions, so we don't need to check

[commit-bot: I haz the power, 2013-12-12 00:19:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/46433002/320001

[commit-bot: I haz the power, 2013-12-12 12:25:36]

Change committed as 240282