Fix a possible crash in the ClientSideDetectionService.

Fix a possible crash in the ClientSideDetectionService.

This fixes a problem where if the model fetch is still running when the
ClientSideDetectionService is destroyed, the URLFetcher is not deleted.
This could potentially cause the OnURLFetchComplete callback to be called
on a deleted ClientSideDetectionService object.

Now the model fetcher will always be deleted, and we will also make sure to
destroy the ClientSideDetectionService before tearing down the IO thread
(since the URLFetcher dtor can post messages there).

BUG=none
TEST=none

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

[noelutz, 2011-01-19 17:14:25]

Nice catch!  Thanks for fixing this.

noe.

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

http://codereview.chromium.org/6298004/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:111:
HandlePhishingVerdict(source, url, status, response_code, cookies, data);
How about deleting source here and then calling reset() on the model_fetcher_
right after HandleModelResponse() returns.  That way the URLFetcher object is
deleted at the same place in both cases.  That seems more clear to me. 
Thoughts?

[Brian Ryner, 2011-01-19 20:11:15]

I'd thought about that, but since it's not really necessary to delete
model_fetcher_ until the destructor runs, it seemed a little clearer to me to
not worry about that one and just delete the "report phishing" fetchers at the
time they are removed from the map.

On 2011/01/19 17:14:25, noelutz wrote:
> Nice catch!  Thanks for fixing this.
> 
> noe.
> 
>
http://codereview.chromium.org/6298004/diff/1/chrome/browser/safe_browsing/cl...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/6298004/diff/1/chrome/browser/safe_browsing/cl...
> chrome/browser/safe_browsing/client_side_detection_service.cc:111:
> HandlePhishingVerdict(source, url, status, response_code, cookies, data);
> How about deleting source here and then calling reset() on the model_fetcher_
> right after HandleModelResponse() returns.  That way the URLFetcher object is
> deleted at the same place in both cases.  That seems more clear to me. 
> Thoughts?

[noelutz, 2011-01-19 20:16:59]

LGTM,
noe.

[Brian Ryner, 2011-01-19 22:49:38]

Lei is out this week; Eric do you want to take a look?

[eroman, 2011-01-20 00:12:21]

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

http://codereview.chromium.org/6298004/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:303: delete
source;
Isn't this a double-free?
You changed model_fetcher_ into a scoped_refptr<>, so why delete it here too?

[Brian Ryner, 2011-01-20 00:20:36]

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

http://codereview.chromium.org/6298004/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:303: delete
source;
On 2011/01/20 00:12:21, eroman wrote:
> Isn't this a double-free?
> You changed model_fetcher_ into a scoped_refptr<>, so why delete it here too?

I don't think so, no.  ClientSideDetectionService creates two types of
URLFetchers:

- model_fetcher_ is used to fetch the client-side detection model, and is owned
by the scoped_ptr I added.
- pingbacks to the server each create a new URLFetcher which is inserted into
client_phishing_reports_.  These are what I'm deleting here.

Does that make sense?

[eroman, 2011-01-20 00:58:17]

indeed you are right, I read that too hastily.

LGTM