Integrate BlacklistManager with Profile.

Integrate BlacklistManager with Profile.

Now each Profile has a BlacklistManager that maintains a compiled Blacklist for that Profile.

The system does not yet pause user-initiated web requests until the blacklist system is ready. However, the code is not supposed to be ready, and is hidden behind a --enable-privacy-blacklists command-line flag.

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

[Erik does not do reviews, 2009-11-09 18:40:01]

http://codereview.chromium.org/371063/diff/1/4
File chrome/browser/net/chrome_url_request_context.cc (left):

http://codereview.chromium.org/371063/diff/1/4#oldcode813
Line 813: // TODO(eroman): this doesn't look safe; sharing between IO and UI
threads!
If you believe that this sharing is now safe, add a comment explaining why. 
Otherwise, someone else will wonder about this in the future.

http://codereview.chromium.org/371063/diff/1/6
File chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc (right):

http://codereview.chromium.org/371063/diff/1/6#newcode37
Line 37: // We don't take a reference to |resolver|, but
mock_host_resolver_override_
nit: newline above comment

http://codereview.chromium.org/371063/diff/1/6#newcode41
Line 41: resolver->AddSimulatedFailure("www.ads.tv");
prefer you use example.com here

http://codereview.chromium.org/371063/diff/1/7
File chrome/browser/privacy_blacklist/blacklist_observer.cc (right):

http://codereview.chromium.org/371063/diff/1/7#newcode22
Line 22: class BlockedContentNotice : public Task {
add comment above class

http://codereview.chromium.org/371063/diff/1/7#newcode60
Line 60: void BlacklistObserver::NonvisualContentBlocked(const URLRequest*
request) {
add comment: // static

http://codereview.chromium.org/371063/diff/1/8
File chrome/browser/privacy_blacklist/blacklist_observer.h (right):

http://codereview.chromium.org/371063/diff/1/8#newcode10
Line 10: class BlacklistObserver {
This class needs a comment.  It also has a bit of a misleading name.  In
reality, this is what triggers UI to be displayed when content is blocked.  I'd
never expect that from its name (just from the name I'd expect it's an
interface).

http://codereview.chromium.org/371063/diff/1/9
File chrome/browser/profile.cc (left):

http://codereview.chromium.org/371063/diff/1/9#oldcode612
Line 612: HasSwitch(switches::kPrivacyBlacklist)) {
Did you intend to remove this switch with this CL?  It seems premature.

http://codereview.chromium.org/371063/diff/1/10
File chrome/browser/profile.h (right):

http://codereview.chromium.org/371063/diff/1/10#newcode290
Line 290: // Returns the Privacy Blaclist Manager for this profile.
typo: Blacklist

[eroman, 2009-11-09 19:43:09]

General: I don't understand the threading model, this seems racy.

BlacklistManager defines itself as NonThreadSafe, but we are clearly using it
across the UI and IO threads now; what happens when the UI thread updates the
blacklist while the IO thread is reading it? At best it seems there is a window
for the IO thread to be reading from a freed pointer.

http://codereview.chromium.org/371063/diff/1/4
File chrome/browser/net/chrome_url_request_context.cc (left):

http://codereview.chromium.org/371063/diff/1/4#oldcode813
Line 813: // TODO(eroman): this doesn't look safe; sharing between IO and UI
threads!
On 2009/11/09 18:40:01, Erik Kay wrote:
> If you believe that this sharing is now safe, add a comment explaining why. 
> Otherwise, someone else will wonder about this in the future.

Comment probably isn't necessary.

(The reason this was dangerous before is it was sharing a raw pointer -- now it
is a refcountdthreadsafe pointer so it is less scary. See for example
http://crbug.com/26733)

http://codereview.chromium.org/371063/diff/1/6
File chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc (right):

http://codereview.chromium.org/371063/diff/1/6#newcode37
Line 37: // We don't take a reference to |resolver|, but
mock_host_resolver_override_
Instead of a comment, just put |resolver| into a scoped_refptr<>.

http://codereview.chromium.org/371063/diff/1/6#newcode77
Line 77: scoped_ptr<net::ScopedDefaultHostResolverProc>
mock_host_resolver_override_;
This needs to be a scoped_refptr<>.

I am surprised this compiles, John just made all of the destructors private....

http://codereview.chromium.org/371063/diff/1/9
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/371063/diff/1/9#newcode770
Line 770: blacklist_manager_ = NULL;
This line and comment, are misleading.

Since BlackListManager is refcounted (and a reference is held by the
ChromeURLRequestContext), the manager is going to outlive this destructor.

If you want to NULL something, it would be wise to NULL the pointer to |Profile|
that BlacklistManager holds... that way if someone tries to dereference it after
the Profile is destroyed we will hard-crash rather than play with freed memory.

http://codereview.chromium.org/371063/diff/1/9#newcode980
Line 980: blacklist_manager_created_ = true;
why do we need this variable |blacklist_manager_create_| ? isn't it sufficient
to test != NULL ? Also in practice GetBlacklistManager() is ALWAYS going to be
called (since the ChromeURLRequestContextGetter will always ask for it), so this
doesn't seem useful...

[Aaron Boodman, 2009-11-10 19:49:50]

I'm going to defer to Erik on this review.

[Aaron Boodman, 2009-11-10 19:50:07]

I'm going to defer to Erik on this review.

http://codereview.chromium.org/371063

[Paweł Hajdan Jr., 2009-11-17 12:42:50]

+brettw, could you take a look at my changes in Profile? Should I keep the
boolean flag which indicates that the BlacklistManager has been created? In
theory it's not necessary, I could just check the value of a pointer.

Thanks for the comments, there were many serious problems with the previous
version of the CL that should be fixed now.

The removal of the command-line flag is intentional. It would be hard to keep
the flag and switch to BlacklistManager. For testing, one can just load an
extension via chrome://extensions/

I also added some more comments and checks in BM. Waiting for blacklist update
while the browser is running is now quite hard, so I removed that part of the
test. I plan to add it in the future when I have better idea how to test it in a
non-flaky way (one can always poll or do other ugly things). I tried to wait for
it reliably, but the problem is that we can quite the UI loop as a result of two
events: extension being unloaded, and blacklist being changed. Short story: it's
hard, but if you have another idea, I can try it.

[Erik does not do reviews, 2009-11-17 16:38:52]

On Tue, Nov 17, 2009 at 4:42 AM, <phajdan.jr@chromium.org> wrote:

> +brettw, could you take a look at my changes in Profile? Should I keep the
> boolean flag which indicates that the BlacklistManager has been created? In
> theory it's not necessary, I could just check the value of a pointer.
>
> Thanks for the comments, there were many serious problems with the previous
> version of the CL that should be fixed now.
>
> The removal of the command-line flag is intentional. It would be hard to
> keep
> the flag and switch to BlacklistManager. For testing, one can just load an
> extension via chrome://extensions/
>

I don't think this is acceptable.  If the way that blacklists get loaded is
via extensions, just add the command-line check at extension load time.  We
do a similar thing for extensions that use experimental APIs.  If you want
to, you could even just make it leverage that switch as well.



> I also added some more comments and checks in BM. Waiting for blacklist
> update
> while the browser is running is now quite hard, so I removed that part of
> the
> test. I plan to add it in the future when I have better idea how to test it
> in a
> non-flaky way (one can always poll or do other ugly things). I tried to
> wait for
> it reliably, but the problem is that we can quite the UI loop as a result
> of two
> events: extension being unloaded, and blacklist being changed. Short story:
> it's
> hard, but if you have another idea, I can try it.
>
>
> http://codereview.chromium.org/371063
>

[brettw, 2009-11-17 17:06:53]

On 2009/11/17 12:42:50, Paweł Hajdan Jr. wrote:
> +brettw, could you take a look at my changes in Profile? Should I keep the
> boolean flag which indicates that the BlacklistManager has been created? In
> theory it's not necessary, I could just check the value of a pointer.

The current design is on purpose. When you have a lazy init for something that
can fail, you don't want to try to create it every single time somebody requests
it, and have it fail over and over. The flag is to ensure we only try to create
it once, while staying with lazy init.

[Paweł Hajdan Jr., 2009-11-18 13:58:07]

On 2009/11/17 16:38:52, Erik Kay wrote:
> I don't think this is acceptable.  If the way that blacklists get loaded is
> via extensions, just add the command-line check at extension load time.  We
> do a similar thing for extensions that use experimental APIs.  If you want
> to, you could even just make it leverage that switch as well.

Yes, blacklists are loaded via extensions. I made a dedicated switch for that to
avoid possible confusion and problems with having too many things in one bag.
The switch disables BlacklistManager altogether - otherwise it would compile an
empty blacklist on startup. This might cause a performance regression or a
stability problem, and that's why I've added the check there and not at
extension load time. What do you think?

Patch updated, please review.

[Paweł Hajdan Jr., 2009-11-19 16:43:05]

ping!

[Erik does not do reviews, 2009-11-19 17:50:52]

I'm looking at it now

On Thu, Nov 19, 2009 at 8:43 AM, <phajdan.jr@chromium.org> wrote:

> ping!
>
>
> http://codereview.chromium.org/371063
>

[Erik does not do reviews, 2009-11-19 17:57:12]

http://codereview.chromium.org/371063/diff/16001/16006
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/371063/diff/16001/16006#newcode39
Line 39: BlacklistManager(Profile* profile, BlacklistPathProvider*
path_provider);
Can there ever be more than one?  If not, why not make the constructor private
and provide a static factory method.  Maybe that can even encapsulate the
Initialize logic.

http://codereview.chromium.org/371063/diff/16001/16006#newcode42
Line 42: // IO thread to ensure that we won't serve any requests until the
blacklist
why do we need to do this?  Safe browsing just queues up pending requests. 
Blocking the IO thread will slow down more than just outgoing network requests. 
Doing all of this file IO on the IO thread seems like a bad plan.

http://codereview.chromium.org/371063/diff/16001/16006#newcode44
Line 44: // BlacklistPathProvider must be ready before calling Initialize.
I'm not sure what the point of this comment is to a caller.  What does a caller
need to do?

http://codereview.chromium.org/371063/diff/16001/16006#newcode67
Line 67: void InitializeOnIOThread(const std::vector<FilePath>&
persistent_blacklists,
comment

http://codereview.chromium.org/371063/diff/16001/16006#newcode86
Line 86: // initialized twice.
Why would this be initialized twice?

http://codereview.chromium.org/371063/diff/16001/16007
File chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc (right):

http://codereview.chromium.org/371063/diff/16001/16007#newcode102
Line 102: ui_test_utils::RunMessageLoop();
should there be a ASSERT_TRUE(GetAndReset...) after this?

http://codereview.chromium.org/371063/diff/16001/16007#newcode104
Line 104: ui_test_utils::NavigateToURL(browser(), GURL(kTestUrl));
What is this last bit doing?  It doesn't look like you're verifying that the
navigation was blocked.

[Paweł Hajdan Jr., 2009-11-23 19:47:29]

erikkay: Please review.
eroman, jam: Mostly FYI.

My plan is to change RDH, UserScriptListener and friends in another patch to
make it easier and more readable to pause requests until some event happens.

I'm not doing that in this patch because it would become enormous. The code is
hidden behind a command-line flag, so I think it's ok.

[Paweł Hajdan Jr., 2009-11-24 17:14:22]

ping!

[Erik does not do reviews, 2009-11-25 01:28:39]

Apologies again, but I wasn't able to get to this today.  I'll look first
thing tomorrow.

Erik


On Tue, Nov 24, 2009 at 9:14 AM, <phajdan.jr@chromium.org> wrote:

> ping!
>
>
> http://codereview.chromium.org/371063
>

[Erik does not do reviews, 2009-11-25 16:58:13]

http://codereview.chromium.org/371063/diff/21001/22006
File chrome/browser/privacy_blacklist/blacklist_manager.cc (right):

http://codereview.chromium.org/371063/diff/21001/22006#newcode64
chrome/browser/privacy_blacklist/blacklist_manager.cc:64: CompileBlacklist();
Doesn't this mean that for each and every extension that's loaded, we'll
recompile the blacklist?  Why not check if the extension (in the details) has a
blacklist?

Also, perhaps this shouldn't do anything until EXTENSIONS_READY has been called.

http://codereview.chromium.org/371063/diff/21001/22006#newcode77
chrome/browser/privacy_blacklist/blacklist_manager.cc:77:
DCHECK(path_provider_->AreBlacklistPathsReady());
given that you're calling this before EXTENSIONS_READY, this will almost always
trip

http://codereview.chromium.org/371063/diff/21001/22006#newcode132
chrome/browser/privacy_blacklist/blacklist_manager.cc:132: void
BlacklistManager::DoReadBlacklist(
I'm still a bit confused by the difference "transient blacklists" and
"persistent blacklists".  These method names muddle the issue since
DoCompileBlacklist only operates on the persistent ones, but DoReadBlacklist
only operates on the transient ones.  Maybe persistent and transient should be
in the method names?  Or maybe just better comments would help.

http://codereview.chromium.org/371063/diff/21001/22007
File chrome/browser/privacy_blacklist/blacklist_manager.h (right):

http://codereview.chromium.org/371063/diff/21001/22007#newcode60
chrome/browser/privacy_blacklist/blacklist_manager.h:60: ~BlacklistManager();
this needs to be virtual

[Paweł Hajdan Jr., 2009-11-25 18:00:53]

Please take a look at the updated patch (if things seem clear now etc). I
haven't fully tested the patch yet, will finish that today.

[Erik does not do reviews, 2009-11-25 18:09:43]

LGTM - much more clear, thanks!

[Paweł Hajdan Jr., 2009-11-25 21:46:01]

Unfortunately, the unit_tests stopped passing because of the added check
that the Extension contains at least one privacy blacklist. I'd need to get
such an Extension object somehow. I see these choices:

1. Instantiate an ExtensionsService, use it as the BlacklistPathProvider,
tell it to load an extension from
chrome/test/data/extensions/common/privacy_blacklist. That would negate the
need for BlacklistPathProvider interface, so I'd then remove it, and use
ExtensionsService directly in BlacklistManager.

2. Using InitializeFromValue manually on an empty Extension object. Seems
cumbersome to me.

3. Adding some method to Extension to hackily inject a blacklist path to the
extension. Sounds too unclean to me.

What would you recommend? Maybe there's some not listed here, better way?

On Wed, Nov 25, 2009 at 19:09, <erikkay@chromium.org> wrote:

> LGTM - much more clear, thanks!
>
>
>
> http://codereview.chromium.org/371063
>

[Erik does not do reviews, 2009-11-25 22:06:23]

Sorry, I think I'm missing something.  Which test is failing?  Which check
is causing the problem?

Erik


On Wed, Nov 25, 2009 at 1:45 PM, Paweł Hajdan Jr.
<phajdan.jr@chromium.org>wrote:

> Unfortunately, the unit_tests stopped passing because of the added check
> that the Extension contains at least one privacy blacklist. I'd need to get
> such an Extension object somehow. I see these choices:
>
> 1. Instantiate an ExtensionsService, use it as the BlacklistPathProvider,
> tell it to load an extension from
> chrome/test/data/extensions/common/privacy_blacklist. That would negate the
> need for BlacklistPathProvider interface, so I'd then remove it, and use
> ExtensionsService directly in BlacklistManager.
>
> 2. Using InitializeFromValue manually on an empty Extension object. Seems
> cumbersome to me.
>
> 3. Adding some method to Extension to hackily inject a blacklist path to
> the extension. Sounds too unclean to me.
>
> What would you recommend? Maybe there's some not listed here, better way?
>
>
> On Wed, Nov 25, 2009 at 19:09, <erikkay@chromium.org> wrote:
>
>> LGTM - much more clear, thanks!
>>
>>
>>
>> http://codereview.chromium.org/371063
>>
>
>

[Paweł Hajdan Jr., 2009-11-26 07:58:48]

On Wed, Nov 25, 2009 at 23:06, Erik Kay <erikkay@chromium.org> wrote:

> Sorry, I think I'm missing something.  Which test is failing?  Which check
> is causing the problem?


Sorry, I didn't explain everything. So when running unit_tests,
BlacklistManagerTest.PathProvider hangs when waiting for the blacklist to
load. In the test, in the notifications, we're using a dummy Extension
object, which doesn't have any privacy blacklists. After I changed to
Observe method of BlacklistManager to ignore notifications for extensions
which don't have privacy blacklist, a notification for dummy Extension no
longer triggers the re-read of the blacklist.


> On Wed, Nov 25, 2009 at 1:45 PM, Paweł Hajdan Jr. <phajdan.jr@chromium.org
> > wrote:
>
>> Unfortunately, the unit_tests stopped passing because of the added check
>> that the Extension contains at least one privacy blacklist. I'd need to get
>> such an Extension object somehow. I see these choices:
>>
>> 1. Instantiate an ExtensionsService, use it as the BlacklistPathProvider,
>> tell it to load an extension from
>> chrome/test/data/extensions/common/privacy_blacklist. That would negate the
>> need for BlacklistPathProvider interface, so I'd then remove it, and use
>> ExtensionsService directly in BlacklistManager.
>>
>> 2. Using InitializeFromValue manually on an empty Extension object. Seems
>> cumbersome to me.
>>
>> 3. Adding some method to Extension to hackily inject a blacklist path to
>> the extension. Sounds too unclean to me.
>>
>> What would you recommend? Maybe there's some not listed here, better way?
>>
>>
>> On Wed, Nov 25, 2009 at 19:09, <erikkay@chromium.org> wrote:
>>
>>> LGTM - much more clear, thanks!
>>>
>>>
>>>
>>> http://codereview.chromium.org/371063
>>>
>>
>>
>

[Paweł Hajdan Jr., 2009-11-30 15:02:09]

Please take a look at the updated patch.

[Erik does not do reviews, 2009-11-30 15:05:33]

The only change is the test, right?

if so, LGTM

[Paweł Hajdan Jr., 2009-11-30 16:18:46]

On 2009/11/30 15:05:33, Erik Kay wrote:
> The only change is the test, right?

Yes, just the test so the Extension declares its privacy blacklist.

> if so, LGTM

Thanks for reviewing this quickly. Landing (try servers passed).