Only send the client-side phishing model to a renderer if its profile has SafeBrowsing enabled.

Only send the client-side phishing model to a renderer if its profile has SafeBrowsing enabled.

BUG=99390
TEST=none


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

[noelutz, 2011-10-17 17:48:12]

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

http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:222:
process->Send(new SafeBrowsingMsg_SetPhishingModel(model_str_));
What if the renderer already has an older model?  Maybe send an empty model
which tells the renderer that CSD is disabled?

[Brian Ryner, 2011-10-17 18:21:45]

Our current handling of turning CSD off on-the-fly isn't perfect, so I think
there are a few related issues:

- We should probably observe pref changes so that we can immediately
enable/disable CSD on the relevant render processes if one of the profiles turns
CSD on/off.  I think this addresses the point you brought up.

- If the CSD service is completely disabled (meaning all profiles have it
disabled) we should disable CSD in all renderers.

- Currently the renderer side does not support completely tearing down the
Scorer object, which we should do to completely stop all CSD-related activity.

If that sounds reasonable, I'll work on fixing these as part of this bug.

Thanks,

On 2011/10/17 17:48:12, noelutz wrote:
>
http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
> chrome/browser/safe_browsing/client_side_detection_service.cc:222:
> process->Send(new SafeBrowsingMsg_SetPhishingModel(model_str_));
> What if the renderer already has an older model?  Maybe send an empty model
> which tells the renderer that CSD is disabled?

[noelutz, 2011-10-17 19:23:22]

On Mon, Oct 17, 2011 at 11:21 AM,  <bryner@chromium.org> wrote:
> Our current handling of turning CSD off on-the-fly isn't perfect, so I think
> there are a few related issues:
>
> - We should probably observe pref changes so that we can immediately
> enable/disable CSD on the relevant render processes if one of the profiles
> turns
> CSD on/off.  I think this addresses the point you brought up.

Sounds good.  You can simply modify
ClientSideDetectionService::SetEnabled which is called whenever
SafeBrowsing is enabled or disabled.

> - If the CSD service is completely disabled (meaning all profiles have it
> disabled) we should disable CSD in all renderers.

How about just sending an empty model to disable csd in the renderer?

> - Currently the renderer side does not support completely tearing down the
> Scorer object, which we should do to completely stop all CSD-related
> activity.
>
> If that sounds reasonable, I'll work on fixing these as part of this bug.
>
> Thanks,
>
> On 2011/10/17 17:48:12, noelutz wrote:
>
>
http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
>>
>> File chrome/browser/safe_browsing/client_side_detection_service.cc
>> (right):
>
>
>
http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
>>
>> chrome/browser/safe_browsing/client_side_detection_service.cc:222:
>> process->Send(new SafeBrowsingMsg_SetPhishingModel(model_str_));
>> What if the renderer already has an older model?  Maybe send an empty
>> model
>> which tells the renderer that CSD is disabled?
>
>
>
> http://codereview.chromium.org/8310014/
>



-- 
-|||--

[Brian Ryner, 2011-10-18 00:21:22]

Please take another look.

[noelutz, 2011-10-18 00:33:42]

nice.  just some minor comments.

Thanks,
noé.

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

http://codereview.chromium.org/8310014/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:97:
SendModelToRenderers();  // always refresh the renderer state
What's the thought behind sending the model if the state has not changed?

http://codereview.chromium.org/8310014/diff/7001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_service.cc:218: if
(profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
Why not simply wait until enabled_ is set in SetEnabledAndRefreshState(...) and
then use that value instead of reading the pref ourselves?

[Brian Ryner, 2011-10-18 00:49:57]

I think these can both be answered with the same explanation.

The ClientSideDetectionService is a global object; the SafeBrowsing pref is
per-profile.  In the current implementation, the CSDService object is "enabled"
if _any_ profile has the pref enabled.  So, if I have two profiles open, and
disable SB in one of them, SetEnabledAndRefreshState will be called with enabled
= true.  We have to check the pref for each profile separately to see whether to
actually enable CSD for it.

Does that make sense?

On 2011/10/18 00:33:42, noelutz wrote:
> nice.  just some minor comments.
> 
> Thanks,
> noé.
> 
>
http://codereview.chromium.org/8310014/diff/7001/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
http://codereview.chromium.org/8310014/diff/7001/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/client_side_detection_service.cc:97:
> SendModelToRenderers();  // always refresh the renderer state
> What's the thought behind sending the model if the state has not changed?
> 
>
http://codereview.chromium.org/8310014/diff/7001/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/client_side_detection_service.cc:218: if
> (profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
> Why not simply wait until enabled_ is set in SetEnabledAndRefreshState(...)
and
> then use that value instead of reading the pref ourselves?

[mattm, 2011-10-18 01:26:45]

Hmmm.. confusing.  I thought we had already handled this with the
TabContentsWrapper::UpdateSafebrowsingDetectionHost and related.  I guess this
doesn't hurt anything though, and avoids those render processes from parsing the
model unnecessarily.

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

http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:76:
NotificationService::AllSources());
Should change to NotificationService::AllBrowserContextsAndSources() to indicate
it is safe.

[Brian Ryner, 2011-10-18 22:02:23]

Agreed this is a little confusing.  It might be cleaner at some point to have
the CSDHost handle sending the model to the renderers, since we already create
those conditionally based on the pref.  Of course, then the CSDService would
have to notify all of the hosts when there's a new model.

I think this should be good for now though, please take another look.

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

http://codereview.chromium.org/8310014/diff/1/chrome/browser/safe_browsing/cl...
chrome/browser/safe_browsing/client_side_detection_service.cc:76:
NotificationService::AllSources());
On 2011/10/18 01:26:46, mattm wrote:
> Should change to NotificationService::AllBrowserContextsAndSources() to
indicate
> it is safe.

Done.

[noelutz, 2011-10-18 22:49:57]

lgtm

LGTM.

Nit: maybe add a comment that explains why you're reading the pref in this case?

Thanks,
noe.

[mattm, 2011-10-19 01:12:49]

lgtm

[commit-bot: I haz the power, 2011-10-19 04:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryner@chromium.org/8310014/13002

[commit-bot: I haz the power, 2011-10-19 05:44:35]

Change committed as 106237