Mostly integrate new malware IP blacklist with the csd client. When

Mostly integrate new malware IP blacklist with the csd client. When
CL/32683003 is submitted we just need to change the call in
the database manager.

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

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

[mattm, 2013-10-25 06:28:11]

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:265:
AddFeature(features::kBadIpFetch + it->first, 1.0, request);
This doesn't exist in the new version, intentional?

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:241: return;
I think remove this, the DCHECK should be fine. Chrome's philosophy is that it
does happen in a release build, we'd rather have a crash report than just have
things silently break. (At least, I think calling .Run on a null callback causes
a crash?)

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:246: info.ips.end()));
If the ips map isn't used by any other functions, maybe just do a .swap with an
empty map?

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:253:
weak_factory_.GetWeakPtr(),
weakptrs shouldn't be de-referenced on a different thread. Have you tried this
in a debug build? In any case, even if the weakptr was valid when
StartExtractMalwareFeatures ran, the object could still be deleted at any point
on the UI thread while the function was still running on the IO thread.


I think what you could do here is make ExtractMalwareFeaturesOnIOThread an
anonymous function, pass in the database_manager_ (it's refcountedthreadsafe),
and pass in the copy of the ips map, and pass in a callback to something like
"FinishExtractMalwareFeatures". That callback could safely use a weakptr,
because it would only be de-reffed on the UI thread.

The ExtractMalwareFeaturesOnIOThread function could then go through the map,
remove any non-blacklisted entries, and post the ip map back to
FinishExtractMalwareFeatures on the UI thread.

FinishExtractMalwareFeatures would go through the ip map and do the actual
AddMalwareFeature calls.

(Actually you could use PostTaskAndReply for this instead of having
ExtractMalwareFeaturesOnIOThread manually post back.)

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:257: static void
CallMalwareCallback(
Chromium style is to use anonymous namespaces rather than static functions.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:264: void
BrowserFeatureExtractor::StartExtractMalwareFeatures(
Maybe name ExtractMalwareFeaturesOnIOThread?

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:278: it->second, 1.0,
request);
I don't think AddMalwareFeature is threadsafe?

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:292:
base::Bind(&CallMalwareCallback, callback, true, request));
You can just do base::Bind(callback, true, request)

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_host.cc:412: // This function
doesn't expect brose_info_ to stay around after this
browse_info_

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_host.h:95:
ClientMalwareRequest* request);
Why removing the scoped_ptr?  It's the explicit way of saying "this function
takes ownership".

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(left):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:652:
EXPECT_FALSE(csd_service_->IsBadIpAddress("192.2.0.0"));
did all of this get put somewhere in the new impl?

[noé, 2013-10-28 23:39:26]

Thanks for your comments Matt.  Please take another look.

Thanks,
noe.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:265:
AddFeature(features::kBadIpFetch + it->first, 1.0, request);
On 2013/10/25 06:28:12, mattm wrote:
> This doesn't exist in the new version, intentional?

Good catch :-).  Yes, this is intentional.  We don't need it as part of the
phishing classifier anymore.  I'll re-add it if we need it again in the future.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:241: return;
On 2013/10/25 06:28:12, mattm wrote:
> I think remove this, the DCHECK should be fine. Chrome's philosophy is that it
> does happen in a release build, we'd rather have a crash report than just have
> things silently break. (At least, I think calling .Run on a null callback
causes
> a crash?)

Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:246: info.ips.end()));
On 2013/10/25 06:28:12, mattm wrote:
> If the ips map isn't used by any other functions, maybe just do a .swap with
an
> empty map?

Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:253:
weak_factory_.GetWeakPtr(),
On 2013/10/25 06:28:12, mattm wrote:
> weakptrs shouldn't be de-referenced on a different thread. Have you tried this
> in a debug build? In any case, even if the weakptr was valid when
> StartExtractMalwareFeatures ran, the object could still be deleted at any
point
> on the UI thread while the function was still running on the IO thread.
> 
> 
> I think what you could do here is make ExtractMalwareFeaturesOnIOThread an
> anonymous function, pass in the database_manager_ (it's refcountedthreadsafe),
> and pass in the copy of the ips map, and pass in a callback to something like
> "FinishExtractMalwareFeatures". That callback could safely use a weakptr,
> because it would only be de-reffed on the UI thread.
> 
> The ExtractMalwareFeaturesOnIOThread function could then go through the map,
> remove any non-blacklisted entries, and post the ip map back to
> FinishExtractMalwareFeatures on the UI thread.
> 
> FinishExtractMalwareFeatures would go through the ip map and do the actual
> AddMalwareFeature calls.
> 
> (Actually you could use PostTaskAndReply for this instead of having
> ExtractMalwareFeaturesOnIOThread manually post back.)

Thank you very much for providing that example.  That was super useful. Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:257: static void
CallMalwareCallback(
On 2013/10/25 06:28:12, mattm wrote:
> Chromium style is to use anonymous namespaces rather than static functions.

Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:264: void
BrowserFeatureExtractor::StartExtractMalwareFeatures(
On 2013/10/25 06:28:12, mattm wrote:
> Maybe name ExtractMalwareFeaturesOnIOThread?

Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:278: it->second, 1.0,
request);
On 2013/10/25 06:28:12, mattm wrote:
> I don't think AddMalwareFeature is threadsafe?

Can you elaborate why you think it is not thread safe?  Are you concerned about
the request object?  We own it here so I don't think anybody else even knows
about this pointer.

In any case, I think this comment isn't relevant anymore since I only access the
request object on a single thread.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:292:
base::Bind(&CallMalwareCallback, callback, true, request));
On 2013/10/25 06:28:12, mattm wrote:
> You can just do base::Bind(callback, true, request)

Neat!

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_host.cc:412: // This function
doesn't expect brose_info_ to stay around after this
On 2013/10/25 06:28:12, mattm wrote:
> browse_info_

Done.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_host.h:95:
ClientMalwareRequest* request);
On 2013/10/25 06:28:12, mattm wrote:
> Why removing the scoped_ptr?  It's the explicit way of saying "this function
> takes ownership".

Kept the scoped_ptr here but had to remove it from the extractor API since it's
not possible to mock a method that has a scoped_ptr :-(.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(left):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:652:
EXPECT_FALSE(csd_service_->IsBadIpAddress("192.2.0.0"));
On 2013/10/25 06:28:12, mattm wrote:
> did all of this get put somewhere in the new impl?

I did and made them even better :-).

[mattm, 2013-10-29 01:11:46]

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:241: return;
On 2013/10/28 23:39:26, noelutz1 wrote:
> On 2013/10/25 06:28:12, mattm wrote:
> > I think remove this, the DCHECK should be fine. Chrome's philosophy is that
it
> > does happen in a release build, we'd rather have a crash report than just
have
> > things silently break. (At least, I think calling .Run on a null callback
> causes
> > a crash?)
> 
> Done.

It looks like you removed the one from the other method :)

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:278: it->second, 1.0,
request);
On 2013/10/28 23:39:26, noelutz1 wrote:
> On 2013/10/25 06:28:12, mattm wrote:
> > I don't think AddMalwareFeature is threadsafe?
> 
> Can you elaborate why you think it is not thread safe?  Are you concerned
about
> the request object?  We own it here so I don't think anybody else even knows
> about this pointer.

Oh, you're right. I missed that the request objects in ExtractMalwareFeatures
and ExtractFeatures are different objects.

> In any case, I think this comment isn't relevant anymore since I only access
the
> request object on a single thread.

indeed.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:230: void
FilterBenignIpsOnOnIOThread(
OnOn

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:247: }  // namespace
The chromium style seems to be to have the anonymous namespace at the top of the
file, though I don't think it's actually written anywhere..

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:270:
BrowserThread::PostTaskAndReply(
It should be possible to do this with ips still as a scoped_ptr instead of a
RefCountedData.  In the first bind call you can pass a raw ptr to it, in the
second call you can use base::Passed().  Ex: 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

(Do note the saving the raw ptr separately instead of calling
scoped_ptr_obj.get() in the first bind call.  base::Passed takes ownership and
clears the passed in variable, so if you call .get() in the first bind call, it
might be null due to undefined execution order.)

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:530: bool success =
true;
unnecessary?

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:180:
scoped_ptr<ClientMalwareRequest> request_copy(new ClientMalwareRequest(
hm, not sure I follow whats going on with the copying

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.cc:257:
weak_factory_(this) { }
It looks like this constructor doesn't initialize all members.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.h:59: // ownership of
the manager object.
Not sure it makes sense to say "takes ownership" of a refcounted object. You
could say it takes a reference or holds a reference.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.h:61:
SafeBrowsingDatabaseManager* database_manager);
Was it not possible to use the other constructor along with
set_safe_browsing_managers()?

[noé, 2013-10-31 02:41:12]

Thanks for your feedback.  Please take another look
noé.

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:241: return;
On 2013/10/29 01:11:47, mattm wrote:
> On 2013/10/28 23:39:26, noelutz1 wrote:
> > On 2013/10/25 06:28:12, mattm wrote:
> > > I think remove this, the DCHECK should be fine. Chrome's philosophy is
that
> it
> > > does happen in a release build, we'd rather have a crash report than just
> have
> > > things silently break. (At least, I think calling .Run on a null callback
> > causes
> > > a crash?)
> > 
> > Done.
> 
> It looks like you removed the one from the other method :)

Removed it in both cases now :).

https://codereview.chromium.org/42553002/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/browser_feature_extractor.cc:278: it->second, 1.0,
request);
On 2013/10/29 01:11:47, mattm wrote:
> On 2013/10/28 23:39:26, noelutz1 wrote:
> > On 2013/10/25 06:28:12, mattm wrote:
> > > I don't think AddMalwareFeature is threadsafe?
> > 
> > Can you elaborate why you think it is not thread safe?  Are you concerned
> about
> > the request object?  We own it here so I don't think anybody else even knows
> > about this pointer.
> 
> Oh, you're right. I missed that the request objects in ExtractMalwareFeatures
> and ExtractFeatures are different objects.
> 
> > In any case, I think this comment isn't relevant anymore since I only access
> the
> > request object on a single thread.
> 
> indeed.

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:230: void
FilterBenignIpsOnOnIOThread(
On 2013/10/29 01:11:47, mattm wrote:
> OnOn

DoneDone.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:230: void
FilterBenignIpsOnOnIOThread(
On 2013/10/29 01:11:47, mattm wrote:
> OnOn

DoneDone.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:247: }  // namespace
On 2013/10/29 01:11:47, mattm wrote:
> The chromium style seems to be to have the anonymous namespace at the top of
the
> file, though I don't think it's actually written anywhere..

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:247: }  // namespace
On 2013/10/29 01:11:47, mattm wrote:
> The chromium style seems to be to have the anonymous namespace at the top of
the
> file, though I don't think it's actually written anywhere..

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:530: bool success =
true;
On 2013/10/29 01:11:47, mattm wrote:
> unnecessary?

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:180:
scoped_ptr<ClientMalwareRequest> request_copy(new ClientMalwareRequest(
On 2013/10/29 01:11:47, mattm wrote:
> hm, not sure I follow whats going on with the copying

Fixed and added a comment.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.cc:257:
weak_factory_(this) { }
On 2013/10/29 01:11:47, mattm wrote:
> It looks like this constructor doesn't initialize all members.

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.h:59: // ownership of
the manager object.
On 2013/10/29 01:11:47, mattm wrote:
> Not sure it makes sense to say "takes ownership" of a refcounted object. You
> could say it takes a reference or holds a reference.

Done.

https://codereview.chromium.org/42553002/diff/180001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_host.h:61:
SafeBrowsingDatabaseManager* database_manager);
On 2013/10/29 01:11:47, mattm wrote:
> Was it not possible to use the other constructor along with
> set_safe_browsing_managers()?

Done.

[mattm, 2013-10-31 05:26:39]

lgtm with nits

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:45: IPUrlMap* ips) {
could add  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:51:
to_delete.push_back(it->first);
It should be possible to do this without the second loop.. if you make a copy of
the iterator, then advance it, and then use the copy of the old location to do
the erase.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:256: // Copy the IPs
because they might go away before we're done
s/Copy/Grab/ (or something)

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:514:
scoped_ptr<ClientMalwareRequest> request) {
could add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:99:
host_.reset();
Are these reset()s necessary to release references before the rest of the
teardown instead of just waiting for the destructor to get them?  Maybe add a
comment.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:230:
scoped_ptr<ClientMalwareRequest> request_copy) {
not a copy anymore

[noé, 2013-10-31 20:39:58]

Thanks.
noe.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:45: IPUrlMap* ips) {
On 2013/10/31 05:26:40, mattm wrote:
> could add  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

Done.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:51:
to_delete.push_back(it->first);
On 2013/10/31 05:26:40, mattm wrote:
> It should be possible to do this without the second loop.. if you make a copy
of
> the iterator, then advance it, and then use the copy of the old location to do
> the erase.

Actually, we don't even need a copy iterator.  Done.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:256: // Copy the IPs
because they might go away before we're done
On 2013/10/31 05:26:40, mattm wrote:
> s/Copy/Grab/ (or something)

Done.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:514:
scoped_ptr<ClientMalwareRequest> request) {
On 2013/10/31 05:26:40, mattm wrote:
> could add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:99:
host_.reset();
On 2013/10/31 05:26:40, mattm wrote:
> Are these reset()s necessary to release references before the rest of the
> teardown instead of just waiting for the destructor to get them?  Maybe add a
> comment.

Fixed.  I thought this was causing the sb_service_ to stick around but no.

https://codereview.chromium.org/42553002/diff/490001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:230:
scoped_ptr<ClientMalwareRequest> request_copy) {
On 2013/10/31 05:26:40, mattm wrote:
> not a copy anymore

Done.

[commit-bot: I haz the power, 2013-11-01 21:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/820001

[commit-bot: I haz the power, 2013-11-02 02:02:29]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-02 03:59:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/820001

[commit-bot: I haz the power, 2013-11-02 07:01:47]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-03 06:08:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/820001

[commit-bot: I haz the power, 2013-11-03 09:24:50]

Change committed as 232679

[mattm, 2013-11-04 22:20:48]

patch set 9 lgtm

[commit-bot: I haz the power, 2013-11-05 02:37:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/1230001

[commit-bot: I haz the power, 2013-11-05 03:02:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/1230001

[commit-bot: I haz the power, 2013-11-05 04:13:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 04:28:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/1230001

[commit-bot: I haz the power, 2013-11-05 08:12:16]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-05 17:21:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@chromium.org/42553002/1230001

[commit-bot: I haz the power, 2013-11-05 22:38:15]

Change committed as 233110