Add "enabled" state to the ClientSideDetectionService.

Add "enabled" state to the ClientSideDetectionService.

The service won't download the model nor report phishing URLs when disabled.
It is only enabled if it exists (only when client side reporting is not
disabled), and at least one profile has safe browsing enabled.

BUG=88661
TEST=SafeBrowsing and phishing detection work as before.


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

[Joao da Silva, 2011-08-08 16:43:50]

@shess: Please review.

[Joao da Silva, 2011-08-08 16:45:47]

@shess: Please review. (sorry for the other email, hit enter too soon)

@mattm, @mirandac: FYI. Please have a look too; I think this closes 88661, and
if you agree I'll merge to m14.

[mattm, 2011-08-08 21:59:46]

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:111: // we have to
invoke the callback.
Hm, are you sure this can't use the same clean-up mechanism the destructor uses?

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:589:
MockClientSideDetectionService* service = new MockClientSideDetectionService;
Need to use StrictMock for the checks of unexpected calls to work.

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:593: 
Should do a Mock::VerifyAndClearExpectations(service) here to check for any
unexpected calls before setting SetEnabled?

[mirandac, 2011-08-09 08:06:53]

Looks good to me for closing 88661 -- thank you!

On Mon, Aug 8, 2011 at 18:45, <joaodasilva@chromium.org> wrote:

> @shess: Please review. (sorry for the other email, hit enter too soon)
>
> @mattm, @mirandac: FYI. Please have a look too; I think this closes 88661,
> and
> if you agree I'll merge to m14.
>
>
>
http://codereview.chromium.**org/7583007/<http://codereview.chromium.org/7583...
>

[Joao da Silva, 2011-08-09 09:16:12]

@mattm: thanks for reviewing, please see the comment below.

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:111: // we have to
invoke the callback.
On 2011/08/08 21:59:46, mattm wrote:
> Hm, are you sure this can't use the same clean-up mechanism the destructor
uses?

It can, but deleting the URLFetchers will cancel their callbacks (to
OnURLFetchComplete), and therefore the callbacks passed to
SendClientReportPhishingRequest won't be called, and the client of CSDS might be
waiting for it.

We could also invoke all the callbacks with a "false" verdict before deleting
them here... wdyt?

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:589:
MockClientSideDetectionService* service = new MockClientSideDetectionService;
On 2011/08/08 21:59:46, mattm wrote:
> Need to use StrictMock for the checks of unexpected calls to work.

Done.

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:593: 
On 2011/08/08 21:59:46, mattm wrote:
> Should do a Mock::VerifyAndClearExpectations(service) here to check for any
> unexpected calls before setting SetEnabled?

Done.

[mattm, 2011-08-10 23:38:26]

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:111: // we have to
invoke the callback.
On 2011/08/09 09:16:12, Joao da Silva wrote:
> On 2011/08/08 21:59:46, mattm wrote:
> > Hm, are you sure this can't use the same clean-up mechanism the destructor
> uses?
> 
> It can, but deleting the URLFetchers will cancel their callbacks (to
> OnURLFetchComplete), and therefore the callbacks passed to
> SendClientReportPhishingRequest won't be called, and the client of CSDS might
be
> waiting for it.

With the current implementation it wouldn't care, but I guess it might be better
not to depend on that.

> We could also invoke all the callbacks with a "false" verdict before deleting
> them here... wdyt? 


Yeah, that seems cleaner than waiting for the requests to complete and checking
the enabled flag in HandlePhishingVerdict.

[Scott Hess - ex-Googler, 2011-08-10 23:54:15]

I don't have anything to add - it looks right, but I'm not entirely confident
I'd be able to catch missed edge cases.

[Joao da Silva, 2011-08-11 11:51:50]

@mattm: reviewed, please have another look. Thanks!

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:111: // we have to
invoke the callback.
On 2011/08/10 23:38:26, mattm wrote:
> On 2011/08/09 09:16:12, Joao da Silva wrote:
> > On 2011/08/08 21:59:46, mattm wrote:
> > > Hm, are you sure this can't use the same clean-up mechanism the destructor
> > uses?
> > 
> > It can, but deleting the URLFetchers will cancel their callbacks (to
> > OnURLFetchComplete), and therefore the callbacks passed to
> > SendClientReportPhishingRequest won't be called, and the client of CSDS
might
> be
> > waiting for it.
> 
> With the current implementation it wouldn't care, but I guess it might be
better
> not to depend on that.
> 
> > We could also invoke all the callbacks with a "false" verdict before
deleting
> > them here... wdyt? 
> 
> 
> Yeah, that seems cleaner than waiting for the requests to complete and
checking
> the enabled flag in HandlePhishingVerdict.

Agree, done.

[mattm, 2011-08-11 21:59:29]

Thanks.  Did you see the SafeBrowsingServiceTest.StartAndStop failure on the Mac
try bot?

On 2011/08/11 11:51:50, Joao da Silva wrote:
> @mattm: reviewed, please have another look. Thanks!
> 
>
http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/7583007/diff/4001/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/client_side_detection_service.cc:111: // we have
to
> invoke the callback.
> On 2011/08/10 23:38:26, mattm wrote:
> > On 2011/08/09 09:16:12, Joao da Silva wrote:
> > > On 2011/08/08 21:59:46, mattm wrote:
> > > > Hm, are you sure this can't use the same clean-up mechanism the
destructor
> > > uses?
> > > 
> > > It can, but deleting the URLFetchers will cancel their callbacks (to
> > > OnURLFetchComplete), and therefore the callbacks passed to
> > > SendClientReportPhishingRequest won't be called, and the client of CSDS
> might
> > be
> > > waiting for it.
> > 
> > With the current implementation it wouldn't care, but I guess it might be
> better
> > not to depend on that.
> > 
> > > We could also invoke all the callbacks with a "false" verdict before
> deleting
> > > them here... wdyt? 
> > 
> > 
> > Yeah, that seems cleaner than waiting for the requests to complete and
> checking
> > the enabled flag in HandlePhishingVerdict.
> 
> Agree, done.

[Joao da Silva, 2011-08-12 15:31:10]

@mattm: it was a nasty shutdown crash, it's been fixed. Please have another
look, thank you!

The problem was that the CSDS created in the test was scoped to the test
function, and deleted while the SBS still held a pointer to it. During shutdown
the SBS was notified of the destruction of the profile (BrowserProcessImpl
deletes the ProfileManager while SBS still lives), and invoked
CSDS->SetEnabled(x), leading to the crash.

I've made the SBS the owner of the CSDS, which only exists if the SBS exists too
anyway. This makes the dependency clearer. The current CL also distinguishes
between Stopping the service and ShutDown, which is invoked during shutdown just
before the IO thread goes away. ShutDown() now deletes the CSDS at the right
time.

I've left it running with gtest_repeats=-1 on Linux and Mac for a long while,
and didn't have any issues. Doesn't prove anything :-) But I think this CL is
sane.

Thanks for the help!

[mattm, 2011-08-12 21:36:33]

LGTM, thanks!

[Brian Ryner, 2011-08-13 00:34:40]

Noe, do you want to take a look at this too?

[noelutz, 2011-08-13 01:24:10]

On 2011/08/13 00:34:40, Brian Ryner wrote:
> Noe, do you want to take a look at this too?

nit: you might want to change
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc where it creates the
client-side detection host object.  I think the SafeBrowsing pref check is not
necessary anymore since you have the enabled() accessor.  I would check if the
service object exists and if it's enabled.

[noelutz, 2011-08-13 01:24:25]

LGTM other than that

Thanks,
noe.

On 2011/08/13 01:24:10, noelutz wrote:
> On 2011/08/13 00:34:40, Brian Ryner wrote:
> > Noe, do you want to take a look at this too?
> 
> nit: you might want to change
> chrome/browser/ui/tab_contents/tab_contents_wrapper.cc where it creates the
> client-side detection host object.  I think the SafeBrowsing pref check is not
> necessary anymore since you have the enabled() accessor.  I would check if the
> service object exists and if it's enabled.

[Joao da Silva, 2011-08-16 11:01:50]

On 2011/08/13 01:24:10, noelutz wrote:
> nit: you might want to change
> chrome/browser/ui/tab_contents/tab_contents_wrapper.cc where it creates the
> client-side detection host object.  I think the SafeBrowsing pref check is not
> necessary anymore since you have the enabled() accessor.  I would check if the
> service object exists and if it's enabled.

The pref check is still necessary, since the service can be used by a profile A
but not by profile B. The service itself stays enabled as long as at least one
profile uses it.

@shess: need OWNER approval to land this, so I'll commit if it looks good to
you.

[Joao da Silva, 2011-08-16 11:06:57]

@lzheng: since @shess seems to be away, can you give this an owners approval?

[noelutz, 2011-08-16 14:56:33]

LGTM 

On 2011/08/16 11:01:50, Joao da Silva wrote:
> On 2011/08/13 01:24:10, noelutz wrote:
> > nit: you might want to change
> > chrome/browser/ui/tab_contents/tab_contents_wrapper.cc where it creates the
> > client-side detection host object.  I think the SafeBrowsing pref check is
not
> > necessary anymore since you have the enabled() accessor.  I would check if
the
> > service object exists and if it's enabled.
> 
> The pref check is still necessary, since the service can be used by a profile
A
> but not by profile B. The service itself stays enabled as long as at least one
> profile uses it.

Interesting.  I didn't know you could have multiple profiles besides incognito
mode.

Thanks for the clarification.
noé.

> 
> @shess: need OWNER approval to land this, so I'll commit if it looks good to
> you.

[Brian Ryner, 2011-08-22 21:19:00]

Is this ready to go?  Matt suggested that I wait on submitting
http://codereview.chromium.org/7635010/ until this has landed.

[Scott Hess - ex-Googler, 2011-08-22 21:37:33]

LGTM.

Unfortunately, lzheng@chromium.org has left the building.  I should go looking
for a replacement victim.

[commit-bot: I haz the power, 2011-08-23 12:56:10]

Change committed as 97847