Cleanup IPC::ChannelProxy to use SingleThreadTaskRunner

Cleanup IPC::ChannelProxy to use SingleThreadTaskRunner

Also removed ClearIPCMessageLoop() as it is not needed anymore.

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

[jam, 2012-06-28 01:49:53]

there's no bug listed, so I can't see what's the motivation for doing this? is
it to get run of MessageLoopProxy completely?

[Sergey Ulanov, 2012-06-28 02:42:23]

On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> there's no bug listed, so I can't see what's the motivation for doing this? is
> it to get run of MessageLoopProxy completely?

I was cleaning up chromoting code (http://codereview.chromium.org/10572005) to
use SingleThreadTaskRunner and some of it uses this IPC code.
I can put ClearIPCMessageLoop() back if you are concerned about potential
regressions.

[jam, 2012-06-28 23:01:13]

On 2012/06/28 02:42:23, sergeyu wrote:
> On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > there's no bug listed, so I can't see what's the motivation for doing this?
is
> > it to get run of MessageLoopProxy completely?
> 
> I was cleaning up chromoting code (http://codereview.chromium.org/10572005) to
> use SingleThreadTaskRunner and some of it uses this IPC code.
> I can put ClearIPCMessageLoop() back if you are concerned about potential
> regressions.

I'm still not sure I understand the motivation. i.e. why are you converting the
remoting code from ML to TR? Are you trying to get rid of MLP in the code
completely? 

If you look at the comment where we call ClearIPCMessageLoop, getting rid of it
would have unintended consequences.

[Sergey Ulanov, 2012-06-28 23:31:29]

On 2012/06/28 23:01:13, John Abd-El-Malek wrote:
> On 2012/06/28 02:42:23, sergeyu wrote:
> > On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > > there's no bug listed, so I can't see what's the motivation for doing
this?
> is
> > > it to get run of MessageLoopProxy completely?
> > 
> > I was cleaning up chromoting code (http://codereview.chromium.org/10572005)
to
> > use SingleThreadTaskRunner and some of it uses this IPC code.
> > I can put ClearIPCMessageLoop() back if you are concerned about potential
> > regressions.
> 
> I'm still not sure I understand the motivation. i.e. why are you converting
the
> remoting code from ML to TR? Are you trying to get rid of MLP in the code
> completely? 

MLP is intended to be a ref-counted proxy for MessageLoop class. It is currently
an abstract interface, but it is not supposed to be (ideally at some point we
should move MessageLoopProxyImpl to MLP) - that was raised by willchan@ in this
CL: http://codereview.chromium.org/10210008/
Chromoting has some threads that don't use MessageLoop. Specifically the plugin
threads don't have MessageLoop instance so we shouldn't really have
MessageLoopProxy for such threads, but it's possible to implement
SingleThreadTaskRunner on top of PPAPI and NPAPI. There is currently
PluginMessageLoopProxy which implements MessageLoopProxy on top of
SingleThreadTaskRunner, but I'm trying to remove dependency on that
MessageLoopProxy and use SingleThreadTaskRunner consistently in remoting code.

> 
> If you look at the comment where we call ClearIPCMessageLoop, getting rid of
it
> would have unintended consequences.

That comments were written when that code was using raw pointers to MessageLoop.
Later it was changed to use MessageLoopProxy which is ref-counted - so I think
it's safe to remove this method now.

[jam, 2012-06-28 23:38:42]

On 2012/06/28 23:31:29, sergeyu wrote:
> On 2012/06/28 23:01:13, John Abd-El-Malek wrote:
> > On 2012/06/28 02:42:23, sergeyu wrote:
> > > On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > > > there's no bug listed, so I can't see what's the motivation for doing
> this?
> > is
> > > > it to get run of MessageLoopProxy completely?
> > > 
> > > I was cleaning up chromoting code
(http://codereview.chromium.org/10572005)
> to
> > > use SingleThreadTaskRunner and some of it uses this IPC code.
> > > I can put ClearIPCMessageLoop() back if you are concerned about potential
> > > regressions.
> > 
> > I'm still not sure I understand the motivation. i.e. why are you converting
> the
> > remoting code from ML to TR? Are you trying to get rid of MLP in the code
> > completely? 
> 
> MLP is intended to be a ref-counted proxy for MessageLoop class. It is
currently
> an abstract interface, but it is not supposed to be (ideally at some point we
> should move MessageLoopProxyImpl to MLP) - that was raised by willchan@ in
this
> CL: http://codereview.chromium.org/10210008/

Yep I'm familiar with MLP and TaskRunner, but see below.

> Chromoting has some threads that don't use MessageLoop. Specifically the
plugin
> threads don't have MessageLoop instance so we shouldn't really have
> MessageLoopProxy for such threads, but it's possible to implement
> SingleThreadTaskRunner on top of PPAPI and NPAPI. There is currently
> PluginMessageLoopProxy which implements MessageLoopProxy on top of
> SingleThreadTaskRunner, but I'm trying to remove dependency on that
> MessageLoopProxy and use SingleThreadTaskRunner consistently in remoting code.

What I was trying to get at, but perhaps not clearly enough, is that the
MessageLoop now that ChannelProxy for the IO thread is not any regular message
loop. It's of the MessageLoopForIO type. So making it take a generic task runner
means it would be hard to see this distinction.

This change, and the description, makes it look like ChannelProxy will now be
able to be used with an IO thread that comes from plugin threads, but that won't
be the case.

> 
> > 
> > If you look at the comment where we call ClearIPCMessageLoop, getting rid of
> it
> > would have unintended consequences.
> 
> That comments were written when that code was using raw pointers to
MessageLoop.
> Later it was changed to use MessageLoopProxy which is ref-counted - so I think
> it's safe to remove this method now.

See this part of the comment:

  80   // NOTE: this also has the side-effect of not closing the main IPC
channel to	
  81   // the browser process.  This is needed because this is the signal that
the	
  82   // browser uses to know that this process has died, so we need it to be
alive	
  83   // until this process is shut down, and the OS closes the handle	
  84   // automatically.  We used to watch the object handle on Windows to do
this,	
  85   // but it wasn't possible to do so on POSIX.

[Sergey Ulanov, 2012-06-29 19:05:02]

On 2012/06/28 23:38:42, John Abd-El-Malek wrote:
> On 2012/06/28 23:31:29, sergeyu wrote:
> > On 2012/06/28 23:01:13, John Abd-El-Malek wrote:
> > > On 2012/06/28 02:42:23, sergeyu wrote:
> > > > On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > > > > there's no bug listed, so I can't see what's the motivation for doing
> > this?
> > > is
> > > > > it to get run of MessageLoopProxy completely?
> > > > 
> > > > I was cleaning up chromoting code
> (http://codereview.chromium.org/10572005)
> > to
> > > > use SingleThreadTaskRunner and some of it uses this IPC code.
> > > > I can put ClearIPCMessageLoop() back if you are concerned about
potential
> > > > regressions.
> > > 
> > > I'm still not sure I understand the motivation. i.e. why are you
converting
> > the
> > > remoting code from ML to TR? Are you trying to get rid of MLP in the code
> > > completely? 
> > 
> > MLP is intended to be a ref-counted proxy for MessageLoop class. It is
> currently
> > an abstract interface, but it is not supposed to be (ideally at some point
we
> > should move MessageLoopProxyImpl to MLP) - that was raised by willchan@ in
> this
> > CL: http://codereview.chromium.org/10210008/
> 
> Yep I'm familiar with MLP and TaskRunner, but see below.
> 
> > Chromoting has some threads that don't use MessageLoop. Specifically the
> plugin
> > threads don't have MessageLoop instance so we shouldn't really have
> > MessageLoopProxy for such threads, but it's possible to implement
> > SingleThreadTaskRunner on top of PPAPI and NPAPI. There is currently
> > PluginMessageLoopProxy which implements MessageLoopProxy on top of
> > SingleThreadTaskRunner, but I'm trying to remove dependency on that
> > MessageLoopProxy and use SingleThreadTaskRunner consistently in remoting
code.
> 
> What I was trying to get at, but perhaps not clearly enough, is that the
> MessageLoop now that ChannelProxy for the IO thread is not any regular message
> loop. It's of the MessageLoopForIO type. So making it take a generic task
runner
> means it would be hard to see this distinction.
> 
> This change, and the description, makes it look like ChannelProxy will now be
> able to be used with an IO thread that comes from plugin threads, but that
won't
> be the case.

This code was previously using MessageLoopProxy which can be created for any
MessageLoop, not only for MessageLoopForIO, so it was already the case that
there is no compile-time checks for the type of the thread passed to this class.
This CL doesn't change anything in that respect. I've updated comments to make
the requirements clearer. Also note that ChannelProxy doesn't really care what
type of the thread corresponds to the ipc_task_runner_ as long as the
IPC::Channel implementation can work on that thread. Some implementations of
IPC::Channel may not need IO thread, e.g. ipc_channel_nacl.cc doesn't depend on
MessageLoopForIO.

> 
> > 
> > > 
> > > If you look at the comment where we call ClearIPCMessageLoop, getting rid
of
> > it
> > > would have unintended consequences.
> > 
> > That comments were written when that code was using raw pointers to
> MessageLoop.
> > Later it was changed to use MessageLoopProxy which is ref-counted - so I
think
> > it's safe to remove this method now.
> 
> See this part of the comment:
> 
>   80   // NOTE: this also has the side-effect of not closing the main IPC
> channel to	
>   81   // the browser process.  This is needed because this is the signal that
> the	
>   82   // browser uses to know that this process has died, so we need it to be
> alive	
>   83   // until this process is shut down, and the OS closes the handle	
>   84   // automatically.  We used to watch the object handle on Windows to do
> this,	
>   85   // but it wasn't possible to do so on POSIX.

This comment doesn't look to be correct anymore. ChannelProxy::Close() is called
above - it schedules a task for ChannelProxy::Context::OnChannelClosed() on the
IPC thread. Clearing ipc_message_loop_ here doesn't really change anything. The
Close() call above was added in this CL: http://codereview.chromium.org/5706001
, this comment should have been updated, but it wasn't. Am I missing something?

[jam, 2012-06-29 20:50:15]

On 2012/06/29 19:05:02, sergeyu wrote:
> On 2012/06/28 23:38:42, John Abd-El-Malek wrote:
> > On 2012/06/28 23:31:29, sergeyu wrote:
> > > On 2012/06/28 23:01:13, John Abd-El-Malek wrote:
> > > > On 2012/06/28 02:42:23, sergeyu wrote:
> > > > > On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > > > > > there's no bug listed, so I can't see what's the motivation for
doing
> > > this?
> > > > is
> > > > > > it to get run of MessageLoopProxy completely?
> > > > > 
> > > > > I was cleaning up chromoting code
> > (http://codereview.chromium.org/10572005)
> > > to
> > > > > use SingleThreadTaskRunner and some of it uses this IPC code.
> > > > > I can put ClearIPCMessageLoop() back if you are concerned about
> potential
> > > > > regressions.
> > > > 
> > > > I'm still not sure I understand the motivation. i.e. why are you
> converting
> > > the
> > > > remoting code from ML to TR? Are you trying to get rid of MLP in the
code
> > > > completely? 
> > > 
> > > MLP is intended to be a ref-counted proxy for MessageLoop class. It is
> > currently
> > > an abstract interface, but it is not supposed to be (ideally at some point
> we
> > > should move MessageLoopProxyImpl to MLP) - that was raised by willchan@ in
> > this
> > > CL: http://codereview.chromium.org/10210008/
> > 
> > Yep I'm familiar with MLP and TaskRunner, but see below.
> > 
> > > Chromoting has some threads that don't use MessageLoop. Specifically the
> > plugin
> > > threads don't have MessageLoop instance so we shouldn't really have
> > > MessageLoopProxy for such threads, but it's possible to implement
> > > SingleThreadTaskRunner on top of PPAPI and NPAPI. There is currently
> > > PluginMessageLoopProxy which implements MessageLoopProxy on top of
> > > SingleThreadTaskRunner, but I'm trying to remove dependency on that
> > > MessageLoopProxy and use SingleThreadTaskRunner consistently in remoting
> code.
> > 
> > What I was trying to get at, but perhaps not clearly enough, is that the
> > MessageLoop now that ChannelProxy for the IO thread is not any regular
message
> > loop. It's of the MessageLoopForIO type. So making it take a generic task
> runner
> > means it would be hard to see this distinction.
> > 
> > This change, and the description, makes it look like ChannelProxy will now
be
> > able to be used with an IO thread that comes from plugin threads, but that
> won't
> > be the case.
> 
> This code was previously using MessageLoopProxy which can be created for any
> MessageLoop, not only for MessageLoopForIO, so it was already the case that
> there is no compile-time checks for the type of the thread passed to this
class.
> This CL doesn't change anything in that respect. I've updated comments to make
> the requirements clearer. Also note that ChannelProxy doesn't really care what
> type of the thread corresponds to the ipc_task_runner_ as long as the
> IPC::Channel implementation can work on that thread. Some implementations of
> IPC::Channel may not need IO thread, e.g. ipc_channel_nacl.cc doesn't depend
on
> MessageLoopForIO.

I see, ok fine to switch to TR then.

> 
> > 
> > > 
> > > > 
> > > > If you look at the comment where we call ClearIPCMessageLoop, getting
rid
> of
> > > it
> > > > would have unintended consequences.
> > > 
> > > That comments were written when that code was using raw pointers to
> > MessageLoop.
> > > Later it was changed to use MessageLoopProxy which is ref-counted - so I
> think
> > > it's safe to remove this method now.
> > 
> > See this part of the comment:
> > 
> >   80   // NOTE: this also has the side-effect of not closing the main IPC
> > channel to	
> >   81   // the browser process.  This is needed because this is the signal
that
> > the	
> >   82   // browser uses to know that this process has died, so we need it to
be
> > alive	
> >   83   // until this process is shut down, and the OS closes the handle	
> >   84   // automatically.  We used to watch the object handle on Windows to
do
> > this,	
> >   85   // but it wasn't possible to do so on POSIX.
> 
> This comment doesn't look to be correct anymore. ChannelProxy::Close() is
called
> above - it schedules a task for ChannelProxy::Context::OnChannelClosed() on
the
> IPC thread. Clearing ipc_message_loop_ here doesn't really change anything.
The
> Close() call above was added in this CL:
http://codereview.chromium.org/5706001
> , this comment should have been updated, but it wasn't. Am I missing
something?

I just saw that. Looks like this got introduced in r68831 which I think had
other negative effects (i.e. r101435 was attempting to solve this). Let me get
another thread started about that change, since I think we don't want it. so
leave that stuff in.

[Sergey Ulanov, 2012-06-29 21:16:21]

On 2012/06/29 20:50:15, John Abd-El-Malek wrote:
> On 2012/06/29 19:05:02, sergeyu wrote:
> > On 2012/06/28 23:38:42, John Abd-El-Malek wrote:
> > > On 2012/06/28 23:31:29, sergeyu wrote:
> > > > On 2012/06/28 23:01:13, John Abd-El-Malek wrote:
> > > > > On 2012/06/28 02:42:23, sergeyu wrote:
> > > > > > On 2012/06/28 01:49:53, John Abd-El-Malek wrote:
> > > > > > > there's no bug listed, so I can't see what's the motivation for
> doing
> > > > this?
> > > > > is
> > > > > > > it to get run of MessageLoopProxy completely?
> > > > > > 
> > > > > > I was cleaning up chromoting code
> > > (http://codereview.chromium.org/10572005)
> > > > to
> > > > > > use SingleThreadTaskRunner and some of it uses this IPC code.
> > > > > > I can put ClearIPCMessageLoop() back if you are concerned about
> > potential
> > > > > > regressions.
> > > > > 
> > > > > I'm still not sure I understand the motivation. i.e. why are you
> > converting
> > > > the
> > > > > remoting code from ML to TR? Are you trying to get rid of MLP in the
> code
> > > > > completely? 
> > > > 
> > > > MLP is intended to be a ref-counted proxy for MessageLoop class. It is
> > > currently
> > > > an abstract interface, but it is not supposed to be (ideally at some
point
> > we
> > > > should move MessageLoopProxyImpl to MLP) - that was raised by willchan@
in
> > > this
> > > > CL: http://codereview.chromium.org/10210008/
> > > 
> > > Yep I'm familiar with MLP and TaskRunner, but see below.
> > > 
> > > > Chromoting has some threads that don't use MessageLoop. Specifically the
> > > plugin
> > > > threads don't have MessageLoop instance so we shouldn't really have
> > > > MessageLoopProxy for such threads, but it's possible to implement
> > > > SingleThreadTaskRunner on top of PPAPI and NPAPI. There is currently
> > > > PluginMessageLoopProxy which implements MessageLoopProxy on top of
> > > > SingleThreadTaskRunner, but I'm trying to remove dependency on that
> > > > MessageLoopProxy and use SingleThreadTaskRunner consistently in remoting
> > code.
> > > 
> > > What I was trying to get at, but perhaps not clearly enough, is that the
> > > MessageLoop now that ChannelProxy for the IO thread is not any regular
> message
> > > loop. It's of the MessageLoopForIO type. So making it take a generic task
> > runner
> > > means it would be hard to see this distinction.
> > > 
> > > This change, and the description, makes it look like ChannelProxy will now
> be
> > > able to be used with an IO thread that comes from plugin threads, but that
> > won't
> > > be the case.
> > 
> > This code was previously using MessageLoopProxy which can be created for any
> > MessageLoop, not only for MessageLoopForIO, so it was already the case that
> > there is no compile-time checks for the type of the thread passed to this
> class.
> > This CL doesn't change anything in that respect. I've updated comments to
make
> > the requirements clearer. Also note that ChannelProxy doesn't really care
what
> > type of the thread corresponds to the ipc_task_runner_ as long as the
> > IPC::Channel implementation can work on that thread. Some implementations of
> > IPC::Channel may not need IO thread, e.g. ipc_channel_nacl.cc doesn't depend
> on
> > MessageLoopForIO.
> 
> I see, ok fine to switch to TR then.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > If you look at the comment where we call ClearIPCMessageLoop, getting
> rid
> > of
> > > > it
> > > > > would have unintended consequences.
> > > > 
> > > > That comments were written when that code was using raw pointers to
> > > MessageLoop.
> > > > Later it was changed to use MessageLoopProxy which is ref-counted - so I
> > think
> > > > it's safe to remove this method now.
> > > 
> > > See this part of the comment:
> > > 
> > >   80   // NOTE: this also has the side-effect of not closing the main IPC
> > > channel to	
> > >   81   // the browser process.  This is needed because this is the signal
> that
> > > the	
> > >   82   // browser uses to know that this process has died, so we need it
to
> be
> > > alive	
> > >   83   // until this process is shut down, and the OS closes the handle	
> > >   84   // automatically.  We used to watch the object handle on Windows to
> do
> > > this,	
> > >   85   // but it wasn't possible to do so on POSIX.
> > 
> > This comment doesn't look to be correct anymore. ChannelProxy::Close() is
> called
> > above - it schedules a task for ChannelProxy::Context::OnChannelClosed() on
> the
> > IPC thread. Clearing ipc_message_loop_ here doesn't really change anything.
> The
> > Close() call above was added in this CL:
> http://codereview.chromium.org/5706001
> > , this comment should have been updated, but it wasn't. Am I missing
> something?
> 
> I just saw that. Looks like this got introduced in r68831 which I think had
> other negative effects (i.e. r101435 was attempting to solve this). Let me get
> another thread started about that change, since I think we don't want it. so
> leave that stuff in.

Ok, I've put it back for now with TODO to update the comment. Also renamed the
function to ClearIPCTaskRunner().

[jam, 2012-07-01 22:19:41]

lgtm with nits

http://codereview.chromium.org/10694014/diff/4010/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/10694014/diff/4010/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:43: // TODO(sergeyu): The comment below is
not up to date. Figure out if
this code block got copied even though it's not needed. you can remove it all
(line 43-54)

http://codereview.chromium.org/10694014/diff/4010/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/10694014/diff/4010/content/common/child_thread...
content/common/child_thread.cc:78: // TODO(sergeyu): The comment below is not up
to date. Figure out if
no need for the TODO, I'm looking into this :)
http://codereview.chromium.org/10702048/

http://codereview.chromium.org/10694014/diff/4010/content/public/renderer/ren...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/10694014/diff/4010/content/public/renderer/ren...
content/public/renderer/render_thread.h:27: }  // namespace IPC
nit: no need to add this here and below. we add the comment when there's enough
space between the beginning and end. for small forward declarations like this,
it's very obvious to see

http://codereview.chromium.org/10694014/diff/4010/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

http://codereview.chromium.org/10694014/diff/4010/ipc/ipc_channel_proxy.h#new...
ipc/ipc_channel_proxy.h:21: }  // namespace base
ditto

[Sergey Ulanov, 2012-07-02 18:44:34]

http://codereview.chromium.org/10694014/diff/4010/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/10694014/diff/4010/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:43: // TODO(sergeyu): The comment below is
not up to date. Figure out if
On 2012/07/01 22:19:41, John Abd-El-Malek wrote:
> this code block got copied even though it's not needed. you can remove it all
> (line 43-54)

Done.

http://codereview.chromium.org/10694014/diff/4010/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/10694014/diff/4010/content/common/child_thread...
content/common/child_thread.cc:78: // TODO(sergeyu): The comment below is not up
to date. Figure out if
On 2012/07/01 22:19:41, John Abd-El-Malek wrote:
> no need for the TODO, I'm looking into this :)
> http://codereview.chromium.org/10702048/

Done.

http://codereview.chromium.org/10694014/diff/4010/content/public/renderer/ren...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/10694014/diff/4010/content/public/renderer/ren...
content/public/renderer/render_thread.h:27: }  // namespace IPC
On 2012/07/01 22:19:41, John Abd-El-Malek wrote:
> nit: no need to add this here and below. we add the comment when there's
enough
> space between the beginning and end. for small forward declarations like this,
> it's very obvious to see

Done.

http://codereview.chromium.org/10694014/diff/4010/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

http://codereview.chromium.org/10694014/diff/4010/ipc/ipc_channel_proxy.h#new...
ipc/ipc_channel_proxy.h:21: }  // namespace base
On 2012/07/01 22:19:41, John Abd-El-Malek wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-07-02 18:44:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10694014/13003

[commit-bot: I haz the power, 2012-07-02 19:28:03]

Try job failure for 10694014-13003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...