Fix the last scoped_refptr misuses in NewRunnableMethod.

Fix the last scoped_refptr misuses in NewRunnableMethod.

BUG=28083
TEST=builds

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

[willchan no longer on Chromium, 2010-12-15 01:23:50]

rdsmith/ahendrickson/phajdan.jr: download code
sergeyu/hclam/ajwong: remoting

[Sergey Ulanov, 2010-12-15 01:33:32]

On 2010/12/15 01:23:50, willchan wrote:
> rdsmith/ahendrickson/phajdan.jr: download code
> sergeyu/hclam/ajwong: remoting

Changes in remoting LGTM

[Randy Smith (Not in Mondays), 2010-12-15 02:14:53]

On 2010/12/15 01:23:50, willchan wrote:
> rdsmith/ahendrickson/phajdan.jr: download code
> sergeyu/hclam/ajwong: remoting

Will: This isn't quite an example of 28083; OnFinalDownloadName doesn't take a
scoped_refptr<>. Given that, and given that the method is being run on the file
thread and the DownloadManager basically lives on the UI thread, I don't think
creating a scoped_refptr<> while in transit does you any good whatsoever.  I'm
not sure there's a leak here (I'm going to have to spend some time tracing
control paths to satisfy myself one way or the other) but if there is, I don't
think this change does anything.  Am I missing something?

If you agree with my assessment but think there's sto;; a problem (and there may
well be--it's an interesting and tricky situation) and don't see an easy fix,
feel free to create a bug and assign it to me.

[willchan no longer on Chromium, 2010-12-15 02:18:52]

On Tue, Dec 14, 2010 at 6:14 PM, <rdsmith@chromium.org> wrote:

> On 2010/12/15 01:23:50, willchan wrote:
>
>> rdsmith/ahendrickson/phajdan.jr: download code
>> sergeyu/hclam/ajwong: remoting
>>
>
> Will: This isn't quite an example of 28083; OnFinalDownloadName doesn't
> take a
> scoped_refptr<>. Given that, and given that the method is being run on the
> file
> thread and the DownloadManager basically lives on the UI thread, I don't
> think
> creating a scoped_refptr<> while in transit does you any good whatsoever.
>  I'm
> not sure there's a leak here (I'm going to have to spend some time tracing
> control paths to satisfy myself one way or the other) but if there is, I
> don't
> think this change does anything.  Am I missing something?
>

Yes you are!  You've fallen victim to the tricky thing about 28083.  It's
not the function prototype that matters, but the function call itself!  The
RunnableMethod template will create a Params tuple that makes a copy of the
parameter as passed in.  So, const scoped_refptr& will be a const
scoped_refptr member of the Params tuple.  Then on the target then, when
RunnableMethod::Run() is invoked, it will do the implicit conversion of
scoped_refptr<T> to T*.  When the RunnableMethod::Run() completes, the
RunnableMethod will be freed, which will destroy the scoped_refptr, which
will call Release().

Tricky eh?  I'm fixing 28083 which will prevent this antipattern from biting
anyone again.


>
> If you agree with my assessment but think there's sto;; a problem (and
> there may
> well be--it's an interesting and tricky situation) and don't see an easy
> fix,
> feel free to create a bug and assign it to me.
>
>
>
> http://codereview.chromium.org/5852001/
>

[Randy Smith (Not in Mondays), 2010-12-15 02:19:51]

On 2010/12/15 02:14:53, rdsmith wrote:
> On 2010/12/15 01:23:50, willchan wrote:
> > rdsmith/ahendrickson/phajdan.jr: download code
> > sergeyu/hclam/ajwong: remoting
> 
> Will: This isn't quite an example of 28083; OnFinalDownloadName doesn't take a
> scoped_refptr<>. Given that, and given that the method is being run on the
file
> thread and the DownloadManager basically lives on the UI thread, I don't think
> creating a scoped_refptr<> while in transit does you any good whatsoever.  I'm
> not sure there's a leak here (I'm going to have to spend some time tracing
> control paths to satisfy myself one way or the other) but if there is, I don't
> think this change does anything.  Am I missing something?
> 
> If you agree with my assessment but think there's sto;; a problem (and there
may
> well be--it's an interesting and tricky situation) and don't see an easy fix,
> feel free to create a bug and assign it to me.

I took a closer look at the code, and there isn't a leak here, but not for a
reason that will warm either of our hearts.  One of the arguments to
OnFinalDownloadName is the download id, which is used to lookup the DownloadFile
object.  The DownloadFile object does hold a scoped_refptr<> to the
DownloadManager, and if it isn't found, the function returns without doing
anything (specifically, without using the DownloadManager pointer).  Intentional
or accidental?  Only the shadow knows :-J.

I think the right fix is to move the functionality of OnFinalDownloadName into
the DownloadFile object, and nuke the download_manager argument to
OnDownloadFinalName; it's redundant and an attractive nuisance.  I'll put it on
our refactor list; I feel no desperate urge to do it tomorrow, since as I say,
the current behavior is safe.  For now :-}.

[Randy Smith (Not in Mondays), 2010-12-15 02:31:09]

On 2010/12/15 02:18:52, willchan wrote:
> On Tue, Dec 14, 2010 at 6:14 PM, <mailto:rdsmith@chromium.org> wrote:
> 
> > On 2010/12/15 01:23:50, willchan wrote:
> >
> >> rdsmith/ahendrickson/phajdan.jr: download code
> >> sergeyu/hclam/ajwong: remoting
> >>
> >
> > Will: This isn't quite an example of 28083; OnFinalDownloadName doesn't
> > take a
> > scoped_refptr<>. Given that, and given that the method is being run on the
> > file
> > thread and the DownloadManager basically lives on the UI thread, I don't
> > think
> > creating a scoped_refptr<> while in transit does you any good whatsoever.
> >  I'm
> > not sure there's a leak here (I'm going to have to spend some time tracing
> > control paths to satisfy myself one way or the other) but if there is, I
> > don't
> > think this change does anything.  Am I missing something?
> >
> 
> Yes you are!  You've fallen victim to the tricky thing about 28083.  It's
> not the function prototype that matters, but the function call itself!  The
> RunnableMethod template will create a Params tuple that makes a copy of the
> parameter as passed in.  So, const scoped_refptr& will be a const
> scoped_refptr member of the Params tuple.  Then on the target then, when
> RunnableMethod::Run() is invoked, it will do the implicit conversion of
> scoped_refptr<T> to T*.  When the RunnableMethod::Run() completes, the
> RunnableMethod will be freed, which will destroy the scoped_refptr, which
> will call Release().
> 
> Tricky eh?  I'm fixing 28083 which will prevent this antipattern from biting
> anyone again.
> 
> 
> >
> > If you agree with my assessment but think there's sto;; a problem (and
> > there may
> > well be--it's an interesting and tricky situation) and don't see an easy
> > fix,
> > feel free to create a bug and assign it to me.
> >
> >
> >
> > http://codereview.chromium.org/5852001/
> >

Got it!  (Admittedly after some offline chatting.)  LGTM.

[Paweł Hajdan Jr., 2010-12-15 08:36:46]

Download change LGTM.