Convert to base::Callback in safe_browsing client-side-detection code.

Convert to base::Callback in safe_browsing client-side-detection code.

Convert callbacks to base::Callback:
  PhishingTermFeatureExtractor::DoneCallback
  PhishingDOMFeatureExtractor::DoneCallback
  ClientSideDetectionService::ClientReportPhishingRequestCallback
  BrowserFeatureExtractor::DoneCallback
  PhishingClassifier::DoneCallback

Convert ClientSideDetectionHost::cb_factory_ to base::WeakPtrFactory.

Also some easy NewRunnableMethod->base::Bind() conversions in
client_side_detection_host.cc.


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

[Scott Hess - ex-Googler, 2011-11-15 22:49:12]

No preference as to reviewer, but I'm also not sure who's most appropriate...

My git repo has these in orthogonal changes-which-compile-individually form, if
you would rather break it down into three or four more bite-sized reviews.  Your
call.

[noelutz, 2011-11-15 23:06:32]

On 2011/11/15 22:49:12, shess wrote:
> No preference as to reviewer, but I'm also not sure who's most appropriate...
> 
> My git repo has these in orthogonal changes-which-compile-individually form,
if
> you would rather break it down into three or four more bite-sized reviews. 
Your
> call.

I can take an initial look.

[noelutz, 2011-11-15 23:16:17]

I might be slightly slow though.  I'm trying to land something this
week.  If it's urgent maybe Brian should take a look?

noe.

On Tue, Nov 15, 2011 at 3:06 PM,  <noelutz@google.com> wrote:
> On 2011/11/15 22:49:12, shess wrote:
>>
>> No preference as to reviewer, but I'm also not sure who's most
>> appropriate...
>
>> My git repo has these in orthogonal changes-which-compile-individually
>> form,
>
> if
>>
>> you would rather break it down into three or four more bite-sized reviews.
>
> Your
>>
>> call.
>
> I can take an initial look.
>
> http://codereview.chromium.org/8573018/
>



-- 
-|||--

[Scott Hess - ex-Googler, 2011-11-15 23:24:00]

On 2011/11/15 23:16:17, noelutz wrote:
> I might be slightly slow though.  I'm trying to land something this
> week.  If it's urgent maybe Brian should take a look?

It's not urgent, just a maintenance pass, the main issue is that if it goes too
long I'll forget what I'm doing and won't be able to coherently answer
questions.

[Scott Hess - ex-Googler, 2011-11-21 21:42:27]

Do you think either of you can get to this this week?  As I mentioned earlier, I
can break it up into smaller bits for easier digesting.  Mainly I sent it your
way because of how the ownership model changed a bit in some places.  Wanted to
make sure I wasn't missing something subtle.

[noelutz, 2011-11-21 21:46:05]

Looks really good.  I just have one comment.

Thanks,
noe.

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:289: const
ClientReportPhishingRequestCallback& callback) {
note: this callback can be null.

[awong, 2011-11-21 21:59:00]

Sorry for the latency...somehow I missed this in my dashboard.

I just have one confirmation question.

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:404:
weak_factory_.GetWeakPtr()));
To confirm, in this setup, if ClientSideDetectionHost is deleted before
FeatureExtractionDone is called, we will just cancel the callback.

It's better than the previous use-after-free, but what happens if
FeatureExtractionDone isn't called?  Does any dependent code not get signaled,
deadlock, or otherwise behave badly?

[noelutz, 2011-11-21 22:13:51]

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_host.cc:404:
weak_factory_.GetWeakPtr()));
On 2011/11/21 21:59:00, awong wrote:
> To confirm, in this setup, if ClientSideDetectionHost is deleted before
> FeatureExtractionDone is called, we will just cancel the callback.
> 
> It's better than the previous use-after-free, but what happens if
> FeatureExtractionDone isn't called?  Does any dependent code not get signaled,
> deadlock, or otherwise behave badly?

It's OK of the callback is never called.

[Scott Hess - ex-Googler, 2011-11-21 22:15:20]

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:289: const
ClientReportPhishingRequestCallback& callback) {
On 2011/11/21 21:46:05, noelutz wrote:
> note: this callback can be null.

That's odd.  I'm sure I had reviewed the code and verified that a callback used
like this:

  base::Callback<void(void)> cb;
  if (SomethingIsTrue()) {
    cb = base::Bind(&myfun);
  }
  cb.Run();

would be safe to call Run() in either case.  But it doesn't look that way to me
when I look at the code, now.  I might have to sprinkle some is_null() calls in
here.

[noelutz, 2011-11-21 22:24:00]

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:289: const
ClientReportPhishingRequestCallback& callback) {
On 2011/11/21 22:15:20, shess wrote:
> On 2011/11/21 21:46:05, noelutz wrote:
> > note: this callback can be null.
> 
> That's odd.  I'm sure I had reviewed the code and verified that a callback
used
> like this:
> 
>   base::Callback<void(void)> cb;
>   if (SomethingIsTrue()) {
>     cb = base::Bind(&myfun);
>   }
>   cb.Run();
> 
> would be safe to call Run() in either case.

Oh, if this code is safe then we're fine.  In other words: is it ok to call
Run() if the callback is_null()?

> But it doesn't look that way to me
> when I look at the code, now.  I might have to sprinkle some is_null() calls
in
> here.

[Scott Hess - ex-Googler, 2011-11-21 23:40:10]

On 2011/11/21 22:24:00, noelutz wrote:
>
http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/8573018/diff/1/chrome/browser/safe_browsing/cl...
> chrome/browser/safe_browsing/client_side_detection_service.cc:289: const
> ClientReportPhishingRequestCallback& callback) {
> On 2011/11/21 22:15:20, shess wrote:
> > On 2011/11/21 21:46:05, noelutz wrote:
> > > note: this callback can be null.
> > 
> > That's odd.  I'm sure I had reviewed the code and verified that a callback
> used
> > like this:
> > 
> >   base::Callback<void(void)> cb;
> >   if (SomethingIsTrue()) {
> >     cb = base::Bind(&myfun);
> >   }
> >   cb.Run();
> > 
> > would be safe to call Run() in either case.
> 
> Oh, if this code is safe then we're fine.  In other words: is it ok to call
> Run() if the callback is_null()?
> 
> > But it doesn't look that way to me
> > when I look at the code, now.  I might have to sprinkle some is_null() calls
> in
> > here.

I was totally wrong.  I wrote a test to force some of those code paths, with
logging to make sure I hit them, and it crashed without is_null() checks.  So
I've added them back.  Unfortunately, I'm woefully uncertain about the test,
since it's kinda dependent on the internal implementation (without the callback,
I'm uncertain how to make positive assertions about the callback being checked),
so I'm not including it in the CL.

In cases where the previous code did things like blah.get()->Run(), I think it
will maintain the old version's crash if the callback is_null().

[noelutz, 2011-11-22 00:18:48]

lgtm

[commit-bot: I haz the power, 2011-11-22 22:18:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8573018/10003

[commit-bot: I haz the power, 2011-11-23 00:36:50]

Change committed as 111263