Let IPC::SyncMessageFilter take advantage of thread-safe Send.

Let IPC::SyncMessageFilter take advantage of thread-safe Send.

SyncMessageFilter is unnecessarily hopping to the IO thread
before writing to its underlying Sender when the underlying
Sender has a thread-safe Send implementation.

This fixes that.

BUG=516464
Committed: https://crrev.com/ac64ae92ff624f1befe3b973e0f0fd0204f2eac1
Cr-Commit-Position: refs/heads/master@{#342027}

[Ken Rockot(use gerrit already), 2015-08-03 20:47:02]

rockot@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2015-08-03 22:33:49]

nice catch

It would be nice to avoid putting this on IPC::Sender, since it's a trivial
interface right now and it doesn't know about threading. If it was to know about
that, then one would have to worry about how to indicate that the object is
deleted on different threads etc...

Given that we only want SyncMessageFilter in processes that have SyncChannel
(since for example we never want to instantiate it in the browser process since
we don't want any thread there blocked on a child process), how about having a
method on SyncChannel that creates/gets a SyncMessageFilter? Then we wouldn't
have to send the shutdown_event to that method, since SyncChannel already gets
it in its constructor. SyncChannel, when it internally creates a
SyncMessageFilter, can then tell it in its constructor whether the channel
supports thread-safe send.

wdyt?

[Ken Rockot(use gerrit already), 2015-08-03 22:36:22]

On 2015/08/03 22:33:49, jam wrote:
> nice catch
> 
> It would be nice to avoid putting this on IPC::Sender, since it's a trivial
> interface right now and it doesn't know about threading. If it was to know
about
> that, then one would have to worry about how to indicate that the object is
> deleted on different threads etc...
> 

yeah i toyed with a few ways to avoid that, but...

> Given that we only want SyncMessageFilter in processes that have SyncChannel
> (since for example we never want to instantiate it in the browser process
since
> we don't want any thread there blocked on a child process), how about having a
> method on SyncChannel that creates/gets a SyncMessageFilter? Then we wouldn't
> have to send the shutdown_event to that method, since SyncChannel already gets
> it in its constructor. SyncChannel, when it internally creates a
> SyncMessageFilter, can then tell it in its constructor whether the channel
> supports thread-safe send.
> 
> wdyt?

this suggestion sgtm. i'll see how it looks

[Ken Rockot(use gerrit already), 2015-08-04 00:37:42]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2015-08-04 04:35:58]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2015-08-04 04:39:28]

Patchset #3 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2015-08-04 04:39:32]

Patchset #2 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2015-08-04 15:53:34]

OK - turns out there are still a few places where SyncMessageFilter is
constructed manually. Mostly tests, but NaCl also subclasses it. lmkwyt.

[jam, 2015-08-04 16:29:58]

On 2015/08/04 15:53:34, Ken Rockot wrote:
> OK - turns out there are still a few places where SyncMessageFilter is
> constructed manually. Mostly tests, 

for these three tests, we can construct a SyncChannel (other unittests do this)

> but NaCl also subclasses it. lmkwyt.

nacl doesn't need to do that anymore, last week a cl landed which queues
messages: https://codereview.chromium.org/1261253002

[Ken Rockot(use gerrit already), 2015-08-04 21:08:42]

Please take another look. I factored out some common test boilerplate into a
TestThreadSafeSender, and SyncMessageFilter ctor is now protected.

One thing I've also considered (not done) is having SyncChannel always add a
SyncMessageFilter to itself upon construction, then providing a
sync_message_filter() getter. This would be instead of AddNewSyncMessageFilter.
WDYT?

[jam, 2015-08-04 23:06:42]

On 2015/08/04 21:08:42, Ken Rockot wrote:
> Please take another look. I factored out some common test boilerplate into a
> TestThreadSafeSender, and SyncMessageFilter ctor is now protected.
> 
> One thing I've also considered (not done) is having SyncChannel always add a
> SyncMessageFilter to itself upon construction, then providing a
> sync_message_filter() getter. This would be instead of
AddNewSyncMessageFilter.
> WDYT?

i think that's not necessary for now?

https://codereview.chromium.org/1262253004/diff/120001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1262253004/diff/120001/content/child/child_th...
content/child/child_thread_impl.cc:369: histogram_message_filter_ = new
ChildHistogramMessageFilter();
why move this code?

https://codereview.chromium.org/1262253004/diff/120001/content/renderer/gpu/q...
File content/renderer/gpu/queue_message_swap_promise_unittest.cc (right):

https://codereview.chromium.org/1262253004/diff/120001/content/renderer/gpu/q...
content/renderer/gpu/queue_message_swap_promise_unittest.cc:34:
TestSyncMessageFilter() : IPC::SyncMessageFilter(NULL, false) {}
it would be nice to remove this one usage, so that we can hide
SyncMessageFilter's constructor and have the only way to get it be from
SyncChannel. That way we can ensure it's not created incorrectly.

can we do this by making QueueMessageSwapPromise take an IPC::Sender and not a
SyncMessageFilter? i looked through the calls to GetRenderWidget::QueueMessage,
which from cs looks like what's leading to the vectors of messages being put in
it. i only see two calls to it, both with async IPCs. so I don't even know why
SyncMessageFilter was used. also i believe this is only called on the main
thread? in which case it doesn't make sense to use SyncMessageFilter.

would be good to check with the gpu folks who added that method

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:463: scoped_refptr<IPC::SyncMessageFilter>
SyncChannel::AddNewSyncMessageFilter() {
nit: usually functions that return a new objects start with Create.. AddNewFoo
sounds like a method that takes a newly constructed object and adds it to a
container.

CreateSyncMessageFilter() would be clearer i think, and the comment can say that
it also adds it.

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:464: scoped_refptr<IPC::SyncMessageFilter> filter = new
IPC::SyncMessageFilter(
nit: no IPC::

[jabdelmalek, 2015-08-04 23:45:36]

jabdelmalek@google.com changed reviewers:
+ jabdelmalek@google.com

[jabdelmalek, 2015-08-04 23:45:36]

lgtm

[jabdelmalek, 2015-08-04 23:45:45]

(based on IMs)

[Ken Rockot(use gerrit already), 2015-08-04 23:55:45]

https://codereview.chromium.org/1262253004/diff/120001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1262253004/diff/120001/content/child/child_th...
content/child/child_thread_impl.cc:369: histogram_message_filter_ = new
ChildHistogramMessageFilter();
On 2015/08/04 23:06:41, jam wrote:
> why move this code?

only to preserve the order of the filters. reverted since this doesn't matter

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:463: scoped_refptr<IPC::SyncMessageFilter>
SyncChannel::AddNewSyncMessageFilter() {
On 2015/08/04 23:06:41, jam wrote:
> nit: usually functions that return a new objects start with Create.. AddNewFoo
> sounds like a method that takes a newly constructed object and adds it to a
> container.
> 
> CreateSyncMessageFilter() would be clearer i think, and the comment can say
that
> it also adds it.

Done.

https://codereview.chromium.org/1262253004/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:464: scoped_refptr<IPC::SyncMessageFilter> filter = new
IPC::SyncMessageFilter(
On 2015/08/04 23:06:41, jam wrote:
> nit: no IPC::

Done.

[Ken Rockot(use gerrit already), 2015-08-05 00:11:43]

Patchset #4 (id:140001) has been deleted

[jam, 2015-08-05 00:14:43]

lgtm

[Ken Rockot(use gerrit already), 2015-08-05 00:35:05]

rockot@chromium.org changed reviewers:
+ kbr@chromium.org, michaeln@chromium.org, mseaborn@chromium.org

[Ken Rockot(use gerrit already), 2015-08-05 00:35:06]

+kbr: Please review content/renderer/gpu/
+michaeln: Please review content/child/indexed_db/
+mseaborn: Please review components/nacl and ppapi/

Thanks!

[Ken Rockot(use gerrit already), 2015-08-05 00:35:28]

Patchset #1 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2015-08-05 00:35:55]

Patchset #1 (id:100001) has been deleted

[michaeln, 2015-08-05 01:46:15]

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
File content/test/test_thread_safe_sender.cc (right):

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
content/test/test_thread_safe_sender.cc:15: class FakeIPCListener : public
IPC::Listener {
this isn't used?

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
File content/test/test_thread_safe_sender.h (right):

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
content/test/test_thread_safe_sender.h:25: // tests. This creates its on IO
thread and IPC::SyncChannel and controls their
Is any of that actually needed in the unittests? I don't think Test impl here
sends anything anywhere. Would a simpler impl that looks more like this be
sufficient?

bool MockThreadSafeSender::Send(IPC::Message* msg) {
  delete msg;
  return true;
}

And if so, since it doesn't actually function, maybe call it 'mock' instead.

https://codereview.chromium.org/1262253004/diff/160001/ipc/ipc_sync_message_f...
File ipc/ipc_sync_message_filter.cc (right):

https://codereview.chromium.org/1262253004/diff/160001/ipc/ipc_sync_message_f...
ipc/ipc_sync_message_filter.cc:23: sender_->Send(message);
Nice solution to that ordering problem!

[Ken Rockot(use gerrit already), 2015-08-05 05:23:36]

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
File content/test/test_thread_safe_sender.cc (right):

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
content/test/test_thread_safe_sender.cc:15: class FakeIPCListener : public
IPC::Listener {
On 2015/08/05 01:46:15, michaeln wrote:
> this isn't used?

Oops. Yeah, dead code from a previous patch set :D

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
File content/test/test_thread_safe_sender.h (right):

https://codereview.chromium.org/1262253004/diff/160001/content/test/test_thre...
content/test/test_thread_safe_sender.h:25: // tests. This creates its on IO
thread and IPC::SyncChannel and controls their
On 2015/08/05 01:46:15, michaeln wrote:
> Is any of that actually needed in the unittests? I don't think Test impl here
> sends anything anywhere. Would a simpler impl that looks more like this be
> sufficient?
> 
> bool MockThreadSafeSender::Send(IPC::Message* msg) {
>   delete msg;
>   return true;
> }
> 
> And if so, since it doesn't actually function, maybe call it 'mock' instead.

No actually, the tests don't need any of this. This was part of an effort to
hide SyncMessageFilter's constructor - an SMF should be constructed by a
SyncChannel instead, since it knows whether or not it's thread-safe and can tell
the SMF as much. Constructing a SyncChannel needs all this stuff and I didn't
want tests to be duplicating all of it.

However, given that this approach turned out to be insufficient for NaCl's usage
I like your suggestion. The effort to create a mock then becomes small enough
though that it's probably not worth adding to content/test.

I updated the tests individually with a mock SMF.

[michaeln, 2015-08-05 18:36:56]

lgtm!

[Ken Rockot(use gerrit already), 2015-08-05 19:25:03]

rockot@chromium.org changed reviewers:
+ bbudge@chromium.org, bradnelson@chromium.org
- mseaborn@chromium.org

[Ken Rockot(use gerrit already), 2015-08-05 19:26:23]

Looks like mseaborn is OOO, so updating reviewers.

bbudge: Please take a look at ppapi/
bradnelson: Please take a look at components/nacl/
kbr: Please take a look at content/common/gpu/

Thanks!

[bbudge, 2015-08-05 19:31:02]

ppapi lgtm

[bradnelson, 2015-08-05 20:59:29]

lgtm

[Ken Russell (switch to Gerrit), 2015-08-06 00:16:32]

content/common/gpu/client lgtm

[Ken Rockot(use gerrit already), 2015-08-06 00:25:40]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-08-06 00:25:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jabdelmalek@google.com, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1262253004/#ps180001
(title: "minus some code")

[commit-bot: I haz the power, 2015-08-06 00:26:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1262253004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1262253004/180001

[commit-bot: I haz the power, 2015-08-06 00:32:34]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2015-08-06 00:33:07]

Patchset 3 (id:??) landed as
https://crrev.com/ac64ae92ff624f1befe3b973e0f0fd0204f2eac1
Cr-Commit-Position: refs/heads/master@{#342027}