Create BlacklistManager, which will aggregate individual blacklists into one compiled one

Create BlacklistManager, which will aggregate individual blacklists into one compiled one
and maintain it.

This is the first step towards shipping privacy blacklists in extensions.

The next step will be to make Profile own a BlacklistManager, and make ExtensionsService a BlacklistPathsProvider.

TEST=Covered by unit_tests.
BUG=21541
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29135

[Paweł Hajdan Jr., 2009-08-14 00:17:11]

This is not a finished change (no tests, lacking comments), but I wanted to make
sure it's going in the right direction.

The idea is to add GetPrivacyBlacklistManager method to Profile, and then if you
need to get The blacklist you just get it from the BlacklistManager.

ExtensionsService would add itself as a BlacklistProvider, and fire
PRIVACY_BLACKLIST_PROVIDER_UPDATED when a blacklist is installed/uninstalled.

Some specific questions:
- how do I recognize that the compiled blacklist file does not exist or is
corrupted?
- what should we do if the compiled blacklist is still being read, and someone
requests it? At this point we answer with an empty blacklist, to avoid blocking
the caller on I/O.

[idanan, 2009-08-14 17:58:27]

There are some pieces of code (Notifications) which I am very unfamiliar with,
so it would be great if Aaron could check those (all in blacklist_manager.cc).

Other than that, I have a few small comments. Because its a WIP, understanding
the whole picture is not easy, so I may have seen things that will eventually
make sense.

- Itai

http://codereview.chromium.org/164535/diff/1/2
File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):

http://codereview.chromium.org/164535/diff/1/2#newcode47
Line 47: FilePath destination_blacklist_;
Maybe compiled_blacklist_ for consistency.

http://codereview.chromium.org/164535/diff/1/2#newcode79
Line 79: compiled_blacklist_(new Blacklist(FilePath())),
The blacklist class supports this accidentally because of the tests but it
really should not. Since you use a pointer, better make it null. It is a bit
more checking but allows an installation with no blacklists to have no impact
from this feature (other than the occasional null checks).

http://codereview.chromium.org/164535/diff/1/3
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/164535/diff/1/3#newcode25
Line 25: class BlacklistProvider {
I'm not sure why we need this. Is it to break some dependencies? There should be
only one per-profile list of text blacklists. So I guess the profile could be
the provider. 

Also this name is confusing because a Blacklist::Provider exits which is a
provider (as on a site or company) that makes blacklists.

[Paweł Hajdan Jr., 2009-09-11 00:13:36]

Patch updated. Please review.

I plan to update Profile etc. in a separate patch, because that will require new
tests, and would make this CL much bigger.

[idanan, 2009-09-11 14:53:06]

Now that I've seen all this. I'm not sure how this gets called. Maybe you're
missing a TODO?

http://codereview.chromium.org/164535/diff/2001/3001
File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):

http://codereview.chromium.org/164535/diff/2001/3001#newcode40
Line 40: if (!io.Write(destination_blacklist_)) {
Even if you are not reported errors yet, I do no think you should try to write
the blacklist if there were read errors.

http://codereview.chromium.org/164535/diff/2001/3001#newcode44
Line 44: manager_->OnBlacklistCompilationFinished();
Same here. Probably should not be called if there were read OR write errors.

http://codereview.chromium.org/164535/diff/2001/3001#newcode86
Line 86: ReadBlacklist();
ReadBlacklist causes thing to be read asynchronously. It would be very bad if
this lets requests pass unfiltered. Unless there is a mechanism to block on this
which I have not seen yet, you must wait on its completion or perform it
synchronously.

http://codereview.chromium.org/164535/diff/2001/3001#newcode133
Line 133: } else {
Add a TODO for error reporting here too.

http://codereview.chromium.org/164535/diff/2001/3001#newcode135
Line 135: CompileBlacklist();
We failed to read the blacklist (possibly because something was corrupted), so
won't this cause an infinite loop? I think not calling the *Completed function
on error would break the cycle.

http://codereview.chromium.org/164535/diff/2001/3002
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/164535/diff/2001/3002#newcode25
Line 25: class BlacklistPathsProvider {
super-nit: I would drop the 's' in Path.

http://codereview.chromium.org/164535/diff/2001/3007
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/164535/diff/2001/3007#newcode605
Line 605: PRIVACY_BLACKLIST_PATHS_PROVIDER_UPDATED,
Super-nit: I would drop the 'S' from PATH here too.

[Paweł Hajdan Jr., 2009-09-11 20:03:47]

On 2009/09/11 14:53:06, idanan wrote:
> Now that I've seen all this. I'm not sure how this gets called. Maybe you're
> missing a TODO?

This will sit in a Profile, and will replace the Blacklist returning function
there (GetBlacklist).

> http://codereview.chromium.org/164535/diff/2001/3001
> File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):
> 
> http://codereview.chromium.org/164535/diff/2001/3001#newcode40
> Line 40: if (!io.Write(destination_blacklist_)) {
> Even if you are not reported errors yet, I do no think you should try to write
> the blacklist if there were read errors.

Good catch, fixed.

> http://codereview.chromium.org/164535/diff/2001/3001#newcode86
> Line 86: ReadBlacklist();
> ReadBlacklist causes thing to be read asynchronously. It would be very bad if
> this lets requests pass unfiltered. Unless there is a mechanism to block on
this
> which I have not seen yet, you must wait on its completion or perform it
> synchronously.

I'll talk with Darin to see how to solve that. I don't want to use synchronous
ctor here, because it may cause jankiness. But the good thing is that until the
read finished, the manager will return a NULL blacklist. I'm going to recognize
it in the caller.

> http://codereview.chromium.org/164535/diff/2001/3001#newcode135
> Line 135: CompileBlacklist();
> We failed to read the blacklist (possibly because something was corrupted), so
> won't this cause an infinite loop? I think not calling the *Completed function
> on error would break the cycle.

Yes, I noticed this possibility before. Now it's less likely to enter a loop. It
would require that every write to the compiled file succeeds, but then read
fails. It it still possible, so I can handle it, or add a TODO... what do you
think would be better.

> http://codereview.chromium.org/164535/diff/2001/3002
> File chrome/browser/privacy_blacklist/blacklist_manager.h (right):
> 
> http://codereview.chromium.org/164535/diff/2001/3002#newcode25
> Line 25: class BlacklistPathsProvider {
> super-nit: I would drop the 's' in Path.

No problem. Done.

> http://codereview.chromium.org/164535/diff/2001/3007
> File chrome/common/notification_type.h (right):
> 
> http://codereview.chromium.org/164535/diff/2001/3007#newcode605
> Line 605: PRIVACY_BLACKLIST_PATHS_PROVIDER_UPDATED,
> Super-nit: I would drop the 'S' from PATH here too.

Done here too.

[stoyan, 2009-09-12 05:53:47]

http://codereview.chromium.org/164535/diff/2009/2010
File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):

http://codereview.chromium.org/164535/diff/2009/2010#newcode127
Line 127: RunTaskOnBackendLoop(new ReadBlacklistTask(this,
compiled_blacklist_path_));
Perhaps I'm missing something but why explicit Task-derived class is defined to
perform the action?
What is wrong if the workhorse is simply a method of BlacklistManager and is
created as usual with NewRunnableMethod?

[Paweł Hajdan Jr., 2009-09-14 20:43:15]

I don't want to make BlacklistManager refcounted. But indeed, I should think
about shutdown - I used WeakPtr so that the tasks won't explode when
BlacklistManager disappears. NewRunnableMethod doesn't support WeakPtr yet, but
it's planned.

idanan, ping!

[idanan_google.com, 2009-09-14 20:58:51]

Hi Pawel,

I took a brief look and it does not seem all the latest comments were
addressed yet.

What did Darin say about your initialization of the blacklist manager?

You also need to break the cycle in case of errors (even without reporting).

What about Stoyan suggestion? You could get rid of two local classes which
would be cleaner.

I'll take a closer look tomorrow.

- Itai

On Mon, Sep 14, 2009 at 16:43,  <phajdan.jr@chromium.org> wrote:
> I don't want to make BlacklistManager refcounted. But indeed, I should think
> about shutdown - I used WeakPtr so that the tasks won't explode when
> BlacklistManager disappears. NewRunnableMethod doesn't support WeakPtr yet,
> but
> it's planned.
>
> idanan, ping!
>
> http://codereview.chromium.org/164535
>

[stoyan, 2009-09-14 22:34:43]

Well, as long as you pass BlacklistManager pointer to the task (to be
used as callback), as well as some of the members of BlacklistManager
- seems the lifetime of the tasks is bound to the lifetime of the
BlacklistManager.

Stoyan


On Mon, Sep 14, 2009 at 1:43 PM,  <phajdan.jr@chromium.org> wrote:
> I don't want to make BlacklistManager refcounted. But indeed, I should think
> about shutdown - I used WeakPtr so that the tasks won't explode when
> BlacklistManager disappears. NewRunnableMethod doesn't support WeakPtr yet,
> but
> it's planned.
>
> idanan, ping!
>
> http://codereview.chromium.org/164535
>

[Paweł Hajdan Jr., 2009-09-17 20:35:10]

Itai, Stoyan, I think I fixed the issues.

- BlacklistManager is now refcounted, and I ensure that it's always destroyed on
the same thread to prevent nasty surprises. For that I need custom Tasks. I
consulted that with Darin.
- Added code to prevent an infinite loop.
- I talked to Darin, and have a plan to pause network requests until
BlacklistManager is initialized. We have infrastructure for that, and at least
SafeBrowsing code uses it (use case very similar to ours). This will be added by
separate patches, to make them easier to review and manage.

Please review!

[stoyan, 2009-09-17 21:34:14]

LG! (for the multithread/concurency stuff)

I still think simple template specialization 
RunnableMethodTraits<BlacklistManager>::ReleaseCaleee(BlacklistManager* manager)
{
  manager->loop_->ReleaseSoon(manager);   // loop_ = MessageLoop::current() in
BlacklistManager constructor
} 

is better ;-), but I'm fine anyway.

[Paweł Hajdan Jr., 2009-09-18 16:28:33]

Itai, ping.

[idanan, 2009-09-18 19:09:21]

This is much better, I only have on must-fix comment.

I do still find the logic and naming someone convoluted, mostly because a failed
read ends up calling the compile task which calls the read task again... It only
covers and edge case but does make the flag-checking logic somewhat hard to
follow.

http://codereview.chromium.org/164535/diff/11001/12002
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/164535/diff/11001/12002#newcode46
Line 46: const Blacklist* GetCompiledBlacklist() const {
There is no way for callers to distinguish that the blacklist is not ready
(because async ops have not completed) and that it failed, since the return from
this will be null in both cases.

You need to either add another flag or actually set the blacklist and have the
callers call ->is_good() on the resulting pointer if not all or block until it
is ready if it is null. It would be probably cleaner to add a public boolean
flag.

[Paweł Hajdan Jr., 2009-09-18 22:45:33]

Would it be ok to figure it out when I write the calling logic? For now we
have TODOs in place, and nobody else uses the code. Anyway, a decision will
need to be made how the browser should behave if we got an error here.
Should we allow the requests, or block them? (the browser will be
effectively bricked then). So, I think that the best option for now is to
commit this piece of code, or alternatively develop it in a one huge patch,
with everything included, and then review and land it. What do you think?

On Fri, Sep 18, 2009 at 12:09, <idanan@chromium.org> wrote:

> This is much better, I only have on must-fix comment.
>
> I do still find the logic and naming someone convoluted, mostly because a
> failed
> read ends up calling the compile task which calls the read task again... It
> only
> covers and edge case but does make the flag-checking logic somewhat hard to
> follow.
>
>
>
> http://codereview.chromium.org/164535/diff/11001/12002
> File chrome/browser/privacy_blacklist/blacklist_manager.h (right):
>
> http://codereview.chromium.org/164535/diff/11001/12002#newcode46
> Line 46: const Blacklist* GetCompiledBlacklist() const {
> There is no way for callers to distinguish that the blacklist is not
> ready (because async ops have not completed) and that it failed, since
> the return from this will be null in both cases.
>
> You need to either add another flag or actually set the blacklist and
> have the callers call ->is_good() on the resulting pointer if not all or
> block until it is ready if it is null. It would be probably cleaner to
> add a public boolean flag.
>
>
> http://codereview.chromium.org/164535
>

[idanan_google.com, 2009-09-21 14:19:22]

Well, this is a very dangerous situation because if we are not careful
then we can end-up with a
moment when blacklists are not loaded while the user thinks they are.

If you want to figure out the logic later. I suggest you make the
loading synchronous which would
remove the said risk. This is any easy change. Then, you can later
move it async when you get
the details sorted-out, you're going to need to change the class
interface anyways when you do
that.

In the case where we get an error for the blacklist we currently tell
the user and let them quit
(so that they can fix the problem) or let them continue with then
knowing that the blacklists
are not working (we said so in the dialog). You will need to do them
same thing or recreate
the previously compiled blacklist when adding a new one fails.

- Itai

On Fri, Sep 18, 2009 at 18:45, Pawe=C5=82 Hajdan Jr. <phajdan.jr@chromium.o=
rg> wrote:
> Would it be ok to figure it out when I write the calling logic? For now w=
e
> have TODOs in place, and nobody else uses the code. Anyway, a decision wi=
ll
> need to be made how the browser should behave if we got an error here.
> Should we allow the requests, or block them? (the browser will be
> effectively bricked then). So, I think that the best option for now is to
> commit this piece of code, or alternatively develop it in a one huge patc=
h,
> with everything included, and then review and land it. What do you think?
>
> On Fri, Sep 18, 2009 at 12:09, <idanan@chromium.org> wrote:
>>
>> This is much better, I only have on must-fix comment.
>>
>> I do still find the logic and naming someone convoluted, mostly because =
a
>> failed
>> read ends up calling the compile task which calls the read task again...
>> It only
>> covers and edge case but does make the flag-checking logic somewhat hard
>> to
>> follow.
>>
>>
>>
>> http://codereview.chromium.org/164535/diff/11001/12002
>> File chrome/browser/privacy_blacklist/blacklist_manager.h (right):
>>
>> http://codereview.chromium.org/164535/diff/11001/12002#newcode46
>> Line 46: const Blacklist* GetCompiledBlacklist() const {
>> There is no way for callers to distinguish that the blacklist is not
>> ready (because async ops have not completed) and that it failed, since
>> the return from this will be null in both cases.
>>
>> You need to either add another flag or actually set the blacklist and
>> have the callers call ->is_good() on the resulting pointer if not all or
>> block until it is ready if it is null. It would be probably cleaner to
>> add a public boolean flag.
>>
>> http://codereview.chromium.org/164535
>
>

[Paweł Hajdan Jr., 2009-10-15 09:49:00]

aa/stoyan, could you do a quick sanity check on the final revision of this
patch?

Itai's last comments are covered by the e-mail discussion, so it means LG now.

[stoyan, 2009-10-15 16:22:41]

LGTM

http://codereview.chromium.org/164535/diff/17001/18002
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/164535/diff/17001/18002#newcode51
Line 51: virtual void Observe(NotificationType type,
move this method to private:?