Fix memory leak in IPC::ChannelProxy

The messages attached to the task created by an IPC ChannelProxy are leaked when the message loop is destroyed (the MessageLoop deletes its pending tasks on destruction, but not the messages).

BUG=17091
TEST=Run the ui_tests with Purify. We should not be leaking messages.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21900

[kuchhal, 2009-07-24 22:09:12]

This change will fix the message leak for this one place. Apparently
there are dozens of other places where we have the same type of leak
when message loop is destroyed. Since task/RunnableMethod is supposed
to have ownership of message I wonder if we can come up with some way
that will automatically delete the message if the task is deleted.
Though I couldn't think of any way to do that using existing
RunnableMethod since it is doesn't know about the type of params.

On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>
> Reviewers: darin,
>
> Description:
> The messages attached to the task created by an IPC ChannelProxy are
> leaked when the message loop is destroyed (the MessageLoop deletes its
> pending tasks on destruction, but not the messages).
>
> BUG=3D17091
> TEST=3DRun the ui_tests with Purify. We should not be leaking messages.
>
> Please review this at http://codereview.chromium.org/159366
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 ipc/ipc_channel_proxy.h
> =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>
>
> Index: ipc/ipc_channel_proxy.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
> +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
> @@ -14,6 +14,8 @@
>
> =A0namespace IPC {
>
> +class SendTask;
> +
> =A0//--------------------------------------------------------------------=
---------
> =A0// IPC::ChannelProxy
> =A0//
> @@ -174,6 +176,7 @@
>
> =A0 =A0private:
> =A0 =A0 friend class ChannelProxy;
> + =A0 =A0friend class SendTask;
> =A0 =A0 // Create the Channel
> =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode& mo=
de);
>
> @@ -199,6 +202,8 @@
> =A0 Context* context() { return context_; }
>
> =A0private:
> + =A0friend class SendTask;
> +
> =A0 void Init(const std::string& channel_id, Channel::Mode mode,
> =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pipe_no=
w);
>
> Index: ipc/ipc_channel_proxy.cc
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
> +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
> @@ -10,8 +10,30 @@
>
> =A0namespace IPC {
>
> -//----------------------------------------------------------------------=
-------
> +//----------------------------------------------------------------------=
--------
>
> +// This task ensures the message is deleted if the task is deleted witho=
ut
> +// having been run.
> +class SendTask : public Task {
> + public:
> + =A0SendTask(ChannelProxy::Context* context, Message* message)
> + =A0 =A0 =A0: context_(context),
> + =A0 =A0 =A0 =A0message_(message) {
> + =A0}
> +
> + =A0virtual void Run() {
> + =A0 =A0context_->OnSendMessage(message_.release());
> + =A0}
> +
> + private:
> + =A0scoped_refptr<ChannelProxy::Context> context_;
> + =A0scoped_ptr<Message> message_;
> +
> + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
> +};
> +
> +//----------------------------------------------------------------------=
--------
> +
> =A0ChannelProxy::Context::Context(Channel::Listener* listener,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageFil=
ter* filter,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageLoo=
p* ipc_message_loop)
> @@ -254,8 +276,8 @@
> =A0 Logging::current()->OnSendMessage(message, context_->channel_id());
> =A0#endif
>
> - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
> - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
> + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 new SendTask(context_.get(),
> message));
> =A0 return true;
> =A0}
>
>
>
>

[jam, 2009-07-24 22:12:28]

Is this really important to fix?  It seems ok to have these kinds of leaks
on shutdown, and given that Message is POD, deleting it or not doesn't have
other effects.  I think suppressions can be added that won't mask legitimate
leaks.

On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal <kuchhal@chromium.org> wrote:

>
> This change will fix the message leak for this one place. Apparently
> there are dozens of other places where we have the same type of leak
> when message loop is destroyed. Since task/RunnableMethod is supposed
> to have ownership of message I wonder if we can come up with some way
> that will automatically delete the message if the task is deleted.
> Though I couldn't think of any way to do that using existing
> RunnableMethod since it is doesn't know about the type of params.
>
> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
> >
> > Reviewers: darin,
> >
> > Description:
> > The messages attached to the task created by an IPC ChannelProxy are
> > leaked when the message loop is destroyed (the MessageLoop deletes its
> > pending tasks on destruction, but not the messages).
> >
> > BUG=17091
> > TEST=Run the ui_tests with Purify. We should not be leaking messages.
> >
> > Please review this at http://codereview.chromium.org/159366
> >
> > SVN Base: svn://chrome-svn/chrome/trunk/src/
> >
> > Affected files:
> >  M     ipc/ipc_channel_proxy.h
> >  M     ipc/ipc_channel_proxy.cc
> >
> >
> > Index: ipc/ipc_channel_proxy.h
> > ===================================================================
> > --- ipc/ipc_channel_proxy.h     (revision 21433)
> > +++ ipc/ipc_channel_proxy.h     (working copy)
> > @@ -14,6 +14,8 @@
> >
> >  namespace IPC {
> >
> > +class SendTask;
> > +
> >
> 
//-----------------------------------------------------------------------------
> >  // IPC::ChannelProxy
> >  //
> > @@ -174,6 +176,7 @@
> >
> >    private:
> >     friend class ChannelProxy;
> > +    friend class SendTask;
> >     // Create the Channel
> >     void CreateChannel(const std::string& id, const Channel::Mode& mode);
> >
> > @@ -199,6 +202,8 @@
> >   Context* context() { return context_; }
> >
> >  private:
> > +  friend class SendTask;
> > +
> >   void Init(const std::string& channel_id, Channel::Mode mode,
> >             MessageLoop* ipc_thread_loop, bool create_pipe_now);
> >
> > Index: ipc/ipc_channel_proxy.cc
> > ===================================================================
> > --- ipc/ipc_channel_proxy.cc    (revision 21433)
> > +++ ipc/ipc_channel_proxy.cc    (working copy)
> > @@ -10,8 +10,30 @@
> >
> >  namespace IPC {
> >
> >
>
-//-----------------------------------------------------------------------------
> >
>
+//------------------------------------------------------------------------------
> >
> > +// This task ensures the message is deleted if the task is deleted
> without
> > +// having been run.
> > +class SendTask : public Task {
> > + public:
> > +  SendTask(ChannelProxy::Context* context, Message* message)
> > +      : context_(context),
> > +        message_(message) {
> > +  }
> > +
> > +  virtual void Run() {
> > +    context_->OnSendMessage(message_.release());
> > +  }
> > +
> > + private:
> > +  scoped_refptr<ChannelProxy::Context> context_;
> > +  scoped_ptr<Message> message_;
> > +
> > +  DISALLOW_COPY_AND_ASSIGN(SendTask);
> > +};
> > +
> >
>
+//------------------------------------------------------------------------------
> > +
> >  ChannelProxy::Context::Context(Channel::Listener* listener,
> >                                MessageFilter* filter,
> >                                MessageLoop* ipc_message_loop)
> > @@ -254,8 +276,8 @@
> >   Logging::current()->OnSendMessage(message, context_->channel_id());
> >  #endif
> >
> > -  context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
> > -      context_.get(), &Context::OnSendMessage, message));
> > +  context_->ipc_message_loop()->PostTask(FROM_HERE,
> > +                                         new SendTask(context_.get(),
> > message));
> >   return true;
> >  }
> >
> >
> >
> >
>

[jcampan1, 2009-07-27 17:26:20]

I think it should fix the leak for all message send through a proxy
channel which is the bulk of the leaks we were seeing, right?
What are the other message related leaks it won't cover?

jay


On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal<kuchhal@chromium.org> wrote:
>
> This change will fix the message leak for this one place. Apparently
> there are dozens of other places where we have the same type of leak
> when message loop is destroyed. Since task/RunnableMethod is supposed
> to have ownership of message I wonder if we can come up with some way
> that will automatically delete the message if the task is deleted.
> Though I couldn't think of any way to do that using existing
> RunnableMethod since it is doesn't know about the type of params.
>
> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>>
>> Reviewers: darin,
>>
>> Description:
>> The messages attached to the task created by an IPC ChannelProxy are
>> leaked when the message loop is destroyed (the MessageLoop deletes its
>> pending tasks on destruction, but not the messages).
>>
>> BUG=3D17091
>> TEST=3DRun the ui_tests with Purify. We should not be leaking messages.
>>
>> Please review this at http://codereview.chromium.org/159366
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>> =A0M =A0 =A0 ipc/ipc_channel_proxy.h
>> =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>>
>>
>> Index: ipc/ipc_channel_proxy.h
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
>> +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
>> @@ -14,6 +14,8 @@
>>
>> =A0namespace IPC {
>>
>> +class SendTask;
>> +
>> =A0//-------------------------------------------------------------------=
----------
>> =A0// IPC::ChannelProxy
>> =A0//
>> @@ -174,6 +176,7 @@
>>
>> =A0 =A0private:
>> =A0 =A0 friend class ChannelProxy;
>> + =A0 =A0friend class SendTask;
>> =A0 =A0 // Create the Channel
>> =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode& m=
ode);
>>
>> @@ -199,6 +202,8 @@
>> =A0 Context* context() { return context_; }
>>
>> =A0private:
>> + =A0friend class SendTask;
>> +
>> =A0 void Init(const std::string& channel_id, Channel::Mode mode,
>> =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pipe_n=
ow);
>>
>> Index: ipc/ipc_channel_proxy.cc
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
>> +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
>> @@ -10,8 +10,30 @@
>>
>> =A0namespace IPC {
>>
>> -//---------------------------------------------------------------------=
--------
>> +//---------------------------------------------------------------------=
---------
>>
>> +// This task ensures the message is deleted if the task is deleted with=
out
>> +// having been run.
>> +class SendTask : public Task {
>> + public:
>> + =A0SendTask(ChannelProxy::Context* context, Message* message)
>> + =A0 =A0 =A0: context_(context),
>> + =A0 =A0 =A0 =A0message_(message) {
>> + =A0}
>> +
>> + =A0virtual void Run() {
>> + =A0 =A0context_->OnSendMessage(message_.release());
>> + =A0}
>> +
>> + private:
>> + =A0scoped_refptr<ChannelProxy::Context> context_;
>> + =A0scoped_ptr<Message> message_;
>> +
>> + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
>> +};
>> +
>> +//---------------------------------------------------------------------=
---------
>> +
>> =A0ChannelProxy::Context::Context(Channel::Listener* listener,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageFi=
lter* filter,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageLo=
op* ipc_message_loop)
>> @@ -254,8 +276,8 @@
>> =A0 Logging::current()->OnSendMessage(message, context_->channel_id());
>> =A0#endif
>>
>> - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMethod=
(
>> - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
>> + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 new SendTask(context_.get(),
>> message));
>> =A0 return true;
>> =A0}
>>
>>
>>
>>
>

[jcampan1, 2009-07-27 17:35:18]

As you say these are not important leaks as they happen on shutdown.
The problem with suppressing them is that we would have to add the
allocation location of any message that can be leaked to the exception
file.
And that list would have to be updated when we add new messages.
So fixing this is more a way to clean up the Purify reports.

Jay

On Fri, Jul 24, 2009 at 3:12 PM, John Abd-El-Malek<jam@chromium.org> wrote:
> Is this really important to fix? =A0It seems ok to have these kinds of le=
aks
> on shutdown, and given that Message is POD, deleting it or not doesn't ha=
ve
> other effects. =A0I think=A0suppressions=A0can be added that won't mask l=
egitimate
> leaks.
>
> On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal <kuchhal@chromium.org> wro=
te:
>>
>> This change will fix the message leak for this one place. Apparently
>> there are dozens of other places where we have the same type of leak
>> when message loop is destroyed. Since task/RunnableMethod is supposed
>> to have ownership of message I wonder if we can come up with some way
>> that will automatically delete the message if the task is deleted.
>> Though I couldn't think of any way to do that using existing
>> RunnableMethod since it is doesn't know about the type of params.
>>
>> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>> >
>> > Reviewers: darin,
>> >
>> > Description:
>> > The messages attached to the task created by an IPC ChannelProxy are
>> > leaked when the message loop is destroyed (the MessageLoop deletes its
>> > pending tasks on destruction, but not the messages).
>> >
>> > BUG=3D17091
>> > TEST=3DRun the ui_tests with Purify. We should not be leaking messages=
.
>> >
>> > Please review this at http://codereview.chromium.org/159366
>> >
>> > SVN Base: svn://chrome-svn/chrome/trunk/src/
>> >
>> > Affected files:
>> > =A0M =A0 =A0 ipc/ipc_channel_proxy.h
>> > =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>> >
>> >
>> > Index: ipc/ipc_channel_proxy.h
>> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> > --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
>> > +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
>> > @@ -14,6 +14,8 @@
>> >
>> > =A0namespace IPC {
>> >
>> > +class SendTask;
>> > +
>> >
>> > =A0//-----------------------------------------------------------------=
------------
>> > =A0// IPC::ChannelProxy
>> > =A0//
>> > @@ -174,6 +176,7 @@
>> >
>> > =A0 =A0private:
>> > =A0 =A0 friend class ChannelProxy;
>> > + =A0 =A0friend class SendTask;
>> > =A0 =A0 // Create the Channel
>> > =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode&
>> > mode);
>> >
>> > @@ -199,6 +202,8 @@
>> > =A0 Context* context() { return context_; }
>> >
>> > =A0private:
>> > + =A0friend class SendTask;
>> > +
>> > =A0 void Init(const std::string& channel_id, Channel::Mode mode,
>> > =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pipe=
_now);
>> >
>> > Index: ipc/ipc_channel_proxy.cc
>> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> > --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
>> > +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
>> > @@ -10,8 +10,30 @@
>> >
>> > =A0namespace IPC {
>> >
>> >
>> > -//-------------------------------------------------------------------=
----------
>> >
>> > +//-------------------------------------------------------------------=
-----------
>> >
>> > +// This task ensures the message is deleted if the task is deleted
>> > without
>> > +// having been run.
>> > +class SendTask : public Task {
>> > + public:
>> > + =A0SendTask(ChannelProxy::Context* context, Message* message)
>> > + =A0 =A0 =A0: context_(context),
>> > + =A0 =A0 =A0 =A0message_(message) {
>> > + =A0}
>> > +
>> > + =A0virtual void Run() {
>> > + =A0 =A0context_->OnSendMessage(message_.release());
>> > + =A0}
>> > +
>> > + private:
>> > + =A0scoped_refptr<ChannelProxy::Context> context_;
>> > + =A0scoped_ptr<Message> message_;
>> > +
>> > + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
>> > +};
>> > +
>> >
>> > +//-------------------------------------------------------------------=
-----------
>> > +
>> > =A0ChannelProxy::Context::Context(Channel::Listener* listener,
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Message=
Filter* filter,
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Message=
Loop* ipc_message_loop)
>> > @@ -254,8 +276,8 @@
>> > =A0 Logging::current()->OnSendMessage(message, context_->channel_id())=
;
>> > =A0#endif
>> >
>> > - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMeth=
od(
>> > - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
>> > + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 new SendTask(context_.get(),
>> > message));
>> > =A0 return true;
>> > =A0}
>> >
>> >
>> >
>> >
>
>

[rahulk, 2009-07-27 19:47:36]

Hmm I think you are right. A couple of dupes that I looked into do
seem to eventually end up sending message through this one place.


On Mon, Jul 27, 2009 at 10:25 AM, Jay Campan<jcampan@google.com> wrote:
> I think it should fix the leak for all message send through a proxy
> channel which is the bulk of the leaks we were seeing, right?
> What are the other message related leaks it won't cover?
>
> jay
>
>
> On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal<kuchhal@chromium.org> wrot=
e:
>>
>> This change will fix the message leak for this one place. Apparently
>> there are dozens of other places where we have the same type of leak
>> when message loop is destroyed. Since task/RunnableMethod is supposed
>> to have ownership of message I wonder if we can come up with some way
>> that will automatically delete the message if the task is deleted.
>> Though I couldn't think of any way to do that using existing
>> RunnableMethod since it is doesn't know about the type of params.
>>
>> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>>>
>>> Reviewers: darin,
>>>
>>> Description:
>>> The messages attached to the task created by an IPC ChannelProxy are
>>> leaked when the message loop is destroyed (the MessageLoop deletes its
>>> pending tasks on destruction, but not the messages).
>>>
>>> BUG=3D17091
>>> TEST=3DRun the ui_tests with Purify. We should not be leaking messages.
>>>
>>> Please review this at http://codereview.chromium.org/159366
>>>
>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>
>>> Affected files:
>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.h
>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>>>
>>>
>>> Index: ipc/ipc_channel_proxy.h
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
>>> +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
>>> @@ -14,6 +14,8 @@
>>>
>>> =A0namespace IPC {
>>>
>>> +class SendTask;
>>> +
>>> =A0//------------------------------------------------------------------=
-----------
>>> =A0// IPC::ChannelProxy
>>> =A0//
>>> @@ -174,6 +176,7 @@
>>>
>>> =A0 =A0private:
>>> =A0 =A0 friend class ChannelProxy;
>>> + =A0 =A0friend class SendTask;
>>> =A0 =A0 // Create the Channel
>>> =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode& =
mode);
>>>
>>> @@ -199,6 +202,8 @@
>>> =A0 Context* context() { return context_; }
>>>
>>> =A0private:
>>> + =A0friend class SendTask;
>>> +
>>> =A0 void Init(const std::string& channel_id, Channel::Mode mode,
>>> =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pipe_=
now);
>>>
>>> Index: ipc/ipc_channel_proxy.cc
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
>>> +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
>>> @@ -10,8 +10,30 @@
>>>
>>> =A0namespace IPC {
>>>
>>> -//--------------------------------------------------------------------=
---------
>>> +//--------------------------------------------------------------------=
----------
>>>
>>> +// This task ensures the message is deleted if the task is deleted wit=
hout
>>> +// having been run.
>>> +class SendTask : public Task {
>>> + public:
>>> + =A0SendTask(ChannelProxy::Context* context, Message* message)
>>> + =A0 =A0 =A0: context_(context),
>>> + =A0 =A0 =A0 =A0message_(message) {
>>> + =A0}
>>> +
>>> + =A0virtual void Run() {
>>> + =A0 =A0context_->OnSendMessage(message_.release());
>>> + =A0}
>>> +
>>> + private:
>>> + =A0scoped_refptr<ChannelProxy::Context> context_;
>>> + =A0scoped_ptr<Message> message_;
>>> +
>>> + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
>>> +};
>>> +
>>> +//--------------------------------------------------------------------=
----------
>>> +
>>> =A0ChannelProxy::Context::Context(Channel::Listener* listener,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageF=
ilter* filter,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MessageL=
oop* ipc_message_loop)
>>> @@ -254,8 +276,8 @@
>>> =A0 Logging::current()->OnSendMessage(message, context_->channel_id());
>>> =A0#endif
>>>
>>> - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMetho=
d(
>>> - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
>>> + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 new SendTask(context_.get(),
>>> message));
>>> =A0 return true;
>>> =A0}
>>>
>>>
>>>
>>>
>>
>

[rahulk, 2009-07-27 19:56:43]

OTOH leaks show up in Purify when NewRunnableMethod is used to post a
task (for example ipc_sync_channel.cc:390). I was referring to this
case in my original mail but may be it is a different problem.

On Mon, Jul 27, 2009 at 12:28 PM, Rahul Kuchhal<rahulk@google.com> wrote:
> Hmm I think you are right. A couple of dupes that I looked into do
> seem to eventually end up sending message through this one place.
>
>
> On Mon, Jul 27, 2009 at 10:25 AM, Jay Campan<jcampan@google.com> wrote:
>> I think it should fix the leak for all message send through a proxy
>> channel which is the bulk of the leaks we were seeing, right?
>> What are the other message related leaks it won't cover?
>>
>> jay
>>
>>
>> On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal<kuchhal@chromium.org> wro=
te:
>>>
>>> This change will fix the message leak for this one place. Apparently
>>> there are dozens of other places where we have the same type of leak
>>> when message loop is destroyed. Since task/RunnableMethod is supposed
>>> to have ownership of message I wonder if we can come up with some way
>>> that will automatically delete the message if the task is deleted.
>>> Though I couldn't think of any way to do that using existing
>>> RunnableMethod since it is doesn't know about the type of params.
>>>
>>> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>>>>
>>>> Reviewers: darin,
>>>>
>>>> Description:
>>>> The messages attached to the task created by an IPC ChannelProxy are
>>>> leaked when the message loop is destroyed (the MessageLoop deletes its
>>>> pending tasks on destruction, but not the messages).
>>>>
>>>> BUG=3D17091
>>>> TEST=3DRun the ui_tests with Purify. We should not be leaking messages=
.
>>>>
>>>> Please review this at http://codereview.chromium.org/159366
>>>>
>>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>>
>>>> Affected files:
>>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.h
>>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>>>>
>>>>
>>>> Index: ipc/ipc_channel_proxy.h
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>> --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
>>>> +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
>>>> @@ -14,6 +14,8 @@
>>>>
>>>> =A0namespace IPC {
>>>>
>>>> +class SendTask;
>>>> +
>>>> =A0//-----------------------------------------------------------------=
------------
>>>> =A0// IPC::ChannelProxy
>>>> =A0//
>>>> @@ -174,6 +176,7 @@
>>>>
>>>> =A0 =A0private:
>>>> =A0 =A0 friend class ChannelProxy;
>>>> + =A0 =A0friend class SendTask;
>>>> =A0 =A0 // Create the Channel
>>>> =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode&=
 mode);
>>>>
>>>> @@ -199,6 +202,8 @@
>>>> =A0 Context* context() { return context_; }
>>>>
>>>> =A0private:
>>>> + =A0friend class SendTask;
>>>> +
>>>> =A0 void Init(const std::string& channel_id, Channel::Mode mode,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pipe=
_now);
>>>>
>>>> Index: ipc/ipc_channel_proxy.cc
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>> --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
>>>> +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
>>>> @@ -10,8 +10,30 @@
>>>>
>>>> =A0namespace IPC {
>>>>
>>>> -//-------------------------------------------------------------------=
----------
>>>> +//-------------------------------------------------------------------=
-----------
>>>>
>>>> +// This task ensures the message is deleted if the task is deleted wi=
thout
>>>> +// having been run.
>>>> +class SendTask : public Task {
>>>> + public:
>>>> + =A0SendTask(ChannelProxy::Context* context, Message* message)
>>>> + =A0 =A0 =A0: context_(context),
>>>> + =A0 =A0 =A0 =A0message_(message) {
>>>> + =A0}
>>>> +
>>>> + =A0virtual void Run() {
>>>> + =A0 =A0context_->OnSendMessage(message_.release());
>>>> + =A0}
>>>> +
>>>> + private:
>>>> + =A0scoped_refptr<ChannelProxy::Context> context_;
>>>> + =A0scoped_ptr<Message> message_;
>>>> +
>>>> + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
>>>> +};
>>>> +
>>>> +//-------------------------------------------------------------------=
-----------
>>>> +
>>>> =A0ChannelProxy::Context::Context(Channel::Listener* listener,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Message=
Filter* filter,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Message=
Loop* ipc_message_loop)
>>>> @@ -254,8 +276,8 @@
>>>> =A0 Logging::current()->OnSendMessage(message, context_->channel_id())=
;
>>>> =A0#endif
>>>>
>>>> - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMeth=
od(
>>>> - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
>>>> + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 new SendTask(context_.get(),
>>>> message));
>>>> =A0 return true;
>>>> =A0}
>>>>
>>>>
>>>>
>>>>
>>>
>>
>

[jcampan1, 2009-07-28 18:02:47]

Hey Darin,
What's your opinion?
Do you think the change is worth it?

Jay

On Mon, Jul 27, 2009 at 12:55 PM, Rahul Kuchhal<rahulk@google.com> wrote:
> OTOH leaks show up in Purify when NewRunnableMethod is used to post a
> task (for example ipc_sync_channel.cc:390). I was referring to this
> case in my original mail but may be it is a different problem.
>
> On Mon, Jul 27, 2009 at 12:28 PM, Rahul Kuchhal<rahulk@google.com> wrote:
>> Hmm I think you are right. A couple of dupes that I looked into do
>> seem to eventually end up sending message through this one place.
>>
>>
>> On Mon, Jul 27, 2009 at 10:25 AM, Jay Campan<jcampan@google.com> wrote:
>>> I think it should fix the leak for all message send through a proxy
>>> channel which is the bulk of the leaks we were seeing, right?
>>> What are the other message related leaks it won't cover?
>>>
>>> jay
>>>
>>>
>>> On Fri, Jul 24, 2009 at 3:08 PM, Rahul Kuchhal<kuchhal@chromium.org> wr=
ote:
>>>>
>>>> This change will fix the message leak for this one place. Apparently
>>>> there are dozens of other places where we have the same type of leak
>>>> when message loop is destroyed. Since task/RunnableMethod is supposed
>>>> to have ownership of message I wonder if we can come up with some way
>>>> that will automatically delete the message if the task is deleted.
>>>> Though I couldn't think of any way to do that using existing
>>>> RunnableMethod since it is doesn't know about the type of params.
>>>>
>>>> On Fri, Jul 24, 2009 at 2:45 PM, <jcampan@chromium.org> wrote:
>>>>>
>>>>> Reviewers: darin,
>>>>>
>>>>> Description:
>>>>> The messages attached to the task created by an IPC ChannelProxy are
>>>>> leaked when the message loop is destroyed (the MessageLoop deletes it=
s
>>>>> pending tasks on destruction, but not the messages).
>>>>>
>>>>> BUG=3D17091
>>>>> TEST=3DRun the ui_tests with Purify. We should not be leaking message=
s.
>>>>>
>>>>> Please review this at http://codereview.chromium.org/159366
>>>>>
>>>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>>>
>>>>> Affected files:
>>>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.h
>>>>> =A0M =A0 =A0 ipc/ipc_channel_proxy.cc
>>>>>
>>>>>
>>>>> Index: ipc/ipc_channel_proxy.h
>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>>> --- ipc/ipc_channel_proxy.h =A0 =A0 (revision 21433)
>>>>> +++ ipc/ipc_channel_proxy.h =A0 =A0 (working copy)
>>>>> @@ -14,6 +14,8 @@
>>>>>
>>>>> =A0namespace IPC {
>>>>>
>>>>> +class SendTask;
>>>>> +
>>>>> =A0//----------------------------------------------------------------=
-------------
>>>>> =A0// IPC::ChannelProxy
>>>>> =A0//
>>>>> @@ -174,6 +176,7 @@
>>>>>
>>>>> =A0 =A0private:
>>>>> =A0 =A0 friend class ChannelProxy;
>>>>> + =A0 =A0friend class SendTask;
>>>>> =A0 =A0 // Create the Channel
>>>>> =A0 =A0 void CreateChannel(const std::string& id, const Channel::Mode=
& mode);
>>>>>
>>>>> @@ -199,6 +202,8 @@
>>>>> =A0 Context* context() { return context_; }
>>>>>
>>>>> =A0private:
>>>>> + =A0friend class SendTask;
>>>>> +
>>>>> =A0 void Init(const std::string& channel_id, Channel::Mode mode,
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 MessageLoop* ipc_thread_loop, bool create_pip=
e_now);
>>>>>
>>>>> Index: ipc/ipc_channel_proxy.cc
>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>>> --- ipc/ipc_channel_proxy.cc =A0 =A0(revision 21433)
>>>>> +++ ipc/ipc_channel_proxy.cc =A0 =A0(working copy)
>>>>> @@ -10,8 +10,30 @@
>>>>>
>>>>> =A0namespace IPC {
>>>>>
>>>>> -//------------------------------------------------------------------=
-----------
>>>>> +//------------------------------------------------------------------=
------------
>>>>>
>>>>> +// This task ensures the message is deleted if the task is deleted w=
ithout
>>>>> +// having been run.
>>>>> +class SendTask : public Task {
>>>>> + public:
>>>>> + =A0SendTask(ChannelProxy::Context* context, Message* message)
>>>>> + =A0 =A0 =A0: context_(context),
>>>>> + =A0 =A0 =A0 =A0message_(message) {
>>>>> + =A0}
>>>>> +
>>>>> + =A0virtual void Run() {
>>>>> + =A0 =A0context_->OnSendMessage(message_.release());
>>>>> + =A0}
>>>>> +
>>>>> + private:
>>>>> + =A0scoped_refptr<ChannelProxy::Context> context_;
>>>>> + =A0scoped_ptr<Message> message_;
>>>>> +
>>>>> + =A0DISALLOW_COPY_AND_ASSIGN(SendTask);
>>>>> +};
>>>>> +
>>>>> +//------------------------------------------------------------------=
------------
>>>>> +
>>>>> =A0ChannelProxy::Context::Context(Channel::Listener* listener,
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Messag=
eFilter* filter,
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Messag=
eLoop* ipc_message_loop)
>>>>> @@ -254,8 +276,8 @@
>>>>> =A0 Logging::current()->OnSendMessage(message, context_->channel_id()=
);
>>>>> =A0#endif
>>>>>
>>>>> - =A0context_->ipc_message_loop()->PostTask(FROM_HERE, NewRunnableMet=
hod(
>>>>> - =A0 =A0 =A0context_.get(), &Context::OnSendMessage, message));
>>>>> + =A0context_->ipc_message_loop()->PostTask(FROM_HERE,
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 new SendTask(context_.get(),
>>>>> message));
>>>>> =A0 return true;
>>>>> =A0}
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[darin (slow to review), 2009-07-28 18:10:36]

I think this change is fine, however, I worry that this is just patching one
particular instance of this bug.  The real problem is with using
NewRunnableMethod to pass ownership of an object.  It might be worth it to find
a better general solution for this kind of thing, but I'm not sure what that
would be.  LGTM.

[jam, 2009-07-29 18:10:29]

Agreed.  This also happens everywhere we use
IPC_MESSAGE_HANDLER_DELAY_REPLY, since we pass the IPC::Message* around.

On Tue, Jul 28, 2009 at 11:10 AM, <darin@chromium.org> wrote:

> I think this change is fine, however, I worry that this is just patching
> one particular instance of this bug.  The real problem is with using
> NewRunnableMethod to pass ownership of an object.  It might be worth it
> to find a better general solution for this kind of thing, but I'm not
> sure what that would be.  LGTM.
>
>
> http://codereview.chromium.org/159366
>