Collect some histograms about signed binary downloads.

Collect some histograms about signed binary downloads.

This hooks up the DownloadProtectionService to run after a download finishes.  For now, this does not send a download pingback since the flag defaults to off.


TEST=DownloadProtectionServiceTest
BUG=none

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

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

[Brian Ryner, 2011-10-19 03:47:42]

Note: I think the Windows trybot run failed because it didn't know how to patch
in the binary files I'm adding.  The tests pass locally, I'm not sure if there's
a better way to run this on the trybot?

[mattm, 2011-10-19 04:08:23]

On 2011/10/19 03:47:42, Brian Ryner wrote:
> Note: I think the Windows trybot run failed because it didn't know how to
patch
> in the binary files I'm adding.  The tests pass locally, I'm not sure if
there's
> a better way to run this on the trybot?

The only way is two make two CLs, the first which only adds the binary files. 
Once that is committed you can run try jobs on the 2nd CL which contains all the
code changes (and keep in mind the try -r flag to specify a revision to try at,
if you don't wait for lkgr to update after committing the 1st CL).

[Brian Ryner, 2011-10-19 04:19:04]

Would you recommend that approach here, or do you think it's ok given that the
tests pass locally?

On 2011/10/19 04:08:23, mattm wrote:
> On 2011/10/19 03:47:42, Brian Ryner wrote:
> > Note: I think the Windows trybot run failed because it didn't know how to
> patch
> > in the binary files I'm adding.  The tests pass locally, I'm not sure if
> there's
> > a better way to run this on the trybot?
> 
> The only way is two make two CLs, the first which only adds the binary files. 
> Once that is committed you can run try jobs on the 2nd CL which contains all
the
> code changes (and keep in mind the try -r flag to specify a revision to try
at,
> if you don't wait for lkgr to update after committing the 1st CL).

[mattm, 2011-10-19 04:25:13]

On 2011/10/19 04:19:04, Brian Ryner wrote:
> Would you recommend that approach here, or do you think it's ok given that the
> tests pass locally?
> 
> On 2011/10/19 04:08:23, mattm wrote:
It's always nice to have green try runs when committing, so I'd say yes (or
definitely yes, if you want to use the commit queue :)

> > On 2011/10/19 03:47:42, Brian Ryner wrote:
> > > Note: I think the Windows trybot run failed because it didn't know how to
> > patch
> > > in the binary files I'm adding.  The tests pass locally, I'm not sure if
> > there's
> > > a better way to run this on the trybot?
> > 
> > The only way is two make two CLs, the first which only adds the binary
files. 
> > Once that is committed you can run try jobs on the 2nd CL which contains all
> the
> > code changes (and keep in mind the try -r flag to specify a revision to try
> at,
> > if you don't wait for lkgr to update after committing the 1st CL).

[noelutz, 2011-10-19 16:12:49]

LG.  I just have some minor comments.

Personally I would like to get this CL submitted as soon as possible because I'm
blocking on it.  I would prefer if you submitted this CL manually without
worrying too much about the try bots.  We can always revert if there is an
issue.

thanks,
noé.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:229: // Begin the
safe browsing download protection check.
Should you also add '#if defined(ENABLE_SAFE_BROWSING)' here?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:242:
base::Unretained(this),
Not sure you need base::Unretained because this is ref-counted.  Wouldn't it be
safer not to use it?  In particular, what if this goes away before the callback
is called?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:28:
FilePath::StringType extension = file.Extension();
Maybe wrap this in StringToLowerASCII to make sure the comparison below does the
right thing?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:178:
RecordStats(REASON_SB_DISABLED);
Mhh.  We might want to treat those two cases separately.  If enabled_ is false
we might still want to call the callback because the download manager might be
waiting for a response?

[Brian Ryner, 2011-10-19 17:41:39]

I'll  just split out the test data into a separate CL -- it won't take long and
that way we'll be reasonably sure this won't hold the tree closed for everyone.

[Brian Ryner, 2011-10-19 18:06:58]

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:229: // Begin the
safe browsing download protection check.
On 2011/10/19 16:12:49, noelutz wrote:
> Should you also add '#if defined(ENABLE_SAFE_BROWSING)' here?

Good catch, done.  Will also make sure to run this on the ChromeOS trybot since
that will have SB disabled.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/download/chrome_...
chrome/browser/download/chrome_download_manager_delegate.cc:242:
base::Unretained(this),
I think you're right -- changed.  I'm not sure about all of the other existing
call sites in this file that use Unretained.

On 2011/10/19 16:12:49, noelutz wrote:
> Not sure you need base::Unretained because this is ref-counted.  Wouldn't it
be
> safer not to use it?  In particular, what if this goes away before the
callback
> is called?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:28:
FilePath::StringType extension = file.Extension();
On 2011/10/19 16:12:49, noelutz wrote:
> Maybe wrap this in StringToLowerASCII to make sure the comparison below does
the
> right thing?

I ended up replacing this with calls to FilePath::MatchesExtension(), which is
case insensitive.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:178:
RecordStats(REASON_SB_DISABLED);
On 2011/10/19 16:12:49, noelutz wrote:
> Mhh.  We might want to treat those two cases separately.  If enabled_ is false
> we might still want to call the callback because the download manager might be
> waiting for a response?

Is theree a reason we specifically want to avoid calling the callback is
enabled_ is true but sb_service_ is null?  If not, it's easiest to just change
the behavior for both cases.

[noelutz, 2011-10-19 18:41:14]

I don't see your previous changes.

noé.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:178:
RecordStats(REASON_SB_DISABLED);
On 2011/10/19 18:06:59, Brian Ryner wrote:
> On 2011/10/19 16:12:49, noelutz wrote:
> > Mhh.  We might want to treat those two cases separately.  If enabled_ is
false
> > we might still want to call the callback because the download manager might
be
> > waiting for a response?
> 
> Is theree a reason we specifically want to avoid calling the callback is
> enabled_ is true but sb_service_ is null?  If not, it's easiest to just change
> the behavior for both cases.

Good point.  We can always call the callback with SAFE here.

[mattm, 2011-10-19 18:54:12]

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:27: bool
IsBinaryFile(const FilePath& file) {
"Binary" doesn't seem like quite the right term.. not coming up with any
brilliant alternatives though

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:41: const
base::WeakPtr<SafeBrowsingService>& sb_service,
Hmm, still not sure about these WeakPtr's but I'm not sure this is safe. 
weak_ptr.h says a WeakPtr should only be derefed on the thread that created the
WeakPtr, which I think is UI for this, but it's derefed on IO thread later on.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:156:
UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedBinaryDownload", 1);
Could use UMA_HISTOGRAM_BOOLEAN to get them both in the same histogram.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:1371:
switches::kEnableImprovedDownloadProtection));
So this isn't aiming to get histograms from users in the wild at this point?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:54: public
base::SupportsWeakPtr<SafeBrowsingService>,
Are you aiming to make SafeBrowsingService not refcounted in the future?  Should
add a comment why this is both refcounted and SupportsWeakPtr.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/si...
File chrome/browser/safe_browsing/signature_util_win_unittest.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/si...
chrome/browser/safe_browsing/signature_util_win_unittest.cc:26:
testdata_path.Append(L"unsigned.exe")));
maybe test of non-existant file too?

[Brian Ryner, 2011-10-19 19:20:11]

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:27: bool
IsBinaryFile(const FilePath& file) {
On 2011/10/19 18:54:12, mattm wrote:
> "Binary" doesn't seem like quite the right term.. not coming up with any
> brilliant alternatives though

I could use "executable" if you think that's better.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:41: const
base::WeakPtr<SafeBrowsingService>& sb_service,
On 2011/10/19 18:54:12, mattm wrote:
> Hmm, still not sure about these WeakPtr's but I'm not sure this is safe. 
> weak_ptr.h says a WeakPtr should only be derefed on the thread that created
the
> WeakPtr, which I think is UI for this, but it's derefed on IO thread later on.

Hm, good point.  Maybe we need to AddRef the SafeBrowsingService during the time
we're executing on the IO thread?  See my comment below about the rationale for
the weak pointer.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:156:
UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedBinaryDownload", 1);
On 2011/10/19 18:54:12, mattm wrote:
> Could use UMA_HISTOGRAM_BOOLEAN to get them both in the same histogram.

Done.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:1371:
switches::kEnableImprovedDownloadProtection));
On 2011/10/19 18:54:12, mattm wrote:
> So this isn't aiming to get histograms from users in the wild at this point?

It will, because we only check the "enabled" state after collecting the
signed-binary histogram.

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:54: public
base::SupportsWeakPtr<SafeBrowsingService>,
On 2011/10/19 18:54:12, mattm wrote:
> Are you aiming to make SafeBrowsingService not refcounted in the future? 
Should
> add a comment why this is both refcounted and SupportsWeakPtr.

So, the issue I'm trying to address here is that currently, there's a circular
reference: SafeBrowsingService has a reference to DownloadProtectionService, and
DownloadProtectionService has a reference to SafeBrowsingService.

The cycle can be broken by calling SafeBrowsingService::ShutDown, but that isn't
something that is consistently called in various tests that use
SafeBrowsingSservice.  It seemed like it might be less error-prone to have a
strong reference going in only one direction.

Does that make sense, or do you think there's a better way to address this?

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/si...
File chrome/browser/safe_browsing/signature_util_win_unittest.cc (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/si...
chrome/browser/safe_browsing/signature_util_win_unittest.cc:26:
testdata_path.Append(L"unsigned.exe")));
On 2011/10/19 18:54:12, mattm wrote:
> maybe test of non-existant file too?

Done.

[mattm, 2011-10-19 20:04:10]

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:54: public
base::SupportsWeakPtr<SafeBrowsingService>,
Still thinking this through, but I think we could:

Make DownloadProtectionService not refcounted at all, but instead
SupportsWeakPtr.  SafeBrowsingService would own the DownloadProtectionService,
and thus we could be sure that the SBService outlives the DPService.  Any time
we post a task for the DPService we'd use a WeakPtr, and those would be safe
since they would each be created only as needed rather than having a long-lived
one like this that would be accessed on multiple threads.

Does that sound sane?

On 2011/10/19 19:20:11, Brian Ryner wrote:
> On 2011/10/19 18:54:12, mattm wrote:
> > Are you aiming to make SafeBrowsingService not refcounted in the future? 
> Should
> > add a comment why this is both refcounted and SupportsWeakPtr.
> 
> So, the issue I'm trying to address here is that currently, there's a circular
> reference: SafeBrowsingService has a reference to DownloadProtectionService,
and
> DownloadProtectionService has a reference to SafeBrowsingService.
> 
> The cycle can be broken by calling SafeBrowsingService::ShutDown, but that
isn't
> something that is consistently called in various tests that use
> SafeBrowsingSservice.  It seemed like it might be less error-prone to have a
> strong reference going in only one direction.
> 
> Does that make sense, or do you think there's a better way to address this?
>

[mattm, 2011-10-19 20:06:07]

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:54: public
base::SupportsWeakPtr<SafeBrowsingService>,
Oh, the one thing I forgot to mention: Since we are sure about the lifetime
being longer, then DownloadProtectionService would hold a bare pointer to
SafeBrowsingService instead of a refptr, breaking the cycle.

On 2011/10/19 20:04:10, mattm wrote:
> Still thinking this through, but I think we could:
> 
> Make DownloadProtectionService not refcounted at all, but instead
> SupportsWeakPtr.  SafeBrowsingService would own the DownloadProtectionService,
> and thus we could be sure that the SBService outlives the DPService.  Any time
> we post a task for the DPService we'd use a WeakPtr, and those would be safe
> since they would each be created only as needed rather than having a
long-lived
> one like this that would be accessed on multiple threads.
> 
> Does that sound sane?
> 
> On 2011/10/19 19:20:11, Brian Ryner wrote:
> > On 2011/10/19 18:54:12, mattm wrote:
> > > Are you aiming to make SafeBrowsingService not refcounted in the future? 
> > Should
> > > add a comment why this is both refcounted and SupportsWeakPtr.
> > 
> > So, the issue I'm trying to address here is that currently, there's a
circular
> > reference: SafeBrowsingService has a reference to DownloadProtectionService,
> and
> > DownloadProtectionService has a reference to SafeBrowsingService.
> > 
> > The cycle can be broken by calling SafeBrowsingService::ShutDown, but that
> isn't
> > something that is consistently called in various tests that use
> > SafeBrowsingSservice.  It seemed like it might be less error-prone to have a
> > strong reference going in only one direction.
> > 
> > Does that make sense, or do you think there's a better way to address this?
> > 
>

[Brian Ryner, 2011-10-19 20:53:52]

How would this work when we post a task for the DPService to do some work on the
IO thread?  As you mentioned, you're not supposed to deref a weak pointer on a
different thread than it was created on.


On 2011/10/19 20:04:10, mattm wrote:
>
http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
> File chrome/browser/safe_browsing/safe_browsing_service.h (right):
> 
>
http://codereview.chromium.org/8345033/diff/1/chrome/browser/safe_browsing/sa...
> chrome/browser/safe_browsing/safe_browsing_service.h:54: public
> base::SupportsWeakPtr<SafeBrowsingService>,
> Still thinking this through, but I think we could:
> 
> Make DownloadProtectionService not refcounted at all, but instead
> SupportsWeakPtr.  SafeBrowsingService would own the DownloadProtectionService,
> and thus we could be sure that the SBService outlives the DPService.  Any time
> we post a task for the DPService we'd use a WeakPtr, and those would be safe
> since they would each be created only as needed rather than having a
long-lived
> one like this that would be accessed on multiple threads.
> 
> Does that sound sane?
> 
> On 2011/10/19 19:20:11, Brian Ryner wrote:
> > On 2011/10/19 18:54:12, mattm wrote:
> > > Are you aiming to make SafeBrowsingService not refcounted in the future? 
> > Should
> > > add a comment why this is both refcounted and SupportsWeakPtr.
> > 
> > So, the issue I'm trying to address here is that currently, there's a
circular
> > reference: SafeBrowsingService has a reference to DownloadProtectionService,
> and
> > DownloadProtectionService has a reference to SafeBrowsingService.
> > 
> > The cycle can be broken by calling SafeBrowsingService::ShutDown, but that
> isn't
> > something that is consistently called in various tests that use
> > SafeBrowsingSservice.  It seemed like it might be less error-prone to have a
> > strong reference going in only one direction.
> > 
> > Does that make sense, or do you think there's a better way to address this?
> >

[mattm, 2011-10-19 22:44:37]

Hi Will,
I'm sure you're dying to get more refcounting / weakptr type questions :)

See comments #11-13, but basically we want want to break a refcounting cycle but
I'm not completely sure about weakptr thread safety.  I thought I've seen other
examples of weakptrs being used with PostTask.  Or if you have any alternate
ideas about how to break the cycle that would be good too.  (feel free to ping
me if you have any questions and don't want to read all the code.)

[Brian Ryner, 2011-10-20 05:59:12]

Going on the theory that using weakptr this way is unsafe (as the comments would
lead you to believe), I can see a couple of other ways to fix this:

- (mentioned above) AddRef the SBService on the UI thread before posting the
task to the IO thread.  Pass a raw pointer to the IO thread task.  Release the
SBService on the UI thread after the IO thread task has finished.

- Do something like ShouldClassifyUrlRequest in client_side_detection_host.cc,
where the code that runs in the IO thread is factored out into a separate,
refcounted object that has a (raw) pointer to the DPService.  The DPService
would keep track of this object so that it can be destroyed before the DPService
goes away.

Thoughts?

On 2011/10/19 22:44:37, mattm wrote:
> Hi Will,
> I'm sure you're dying to get more refcounting / weakptr type questions :)
> 
> See comments #11-13, but basically we want want to break a refcounting cycle
but
> I'm not completely sure about weakptr thread safety.  I thought I've seen
other
> examples of weakptrs being used with PostTask.  Or if you have any alternate
> ideas about how to break the cycle that would be good too.  (feel free to ping
> me if you have any questions and don't want to read all the code.)

[willchan no longer on Chromium, 2011-10-20 14:35:47]

From my brief glance, I don't think refcounting or WeakPtr are necessary. It
seems like SBService owns the DPService. Put the DPService in a scoped_ptr.
In SBService::Shutdown(), release the scoped_ptr and post a
DeleteTask<DPService> onto the IO thread. And I think SBService has some
function that observes pref changes. These pref changes should only result
in calling the DPService if it is non-NULL.

That's from my brief glance. And yeah, WeakPtr is not threadsafe, except for
destruction. It's not safe to deref on a different thread, but it's OK for
it to be destroyed on a different thread.

On Wed, Oct 19, 2011 at 10:59 PM, <bryner@chromium.org> wrote:

> Going on the theory that using weakptr this way is unsafe (as the comments
> would
> lead you to believe), I can see a couple of other ways to fix this:
>
> - (mentioned above) AddRef the SBService on the UI thread before posting
> the
> task to the IO thread.  Pass a raw pointer to the IO thread task.  Release
> the
> SBService on the UI thread after the IO thread task has finished.
>
> - Do something like ShouldClassifyUrlRequest in
> client_side_detection_host.cc,
> where the code that runs in the IO thread is factored out into a separate,
> refcounted object that has a (raw) pointer to the DPService.  The DPService
> would keep track of this object so that it can be destroyed before the
> DPService
> goes away.
>
> Thoughts?
>
>
> On 2011/10/19 22:44:37, mattm wrote:
>
>> Hi Will,
>> I'm sure you're dying to get more refcounting / weakptr type questions :)
>>
>
>  See comments #11-13, but basically we want want to break a refcounting
>> cycle
>>
> but
>
>> I'm not completely sure about weakptr thread safety.  I thought I've seen
>>
> other
>
>> examples of weakptrs being used with PostTask.  Or if you have any
>> alternate
>> ideas about how to break the cycle that would be good too.  (feel free to
>> ping
>> me if you have any questions and don't want to read all the code.)
>>
>
>
>
>
http://codereview.chromium.**org/8345033/<http://codereview.chromium.org/8345...
>

[Brian Ryner, 2011-10-20 17:23:25]

Hi Wlll,

I wanted to clarify the threading a bit here since I think it affects how we do
refcounting.

DPService mainly lives on the UI thread.  When CheckClientDownload is called, it
does some work on the UI thread and then posts a task to itself on the IO thread
to do some additional work.  This IO-thread task (StartCheckClientDownload)
accesses sb_service_.  When the IO thread task is done, it posts another task to
the UI thread (EndCheckClientDownload) to finish up.

So the case that we were concerned with here is where the SBService is destroyed
while the DPService is running (or has posted a task to run) on the IO thread. 
If it deletes the DPService at that point, we will crash.  This is the reason
that we'd originally made DPService refcounted.

Note that we also need to make sure that the DPService IO thread task fails
gracefully if the SBService goes away, so that would seem to mean:
  a) holding a reference to the SBService so that it can't go away
  b) using a weak ptr which would become null (but this is not thread safe, so
cross this option off)
  c) making sure SBService tells DPService it's going away (nulling out its
sb_service_ pointer).  But, this would need to happen in a thread-safe manner.


On 2011/10/20 14:35:47, willchan wrote:
> From my brief glance, I don't think refcounting or WeakPtr are necessary. It
> seems like SBService owns the DPService. Put the DPService in a scoped_ptr.
> In SBService::Shutdown(), release the scoped_ptr and post a
> DeleteTask<DPService> onto the IO thread. And I think SBService has some
> function that observes pref changes. These pref changes should only result
> in calling the DPService if it is non-NULL.
> 
> That's from my brief glance. And yeah, WeakPtr is not threadsafe, except for
> destruction. It's not safe to deref on a different thread, but it's OK for
> it to be destroyed on a different thread.
> 
> On Wed, Oct 19, 2011 at 10:59 PM, <mailto:bryner@chromium.org> wrote:
> 
> > Going on the theory that using weakptr this way is unsafe (as the comments
> > would
> > lead you to believe), I can see a couple of other ways to fix this:
> >
> > - (mentioned above) AddRef the SBService on the UI thread before posting
> > the
> > task to the IO thread.  Pass a raw pointer to the IO thread task.  Release
> > the
> > SBService on the UI thread after the IO thread task has finished.
> >
> > - Do something like ShouldClassifyUrlRequest in
> > client_side_detection_host.cc,
> > where the code that runs in the IO thread is factored out into a separate,
> > refcounted object that has a (raw) pointer to the DPService.  The DPService
> > would keep track of this object so that it can be destroyed before the
> > DPService
> > goes away.
> >
> > Thoughts?
> >
> >
> > On 2011/10/19 22:44:37, mattm wrote:
> >
> >> Hi Will,
> >> I'm sure you're dying to get more refcounting / weakptr type questions :)
> >>
> >
> >  See comments #11-13, but basically we want want to break a refcounting
> >> cycle
> >>
> > but
> >
> >> I'm not completely sure about weakptr thread safety.  I thought I've seen
> >>
> > other
> >
> >> examples of weakptrs being used with PostTask.  Or if you have any
> >> alternate
> >> ideas about how to break the cycle that would be good too.  (feel free to
> >> ping
> >> me if you have any questions and don't want to read all the code.)
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8345033/%3Chttp://codereview.chromium.org/83...>
> >

[willchan no longer on Chromium, 2011-10-21 00:36:23]

On Thu, Oct 20, 2011 at 10:23 AM, <bryner@chromium.org> wrote:

> Hi Wlll,
>
> I wanted to clarify the threading a bit here since I think it affects how
> we do
> refcounting.
>
> DPService mainly lives on the UI thread.  When CheckClientDownload is
> called, it
> does some work on the UI thread and then posts a task to itself on the IO
> thread
> to do some additional work.  This IO-thread task (StartCheckClientDownload)
> accesses sb_service_.  When the IO thread task is done, it posts another
> task to
> the UI thread (EndCheckClientDownload) to finish up.
>
> So the case that we were concerned with here is where the SBService is
> destroyed
> while the DPService is running (or has posted a task to run) on the IO
> thread.
> If it deletes the DPService at that point, we will crash.  This is the
> reason
> that we'd originally made DPService refcounted.
>

This is why I said that the DPService should be deleted on the IO thread.
When SBService is destroyed on the UI thread, it should do:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, new
DeleteTask(dpservice_.release()));


>
> Note that we also need to make sure that the DPService IO thread task fails
> gracefully if the SBService goes away, so that would seem to mean:
>  a) holding a reference to the SBService so that it can't go away
>  b) using a weak ptr which would become null (but this is not thread safe,
> so
> cross this option off)
>  c) making sure SBService tells DPService it's going away (nulling out its
> sb_service_ pointer).  But, this would need to happen in a thread-safe
> manner.
>
>
>
> On 2011/10/20 14:35:47, willchan wrote:
>
>>  From my brief glance, I don't think refcounting or WeakPtr are necessary.
>> It
>> seems like SBService owns the DPService. Put the DPService in a
>> scoped_ptr.
>> In SBService::Shutdown(), release the scoped_ptr and post a
>> DeleteTask<DPService> onto the IO thread. And I think SBService has some
>> function that observes pref changes. These pref changes should only result
>> in calling the DPService if it is non-NULL.
>>
>
>  That's from my brief glance. And yeah, WeakPtr is not threadsafe, except
>> for
>> destruction. It's not safe to deref on a different thread, but it's OK for
>> it to be destroyed on a different thread.
>>
>
>  On Wed, Oct 19, 2011 at 10:59 PM, <mailto:bryner@chromium.org> wrote:
>>
>
>  > Going on the theory that using weakptr this way is unsafe (as the
>> comments
>> > would
>> > lead you to believe), I can see a couple of other ways to fix this:
>> >
>> > - (mentioned above) AddRef the SBService on the UI thread before posting
>> > the
>> > task to the IO thread.  Pass a raw pointer to the IO thread task.
>>  Release
>> > the
>> > SBService on the UI thread after the IO thread task has finished.
>> >
>> > - Do something like ShouldClassifyUrlRequest in
>> > client_side_detection_host.cc,
>> > where the code that runs in the IO thread is factored out into a
>> separate,
>> > refcounted object that has a (raw) pointer to the DPService.  The
>> DPService
>> > would keep track of this object so that it can be destroyed before the
>> > DPService
>> > goes away.
>> >
>> > Thoughts?
>> >
>> >
>> > On 2011/10/19 22:44:37, mattm wrote:
>> >
>> >> Hi Will,
>> >> I'm sure you're dying to get more refcounting / weakptr type questions
>> :)
>> >>
>> >
>> >  See comments #11-13, but basically we want want to break a refcounting
>> >> cycle
>> >>
>> > but
>> >
>> >> I'm not completely sure about weakptr thread safety.  I thought I've
>> seen
>> >>
>> > other
>> >
>> >> examples of weakptrs being used with PostTask.  Or if you have any
>> >> alternate
>> >> ideas about how to break the cycle that would be good too.  (feel free
>> to
>> >> ping
>> >> me if you have any questions and don't want to read all the code.)
>> >>
>> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/8345033/%3Chttp://coderevi**
> ew.chromium.org/8345033/ <http://codereview.chromium.org/8345033/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8345033/<http://codereview.chromium.org/8345...
>

[Brian Ryner, 2011-10-21 00:58:08]

So since the SBService is destroyed on the UI thread, we'd know that no
other callers are accessing the DPService on the UI thread when this happens
(and they should not be able to access it after this, since they should only
get it through the SBService).  I think this could work.  It seems a little
odd to destroy the DPService on a different thread than it was created on,
but if this is a common pattern for this situation, I'm ok with it.

Thanks,

On Thu, Oct 20, 2011 at 5:36 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Thu, Oct 20, 2011 at 10:23 AM, <bryner@chromium.org> wrote:
>
>> Hi Wlll,
>>
>> I wanted to clarify the threading a bit here since I think it affects how
>> we do
>> refcounting.
>>
>> DPService mainly lives on the UI thread.  When CheckClientDownload is
>> called, it
>> does some work on the UI thread and then posts a task to itself on the IO
>> thread
>> to do some additional work.  This IO-thread task
>> (StartCheckClientDownload)
>> accesses sb_service_.  When the IO thread task is done, it posts another
>> task to
>> the UI thread (EndCheckClientDownload) to finish up.
>>
>> So the case that we were concerned with here is where the SBService is
>> destroyed
>> while the DPService is running (or has posted a task to run) on the IO
>> thread.
>> If it deletes the DPService at that point, we will crash.  This is the
>> reason
>> that we'd originally made DPService refcounted.
>>
>
> This is why I said that the DPService should be deleted on the IO thread.
> When SBService is destroyed on the UI thread, it should do:
> BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, new
> DeleteTask(dpservice_.release()));
>
>
>>
>> Note that we also need to make sure that the DPService IO thread task
>> fails
>> gracefully if the SBService goes away, so that would seem to mean:
>>  a) holding a reference to the SBService so that it can't go away
>>  b) using a weak ptr which would become null (but this is not thread safe,
>> so
>> cross this option off)
>>  c) making sure SBService tells DPService it's going away (nulling out its
>> sb_service_ pointer).  But, this would need to happen in a thread-safe
>> manner.
>>
>>
>>
>> On 2011/10/20 14:35:47, willchan wrote:
>>
>>>  From my brief glance, I don't think refcounting or WeakPtr are
>>> necessary. It
>>> seems like SBService owns the DPService. Put the DPService in a
>>> scoped_ptr.
>>> In SBService::Shutdown(), release the scoped_ptr and post a
>>> DeleteTask<DPService> onto the IO thread. And I think SBService has some
>>> function that observes pref changes. These pref changes should only
>>> result
>>> in calling the DPService if it is non-NULL.
>>>
>>
>>  That's from my brief glance. And yeah, WeakPtr is not threadsafe, except
>>> for
>>> destruction. It's not safe to deref on a different thread, but it's OK
>>> for
>>> it to be destroyed on a different thread.
>>>
>>
>>  On Wed, Oct 19, 2011 at 10:59 PM, <mailto:bryner@chromium.org> wrote:
>>>
>>
>>  > Going on the theory that using weakptr this way is unsafe (as the
>>> comments
>>> > would
>>> > lead you to believe), I can see a couple of other ways to fix this:
>>> >
>>> > - (mentioned above) AddRef the SBService on the UI thread before
>>> posting
>>> > the
>>> > task to the IO thread.  Pass a raw pointer to the IO thread task.
>>>  Release
>>> > the
>>> > SBService on the UI thread after the IO thread task has finished.
>>> >
>>> > - Do something like ShouldClassifyUrlRequest in
>>> > client_side_detection_host.cc,
>>> > where the code that runs in the IO thread is factored out into a
>>> separate,
>>> > refcounted object that has a (raw) pointer to the DPService.  The
>>> DPService
>>> > would keep track of this object so that it can be destroyed before the
>>> > DPService
>>> > goes away.
>>> >
>>> > Thoughts?
>>> >
>>> >
>>> > On 2011/10/19 22:44:37, mattm wrote:
>>> >
>>> >> Hi Will,
>>> >> I'm sure you're dying to get more refcounting / weakptr type questions
>>> :)
>>> >>
>>> >
>>> >  See comments #11-13, but basically we want want to break a refcounting
>>> >> cycle
>>> >>
>>> > but
>>> >
>>> >> I'm not completely sure about weakptr thread safety.  I thought I've
>>> seen
>>> >>
>>> > other
>>> >
>>> >> examples of weakptrs being used with PostTask.  Or if you have any
>>> >> alternate
>>> >> ideas about how to break the cycle that would be good too.  (feel free
>>> to
>>> >> ping
>>> >> me if you have any questions and don't want to read all the code.)
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/8345033/%3Chttp://coderevi**
>> ew.chromium.org/8345033/ <http://codereview.chromium.org/8345033/>>
>>
>>> >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8345033/<http://codereview.chromium.org/8345...
>>
>
>


-- 
-Brian

[willchan no longer on Chromium, 2011-10-21 01:08:51]

On Thu, Oct 20, 2011 at 5:58 PM, Brian Ryner <bryner@chromium.org> wrote:

> So since the SBService is destroyed on the UI thread, we'd know that no
> other callers are accessing the DPService on the UI thread when this happens
> (and they should not be able to access it after this, since they should only
> get it through the SBService).  I think this could work.  It seems a little
> odd to destroy the DPService on a different thread than it was created on,
> but if this is a common pattern for this situation, I'm ok with it.


It's already possible for it to get deleted on a different thread than it
was created on, right? You just never think about it since it's hidden
behind a RefCountedThreadSafe. I think explicit deletion is cleaner because
the behavior is now very well-defined.


>
> Thanks,
>
> On Thu, Oct 20, 2011 at 5:36 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Thu, Oct 20, 2011 at 10:23 AM, <bryner@chromium.org> wrote:
>>
>>> Hi Wlll,
>>>
>>> I wanted to clarify the threading a bit here since I think it affects how
>>> we do
>>> refcounting.
>>>
>>> DPService mainly lives on the UI thread.  When CheckClientDownload is
>>> called, it
>>> does some work on the UI thread and then posts a task to itself on the IO
>>> thread
>>> to do some additional work.  This IO-thread task
>>> (StartCheckClientDownload)
>>> accesses sb_service_.  When the IO thread task is done, it posts another
>>> task to
>>> the UI thread (EndCheckClientDownload) to finish up.
>>>
>>> So the case that we were concerned with here is where the SBService is
>>> destroyed
>>> while the DPService is running (or has posted a task to run) on the IO
>>> thread.
>>> If it deletes the DPService at that point, we will crash.  This is the
>>> reason
>>> that we'd originally made DPService refcounted.
>>>
>>
>> This is why I said that the DPService should be deleted on the IO thread.
>> When SBService is destroyed on the UI thread, it should do:
>> BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, new
>> DeleteTask(dpservice_.release()));
>>
>>
>>>
>>> Note that we also need to make sure that the DPService IO thread task
>>> fails
>>> gracefully if the SBService goes away, so that would seem to mean:
>>>  a) holding a reference to the SBService so that it can't go away
>>>  b) using a weak ptr which would become null (but this is not thread
>>> safe, so
>>> cross this option off)
>>>  c) making sure SBService tells DPService it's going away (nulling out
>>> its
>>> sb_service_ pointer).  But, this would need to happen in a thread-safe
>>> manner.
>>>
>>>
>>>
>>> On 2011/10/20 14:35:47, willchan wrote:
>>>
>>>>  From my brief glance, I don't think refcounting or WeakPtr are
>>>> necessary. It
>>>> seems like SBService owns the DPService. Put the DPService in a
>>>> scoped_ptr.
>>>> In SBService::Shutdown(), release the scoped_ptr and post a
>>>> DeleteTask<DPService> onto the IO thread. And I think SBService has some
>>>> function that observes pref changes. These pref changes should only
>>>> result
>>>> in calling the DPService if it is non-NULL.
>>>>
>>>
>>>  That's from my brief glance. And yeah, WeakPtr is not threadsafe, except
>>>> for
>>>> destruction. It's not safe to deref on a different thread, but it's OK
>>>> for
>>>> it to be destroyed on a different thread.
>>>>
>>>
>>>  On Wed, Oct 19, 2011 at 10:59 PM, <mailto:bryner@chromium.org> wrote:
>>>>
>>>
>>>  > Going on the theory that using weakptr this way is unsafe (as the
>>>> comments
>>>> > would
>>>> > lead you to believe), I can see a couple of other ways to fix this:
>>>> >
>>>> > - (mentioned above) AddRef the SBService on the UI thread before
>>>> posting
>>>> > the
>>>> > task to the IO thread.  Pass a raw pointer to the IO thread task.
>>>>  Release
>>>> > the
>>>> > SBService on the UI thread after the IO thread task has finished.
>>>> >
>>>> > - Do something like ShouldClassifyUrlRequest in
>>>> > client_side_detection_host.cc,
>>>> > where the code that runs in the IO thread is factored out into a
>>>> separate,
>>>> > refcounted object that has a (raw) pointer to the DPService.  The
>>>> DPService
>>>> > would keep track of this object so that it can be destroyed before the
>>>> > DPService
>>>> > goes away.
>>>> >
>>>> > Thoughts?
>>>> >
>>>> >
>>>> > On 2011/10/19 22:44:37, mattm wrote:
>>>> >
>>>> >> Hi Will,
>>>> >> I'm sure you're dying to get more refcounting / weakptr type
>>>> questions :)
>>>> >>
>>>> >
>>>> >  See comments #11-13, but basically we want want to break a
>>>> refcounting
>>>> >> cycle
>>>> >>
>>>> > but
>>>> >
>>>> >> I'm not completely sure about weakptr thread safety.  I thought I've
>>>> seen
>>>> >>
>>>> > other
>>>> >
>>>> >> examples of weakptrs being used with PostTask.  Or if you have any
>>>> >> alternate
>>>> >> ideas about how to break the cycle that would be good too.  (feel
>>>> free to
>>>> >> ping
>>>> >> me if you have any questions and don't want to read all the code.)
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>> http://codereview.chromium.****org/8345033/%3Chttp://coderevi**
>>> ew.chromium.org/8345033/ <http://codereview.chromium.org/8345033/>>
>>>
>>>> >
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/8345033/<http://codereview.chromium.org/8345...
>>>
>>
>>
>
>
> --
> -Brian
>

[Brian Ryner, 2011-10-21 01:15:16]

Great point.  Will give this a shot.

On Thu, Oct 20, 2011 at 6:08 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Thu, Oct 20, 2011 at 5:58 PM, Brian Ryner <bryner@chromium.org> wrote:
>
>> So since the SBService is destroyed on the UI thread, we'd know that no
>> other callers are accessing the DPService on the UI thread when this happens
>> (and they should not be able to access it after this, since they should only
>> get it through the SBService).  I think this could work.  It seems a little
>> odd to destroy the DPService on a different thread than it was created on,
>> but if this is a common pattern for this situation, I'm ok with it.
>
>
> It's already possible for it to get deleted on a different thread than it
> was created on, right? You just never think about it since it's hidden
> behind a RefCountedThreadSafe. I think explicit deletion is cleaner because
> the behavior is now very well-defined.
>
>
>>
>> Thanks,
>>
>> On Thu, Oct 20, 2011 at 5:36 PM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> On Thu, Oct 20, 2011 at 10:23 AM, <bryner@chromium.org> wrote:
>>>
>>>> Hi Wlll,
>>>>
>>>> I wanted to clarify the threading a bit here since I think it affects
>>>> how we do
>>>> refcounting.
>>>>
>>>> DPService mainly lives on the UI thread.  When CheckClientDownload is
>>>> called, it
>>>> does some work on the UI thread and then posts a task to itself on the
>>>> IO thread
>>>> to do some additional work.  This IO-thread task
>>>> (StartCheckClientDownload)
>>>> accesses sb_service_.  When the IO thread task is done, it posts another
>>>> task to
>>>> the UI thread (EndCheckClientDownload) to finish up.
>>>>
>>>> So the case that we were concerned with here is where the SBService is
>>>> destroyed
>>>> while the DPService is running (or has posted a task to run) on the IO
>>>> thread.
>>>> If it deletes the DPService at that point, we will crash.  This is the
>>>> reason
>>>> that we'd originally made DPService refcounted.
>>>>
>>>
>>> This is why I said that the DPService should be deleted on the IO thread.
>>> When SBService is destroyed on the UI thread, it should do:
>>> BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, new
>>> DeleteTask(dpservice_.release()));
>>>
>>>
>>>>
>>>> Note that we also need to make sure that the DPService IO thread task
>>>> fails
>>>> gracefully if the SBService goes away, so that would seem to mean:
>>>>  a) holding a reference to the SBService so that it can't go away
>>>>  b) using a weak ptr which would become null (but this is not thread
>>>> safe, so
>>>> cross this option off)
>>>>  c) making sure SBService tells DPService it's going away (nulling out
>>>> its
>>>> sb_service_ pointer).  But, this would need to happen in a thread-safe
>>>> manner.
>>>>
>>>>
>>>>
>>>> On 2011/10/20 14:35:47, willchan wrote:
>>>>
>>>>>  From my brief glance, I don't think refcounting or WeakPtr are
>>>>> necessary. It
>>>>> seems like SBService owns the DPService. Put the DPService in a
>>>>> scoped_ptr.
>>>>> In SBService::Shutdown(), release the scoped_ptr and post a
>>>>> DeleteTask<DPService> onto the IO thread. And I think SBService has
>>>>> some
>>>>> function that observes pref changes. These pref changes should only
>>>>> result
>>>>> in calling the DPService if it is non-NULL.
>>>>>
>>>>
>>>>  That's from my brief glance. And yeah, WeakPtr is not threadsafe,
>>>>> except for
>>>>> destruction. It's not safe to deref on a different thread, but it's OK
>>>>> for
>>>>> it to be destroyed on a different thread.
>>>>>
>>>>
>>>>  On Wed, Oct 19, 2011 at 10:59 PM, <mailto:bryner@chromium.org> wrote:
>>>>>
>>>>
>>>>  > Going on the theory that using weakptr this way is unsafe (as the
>>>>> comments
>>>>> > would
>>>>> > lead you to believe), I can see a couple of other ways to fix this:
>>>>> >
>>>>> > - (mentioned above) AddRef the SBService on the UI thread before
>>>>> posting
>>>>> > the
>>>>> > task to the IO thread.  Pass a raw pointer to the IO thread task.
>>>>>  Release
>>>>> > the
>>>>> > SBService on the UI thread after the IO thread task has finished.
>>>>> >
>>>>> > - Do something like ShouldClassifyUrlRequest in
>>>>> > client_side_detection_host.cc,
>>>>> > where the code that runs in the IO thread is factored out into a
>>>>> separate,
>>>>> > refcounted object that has a (raw) pointer to the DPService.  The
>>>>> DPService
>>>>> > would keep track of this object so that it can be destroyed before
>>>>> the
>>>>> > DPService
>>>>> > goes away.
>>>>> >
>>>>> > Thoughts?
>>>>> >
>>>>> >
>>>>> > On 2011/10/19 22:44:37, mattm wrote:
>>>>> >
>>>>> >> Hi Will,
>>>>> >> I'm sure you're dying to get more refcounting / weakptr type
>>>>> questions :)
>>>>> >>
>>>>> >
>>>>> >  See comments #11-13, but basically we want want to break a
>>>>> refcounting
>>>>> >> cycle
>>>>> >>
>>>>> > but
>>>>> >
>>>>> >> I'm not completely sure about weakptr thread safety.  I thought I've
>>>>> seen
>>>>> >>
>>>>> > other
>>>>> >
>>>>> >> examples of weakptrs being used with PostTask.  Or if you have any
>>>>> >> alternate
>>>>> >> ideas about how to break the cycle that would be good too.  (feel
>>>>> free to
>>>>> >> ping
>>>>> >> me if you have any questions and don't want to read all the code.)
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>> http://codereview.chromium.****org/8345033/%3Chttp://coderevi**
>>>> ew.chromium.org/8345033/ <http://codereview.chromium.org/8345033/>>
>>>>
>>>>> >
>>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/8345033/<http://codereview.chromium.org/8345...
>>>>
>>>
>>>
>>
>>
>> --
>> -Brian
>>
>
>


-- 
-Brian

[Brian Ryner, 2011-10-22 00:30:43]

Ok, updated based on all of the above comments.  Please take another look.

[noelutz, 2011-10-22 00:41:46]

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:59:
base::Unretained(this), enabled));
I just want to make sure I follow the code here.  We're assuming that we will
only delete this service object once the IO thread is going away and no-one will
flush the IO message loop?

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:147:
base::Unretained(this), info, enabled_, callback));
I think there is a race condition here.  You access enabled_ on the UI thread
but it's written and set on the IO thread.  Also, you're not really using
pingback_enabled anywhere as far as I can tell.  Could you just read enabled_ in
StartCheckClientDownload which runs on the IO thread?

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:188:
base::Unretained(this), SAFE, reason, callback));
Will this always work?  What if the destructor is called before
EndCheckClientDownload?

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:230:
base::Unretained(this), SAFE, reason, callback));
same question as above.

[Randy Smith (Not in Mondays), 2011-10-22 17:08:07]

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:228: 
Can we localize the safebrowsing code in this file to a single callout in
ShouldStartDownload (or, alternatively, here)?  I'd like to minimize how
interwoven the download code and the safebrowing code are.  In my ideal world,
the only line in this file would be a call to a safebrowing function containing
a pointer to the download item and a callback to be called when the safebrowsing
checks were done.  Can we move in that direction?

My apologies for not weighing in earlier--this is partially based on my
conversation with Noe' yesterday, which got me paying more attention to this
patch.

[Brian Ryner, 2011-10-22 19:47:31]

rdsmith: So basically that would mean having the DownloadProtectionService API
deal with DownloadItem directly?  I think that should be doable, unless anyone
sees a problem with the dependencies.

Also, fwiw this cannot happen in ShouldStartDownload -- these checks need the
downloaded content (signing info, hashes, etc) so they need to happen after the
download has finished.  The only thing that can happen in ShouldStartDownload is
the bad-download-URL check.  I think we probably need call-outs to SafeBrowsing
in both locations.

Noe, you have some good points about the threading / reference counting.  I'm
working on a fix for this.

On 2011/10/22 17:08:07, rdsmith wrote:
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> chrome/browser/download/chrome_download_manager_delegate.cc:228: 
> Can we localize the safebrowsing code in this file to a single callout in
> ShouldStartDownload (or, alternatively, here)?  I'd like to minimize how
> interwoven the download code and the safebrowing code are.  In my ideal world,
> the only line in this file would be a call to a safebrowing function
containing
> a pointer to the download item and a callback to be called when the
safebrowsing
> checks were done.  Can we move in that direction?
> 
> My apologies for not weighing in earlier--this is partially based on my
> conversation with Noe' yesterday, which got me paying more attention to this
> patch.

[noelutz, 2011-10-24 17:40:25]

On Sat, Oct 22, 2011 at 12:47 PM,  <bryner@chromium.org> wrote:
> rdsmith: So basically that would mean having the DownloadProtectionService
> API
> deal with DownloadItem directly?  I think that should be doable, unless
> anyone
> sees a problem with the dependencies.

That sounds reasonable to me too.

> Also, fwiw this cannot happen in ShouldStartDownload -- these checks need
> the
> downloaded content (signing info, hashes, etc) so they need to happen after
> the
> download has finished.  The only thing that can happen in
> ShouldStartDownload is
> the bad-download-URL check.  I think we probably need call-outs to
> SafeBrowsing
> in both locations.

Randy and I figured that eventually there should be exactly two calls
to SB classes in this code:

1) right after the download starts to match the URL against the bad
binary URL list (existing code).

2) once the download is finished to check the download hash and
contact the download protection servers (new code).

Randy: in the short term it would be nice if we could add a third call
that Brian is adding here.  Once 2) is implemented we can remove the
second existing call into SB which matches the binary hash against the
blacklist of known bad hashes.  If you prefer to have that second
existing call into SB removed sooner rather than later I can do that
right after Brian's CL.  How does this proposal sound to you?  The
reason I advocate for submitting Brian's CL sooner rather than later
is because other changes are blocking on this CL and we would like to
get some UMA stats back asap.

Thanks,
noe.

> Noe, you have some good points about the threading / reference counting.
>  I'm
> working on a fix for this.
>
> On 2011/10/22 17:08:07, rdsmith wrote:
>
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
>>
>> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
>
>
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
>>
>> chrome/browser/download/chrome_download_manager_delegate.cc:228:
>> Can we localize the safebrowsing code in this file to a single callout in
>> ShouldStartDownload (or, alternatively, here)?  I'd like to minimize how
>> interwoven the download code and the safebrowing code are.  In my ideal
>> world,
>> the only line in this file would be a call to a safebrowing function
>
> containing
>>
>> a pointer to the download item and a callback to be called when the
>
> safebrowsing
>>
>> checks were done.  Can we move in that direction?
>
>> My apologies for not weighing in earlier--this is partially based on my
>> conversation with Noe' yesterday, which got me paying more attention to
>> this
>> patch.
>
>
>
> http://codereview.chromium.org/8345033/
>



-- 
-|||--

[Randy Smith (Not in Mondays), 2011-10-24 18:12:24]

On 2011/10/22 19:47:31, Brian Ryner wrote:
> rdsmith: So basically that would mean having the DownloadProtectionService API
> deal with DownloadItem directly?  I think that should be doable, unless anyone
> sees a problem with the dependencies.

Awesome; thank you.  I'm slowly trying to increase the conceptual
consistency--and thus the readability--of the download system, and that would
help.  If it messes up the safebrowsing system and is thus a zero-sum game, we
should rethink.  But my guess is that that won't make the safebrowsing code any
more messy, and it'll make the download code neater.

> Also, fwiw this cannot happen in ShouldStartDownload -- these checks need the
> downloaded content (signing info, hashes, etc) so they need to happen after
the
> download has finished.  The only thing that can happen in ShouldStartDownload
is
> the bad-download-URL check.  I think we probably need call-outs to
SafeBrowsing
> in both locations.

Ah, sorry, I was confused about when UpdatePathForItemInPersistentStore was
called.  But I still think that hooking in UpdatePathForItemInPersistentStore is
a mistake: it's called at the end of the download now, but there's absolutely no
guarantee of being called there (or only there) in the future.  I think you want
to hook in ShouldCompleteDownload; will that work for you?  I think that's
equivalent to UpdatePathForItemInPersistentStore for your purposes.

> Noe, you have some good points about the threading / reference counting.  I'm
> working on a fix for this.
> 
> On 2011/10/22 17:08:07, rdsmith wrote:
> >
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> > File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> > 
> >
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> > chrome/browser/download/chrome_download_manager_delegate.cc:228: 
> > Can we localize the safebrowsing code in this file to a single callout in
> > ShouldStartDownload (or, alternatively, here)?  I'd like to minimize how
> > interwoven the download code and the safebrowing code are.  In my ideal
world,
> > the only line in this file would be a call to a safebrowing function
> containing
> > a pointer to the download item and a callback to be called when the
> safebrowsing
> > checks were done.  Can we move in that direction?
> > 
> > My apologies for not weighing in earlier--this is partially based on my
> > conversation with Noe' yesterday, which got me paying more attention to this
> > patch.

[Randy Smith (Not in Mondays), 2011-10-24 18:15:11]

On 2011/10/24 17:40:25, noelutz wrote:
> Randy and I figured that eventually there should be exactly two calls
> to SB classes in this code:
> 
> 1) right after the download starts to match the URL against the bad
> binary URL list (existing code).
> 
> 2) once the download is finished to check the download hash and
> contact the download protection servers (new code).
> 
> Randy: in the short term it would be nice if we could add a third call
> that Brian is adding here.  Once 2) is implemented we can remove the
> second existing call into SB which matches the binary hash against the
> blacklist of known bad hashes.  If you prefer to have that second
> existing call into SB removed sooner rather than later I can do that
> right after Brian's CL.  How does this proposal sound to you?  The
> reason I advocate for submitting Brian's CL sooner rather than later
> is because other changes are blocking on this CL and we would like to
> get some UMA stats back asap.

Noe': I'm ok with three calls (temporarily :-}) but I would like this one put
into ShouldCompleteDownload() for the reasons I give above, unless someone sees
problems with that approach.


> 
> Thanks,
> noe.
> 
> > Noe, you have some good points about the threading / reference counting.
> >  I'm
> > working on a fix for this.
> >
> > On 2011/10/22 17:08:07, rdsmith wrote:
> >
> >
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> >>
> >> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/8345033/diff/24002/chrome/browser/download/chr...
> >>
> >> chrome/browser/download/chrome_download_manager_delegate.cc:228:
> >> Can we localize the safebrowsing code in this file to a single callout in
> >> ShouldStartDownload (or, alternatively, here)?  I'd like to minimize
how
> >> interwoven the download code and the safebrowing code are.  In my
ideal
> >> world,
> >> the only line in this file would be a call to a safebrowing function
> >
> > containing
> >>
> >> a pointer to the download item and a callback to be called when the
> >
> > safebrowsing
> >>
> >> checks were done.  Can we move in that direction?
> >
> >> My apologies for not weighing in earlier--this is partially based on my
> >> conversation with Noe' yesterday, which got me paying more attention to
> >> this
> >> patch.
> >
> >
> >
> > http://codereview.chromium.org/8345033/
> >
> 
> 
> 
> -- 
> -|||--

[Brian Ryner, 2011-10-24 21:50:19]

On 2011/10/24 18:12:24, rdsmith wrote:
> On 2011/10/22 19:47:31, Brian Ryner wrote:
> > rdsmith: So basically that would mean having the DownloadProtectionService
API
> > deal with DownloadItem directly?  I think that should be doable, unless
anyone
> > sees a problem with the dependencies.
> 
> Awesome; thank you.  I'm slowly trying to increase the conceptual
> consistency--and thus the readability--of the download system, and that would
> help.  If it messes up the safebrowsing system and is thus a zero-sum game, we
> should rethink.  But my guess is that that won't make the safebrowsing code
any
> more messy, and it'll make the download code neater.

I started working on this API change.  It seems like it does complicate testing
the SafeBrowsing code somewhat -- constructing a DownloadItem seems to require
also having a DownloadManager, which needs a delegate, etc.

One possibility would be to keep the current API that takes a DPS::DownloadInfo,
and add a standalone function to convert a DownloadItem to a DPS::DownloadInfo. 
That function could be tested as part of the download tests, if we want, though
it may be too trivial to write a test for.

Thoughts?

[Randy Smith (Not in Mondays), 2011-10-25 02:28:37]

On 2011/10/24 21:50:19, Brian Ryner wrote:
> On 2011/10/24 18:12:24, rdsmith wrote:
> > On 2011/10/22 19:47:31, Brian Ryner wrote:
> > > rdsmith: So basically that would mean having the DownloadProtectionService
> API
> > > deal with DownloadItem directly?  I think that should be doable, unless
> anyone
> > > sees a problem with the dependencies.
> > 
> > Awesome; thank you.  I'm slowly trying to increase the conceptual
> > consistency--and thus the readability--of the download system, and that
would
> > help.  If it messes up the safebrowsing system and is thus a zero-sum game,
we
> > should rethink.  But my guess is that that won't make the safebrowsing code
> any
> > more messy, and it'll make the download code neater.
> 
> I started working on this API change.  It seems like it does complicate
testing
> the SafeBrowsing code somewhat -- constructing a DownloadItem seems to require
> also having a DownloadManager, which needs a delegate, etc.
> 
> One possibility would be to keep the current API that takes a
DPS::DownloadInfo,
> and add a standalone function to convert a DownloadItem to a
DPS::DownloadInfo. 
> That function could be tested as part of the download tests, if we want,
though
> it may be too trivial to write a test for.
> 
> Thoughts?

Chuckle.  I'm having one of those New Age "It's all interconnected" moments :-}.
 I'm at this moment in the process of working on a set of changes to allow unit
testing the DownloadItem, which would also allow it to be created with mock
classes in place of the DownloadManager & etc.  There's also a change going in
soon that creates an interface (Abstract Base Class) for the DownloadItem so
that things other than the DownloadItem that require one can mock it.  I think
this means that you should commit without making this API change, but (if you're
willing) reserve some coding cycles in a month or so to make the change when we
have the proper infrastructure in place.  How does this sound to you?

[Brian Ryner, 2011-10-25 20:19:53]

Ok, please take another look.  I think I've addressed all of the points raised
about thread safety.  Here are the major changes:

- Each request is now represented by an individual refcounted object.  This
ensures that these objects can't go away while tasks for the request are pending
in a message loop.

- The request objects all hold a reference to the SafeBrowsingService, so it
can't go away while requests are in-progress.

- DPS is not refcounted, it's simply owned by the SafeBrowsingService.

- The SafeBrowsingService will now always be destroyed on the UI thread.  I
think this was normally the case but we weren't guaranteeing it.

- The DPS requests now start the URLFetcher from the UI thread, so that it can
be destroyed on this thread.

- The unittest now uses real threads, so that the relevant DCHECKs will fire.

- Factored out the DownloadItem -> DPS::DownloadInfo conversion as discussed
above.

- Removed the special case for getting a synchronous result from the DPS.  The
callback now always happens asynchronously, which simplifies the code in the
download manager delegate.

[noelutz, 2011-10-25 21:43:50]

Nicely done.  Just a few minor comments.

Thanks,
noe.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:238:
fetcher_->set_request_context(service_->request_context_getter_.get());
Check if the request is cancelled before accessing service_?

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:253:
service_->RequestFinished(this);
Same here.  I'm pretty sure it's possible for service_ to be deleted before
sb_service_ is going away.  See comment below.  I think you can simply cancel
all requests when DPS goes away and check canceled here.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:285:
DCHECK(download_requests_.empty());
I think you need to cancel requests here.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.h (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.h:75: virtual void
CheckClientDownload(const DownloadInfo& info,
It would be nice to keep the previous API.  Eventually, we're going to call this
method from ShouldCompleteDownload which has the same API.

[Randy Smith (Not in Mondays), 2011-10-25 21:55:57]

On 2011/10/25 20:19:53, Brian Ryner wrote:
> Ok, please take another look.  I think I've addressed all of the points raised
> about thread safety.  Here are the major changes:
> 
> - Each request is now represented by an individual refcounted object.  This
> ensures that these objects can't go away while tasks for the request are
pending
> in a message loop.
> 
> - The request objects all hold a reference to the SafeBrowsingService, so it
> can't go away while requests are in-progress.
> 
> - DPS is not refcounted, it's simply owned by the SafeBrowsingService.
> 
> - The SafeBrowsingService will now always be destroyed on the UI thread.  I
> think this was normally the case but we weren't guaranteeing it.
> 
> - The DPS requests now start the URLFetcher from the UI thread, so that it can
> be destroyed on this thread.
> 
> - The unittest now uses real threads, so that the relevant DCHECKs will fire.
> 
> - Factored out the DownloadItem -> DPS::DownloadInfo conversion as discussed
> above.
> 
> - Removed the special case for getting a synchronous result from the DPS.  The
> callback now always happens asynchronously, which simplifies the code in the
> download manager delegate.

Would it be possible to move this call out of UpdatePathForItemInPersistentStore
into either ShouldCompleteDownload or OnResponseCompleted?  It'll probably end
up in OnResponseCompleted medium term, depending on what Noe' says to my latest
proposal, but I'm fairly sure it shouldn't be in
UpdatePathForItemInPersistentStore.

[Brian Ryner, 2011-10-25 22:03:06]

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:238:
fetcher_->set_request_context(service_->request_context_getter_.get());
On 2011/10/25 21:43:50, noelutz wrote:
> Check if the request is cancelled before accessing service_?

Done.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:253:
service_->RequestFinished(this);
On 2011/10/25 21:43:50, noelutz wrote:
> Same here.  I'm pretty sure it's possible for service_ to be deleted before
> sb_service_ is going away.  See comment below.  I think you can simply cancel
> all requests when DPS goes away and check canceled here.

Ah, I guess you mean in the ShutDown() case?  Done.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:285:
DCHECK(download_requests_.empty());
On 2011/10/25 21:43:50, noelutz wrote:
> I think you need to cancel requests here.

So normally if we cancel a request, we still rely on the request calling
RequestFinished for the DPS to release its reference.  Are you suggesting that
we change this so that DPS always releases its reference when it Cancels a
request?

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.h (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.h:75: virtual void
CheckClientDownload(const DownloadInfo& info,
On 2011/10/25 21:43:50, noelutz wrote:
> It would be nice to keep the previous API.  Eventually, we're going to call
this
> method from ShouldCompleteDownload which has the same API.

Is there a drawback to always returning false from ShouldCompleteDownload so
that we can proceed asynchronously?  I feel like it makes things somewhat
cleaner.

[Brian Ryner, 2011-10-25 22:04:28]

Maybe I misunderstood, I thought you had said (in comment 28) you were ok with
this temporarily?

On 2011/10/25 21:55:57, rdsmith wrote:
> On 2011/10/25 20:19:53, Brian Ryner wrote:
> > Ok, please take another look.  I think I've addressed all of the points
raised
> > about thread safety.  Here are the major changes:
> > 
> > - Each request is now represented by an individual refcounted object.  This
> > ensures that these objects can't go away while tasks for the request are
> pending
> > in a message loop.
> > 
> > - The request objects all hold a reference to the SafeBrowsingService, so it
> > can't go away while requests are in-progress.
> > 
> > - DPS is not refcounted, it's simply owned by the SafeBrowsingService.
> > 
> > - The SafeBrowsingService will now always be destroyed on the UI thread.  I
> > think this was normally the case but we weren't guaranteeing it.
> > 
> > - The DPS requests now start the URLFetcher from the UI thread, so that it
can
> > be destroyed on this thread.
> > 
> > - The unittest now uses real threads, so that the relevant DCHECKs will
fire.
> > 
> > - Factored out the DownloadItem -> DPS::DownloadInfo conversion as discussed
> > above.
> > 
> > - Removed the special case for getting a synchronous result from the DPS. 
The
> > callback now always happens asynchronously, which simplifies the code in the
> > download manager delegate.
> 
> Would it be possible to move this call out of
UpdatePathForItemInPersistentStore
> into either ShouldCompleteDownload or OnResponseCompleted?  It'll probably end
> up in OnResponseCompleted medium term, depending on what Noe' says to my
latest
> proposal, but I'm fairly sure it shouldn't be in
> UpdatePathForItemInPersistentStore.

[Randy Smith (Not in Mondays), 2011-10-25 22:05:50]

On 2011/10/25 22:04:28, Brian Ryner wrote:
> Maybe I misunderstood, I thought you had said (in comment 28) you were ok with
> this temporarily?

Yes, I'm sorry about the miscommunication.  I'm ok with temporarily having the
code inline, rather than just passing a DownloadItem pointer to an external
routine.  I'd still like to have it out of UpdatePathForPersistentStore.  Is
that a problem?  My current understanding of the flow suggests that you should
just be able to move it.

> 
> On 2011/10/25 21:55:57, rdsmith wrote:
> > On 2011/10/25 20:19:53, Brian Ryner wrote:
> > > Ok, please take another look.  I think I've addressed all of the points
> raised
> > > about thread safety.  Here are the major changes:
> > > 
> > > - Each request is now represented by an individual refcounted object. 
This
> > > ensures that these objects can't go away while tasks for the request are
> > pending
> > > in a message loop.
> > > 
> > > - The request objects all hold a reference to the SafeBrowsingService, so
it
> > > can't go away while requests are in-progress.
> > > 
> > > - DPS is not refcounted, it's simply owned by the SafeBrowsingService.
> > > 
> > > - The SafeBrowsingService will now always be destroyed on the UI thread. 
I
> > > think this was normally the case but we weren't guaranteeing it.
> > > 
> > > - The DPS requests now start the URLFetcher from the UI thread, so that it
> can
> > > be destroyed on this thread.
> > > 
> > > - The unittest now uses real threads, so that the relevant DCHECKs will
> fire.
> > > 
> > > - Factored out the DownloadItem -> DPS::DownloadInfo conversion as
discussed
> > > above.
> > > 
> > > - Removed the special case for getting a synchronous result from the DPS. 
> The
> > > callback now always happens asynchronously, which simplifies the code in
the
> > > download manager delegate.
> > 
> > Would it be possible to move this call out of
> UpdatePathForItemInPersistentStore
> > into either ShouldCompleteDownload or OnResponseCompleted?  It'll probably
end
> > up in OnResponseCompleted medium term, depending on what Noe' says to my
> latest
> > proposal, but I'm fairly sure it shouldn't be in
> > UpdatePathForItemInPersistentStore.

[noelutz, 2011-10-25 22:13:42]

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:253:
service_->RequestFinished(this);
On 2011/10/25 22:03:07, Brian Ryner wrote:
> On 2011/10/25 21:43:50, noelutz wrote:
> > Same here.  I'm pretty sure it's possible for service_ to be deleted before
> > sb_service_ is going away.  See comment below.  I think you can simply
cancel
> > all requests when DPS goes away and check canceled here.
> 
> Ah, I guess you mean in the ShutDown() case?  Done.

Yeah the ShutDown case.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:285:
DCHECK(download_requests_.empty());
On 2011/10/25 22:03:07, Brian Ryner wrote:
> On 2011/10/25 21:43:50, noelutz wrote:
> > I think you need to cancel requests here.
> 
> So normally if we cancel a request, we still rely on the request calling
> RequestFinished for the DPS to release its reference.  Are you suggesting that
> we change this so that DPS always releases its reference when it Cancels a
> request?

Not necessarily but I believe there is a bug here.  In the ShutDown() case this
destructor will be called without SetEnabled being called first (I believe). 
However, some request objects might still live on.  Once they access their
service_ pointer everything will blow up ;).  If you cancel all requests first
you'll be safe assuming you add the checks mentioned above.

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.h (right):

http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.h:75: virtual void
CheckClientDownload(const DownloadInfo& info,
On 2011/10/25 22:03:07, Brian Ryner wrote:
> On 2011/10/25 21:43:50, noelutz wrote:
> > It would be nice to keep the previous API.  Eventually, we're going to call
> this
> > method from ShouldCompleteDownload which has the same API.
> 
> Is there a drawback to always returning false from ShouldCompleteDownload so
> that we can proceed asynchronously?  I feel like it makes things somewhat
> cleaner.

Randy might have some opinion on that.  I don't know if it really matters.

[Brian Ryner, 2011-10-25 22:50:39]

This patch addresses Noe's comments about DPS destruction, please take another
look.

[noelutz, 2011-10-25 22:59:08]

lgtm but first address Randy's comment?

Thanks,
noe.

[Brian Ryner, 2011-10-26 01:16:25]

Moved the check to ShouldCompleteDownload, and synced to the URLFetcher changes
on trunk.  Note that for now I'm always returning true from
ShouldCompleteDownload; we can change this later once we want to take some
action based on the pingback.

Please take another look.

[noelutz, 2011-10-26 02:06:08]

lgtm

[Randy Smith (Not in Mondays), 2011-10-26 16:55:48]

On 2011/10/25 22:13:42, noelutz wrote:
>
http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/download_protection_service.cc (right):
> 
>
http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/download_protection_service.cc:253:
> service_->RequestFinished(this);
> On 2011/10/25 22:03:07, Brian Ryner wrote:
> > On 2011/10/25 21:43:50, noelutz wrote:
> > > Same here.  I'm pretty sure it's possible for service_ to be deleted
before
> > > sb_service_ is going away.  See comment below.  I think you can simply
> cancel
> > > all requests when DPS goes away and check canceled here.
> > 
> > Ah, I guess you mean in the ShutDown() case?  Done.
> 
> Yeah the ShutDown case.
> 
>
http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/download_protection_service.cc:285:
> DCHECK(download_requests_.empty());
> On 2011/10/25 22:03:07, Brian Ryner wrote:
> > On 2011/10/25 21:43:50, noelutz wrote:
> > > I think you need to cancel requests here.
> > 
> > So normally if we cancel a request, we still rely on the request calling
> > RequestFinished for the DPS to release its reference.  Are you suggesting
that
> > we change this so that DPS always releases its reference when it Cancels a
> > request?
> 
> Not necessarily but I believe there is a bug here.  In the ShutDown() case
this
> destructor will be called without SetEnabled being called first (I believe). 
> However, some request objects might still live on.  Once they access their
> service_ pointer everything will blow up ;).  If you cancel all requests first
> you'll be safe assuming you add the checks mentioned above.
> 
>
http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/download_protection_service.h (right):
> 
>
http://codereview.chromium.org/8345033/diff/39002/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/download_protection_service.h:75: virtual void
> CheckClientDownload(const DownloadInfo& info,
> On 2011/10/25 22:03:07, Brian Ryner wrote:
> > On 2011/10/25 21:43:50, noelutz wrote:
> > > It would be nice to keep the previous API.  Eventually, we're going to
call
> > this
> > > method from ShouldCompleteDownload which has the same API.
> > 
> > Is there a drawback to always returning false from ShouldCompleteDownload so
> > that we can proceed asynchronously?  I feel like it makes things somewhat
> > cleaner.
> 
> Randy might have some opinion on that.  I don't know if it really matters.

You're welcome to always return false from ShouldCompleteDownload, as long as
you eventually follow that up with the call to complete the download.  Note that
long-term we're going to need to tweak this code so that the UI has a chance to
prompt the user and block the download based on the result after
ShouldCompleteDownload allows us to continue--Noe' and I are currently talking
about how we'll do that.

[Randy Smith (Not in Mondays), 2011-10-26 16:55:55]

LGTM.

[Brian Ryner, 2011-10-26 17:03:48]

Matt, did you want to take another look before I commit?

[mattm, 2011-10-26 20:08:29]

Have to do an interview + meeting, so I didn't finish re-reviewing yet.  If I
can finish this afternoon if you aren't in a hurry..

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:174: //
thread.
Should be "Delete the host object on the UI thread and release the
SafeBrowsingService." ?

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.h (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.h:127: // The
SafeBrowsingService owns us.
maybe add ", so we don't need to hold a reference to it."

[Brian Ryner, 2011-10-26 20:12:00]

This afternoon would be great.  Thanks.

[mattm, 2011-10-26 23:27:35]

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:164: if (sb_service
&& sb_service->download_protection_service()) {
I think we need a check here or in CheckClientDownload for whether the calling
profile has safebrowsing enabled.  Or maybe make this go through
DownloadSBClient, which has its own per-profile safe_browsing_enabled_.

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:211: // start the
pigback.
pingback

[Brian Ryner, 2011-10-27 01:03:04]

Please take another look.

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:164: if (sb_service
&& sb_service->download_protection_service()) {
On 2011/10/26 23:27:36, mattm wrote:
> I think we need a check here or in CheckClientDownload for whether the calling
> profile has safebrowsing enabled.  Or maybe make this go through
> DownloadSBClient, which has its own per-profile safe_browsing_enabled_.

Done.  In the interest of keeping things simple, I just added the pref check to
this |if|.  We can revisit adding it to DownloadSBClient later as we integrate
this more into the download flow.

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:174: //
thread.
On 2011/10/26 20:08:29, mattm wrote:
> Should be "Delete the host object on the UI thread and release the
> SafeBrowsingService." ?

Done.

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.cc:211: // start the
pigback.
On 2011/10/26 23:27:36, mattm wrote:
> pingback

Done.

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_protection_service.h (right):

http://codereview.chromium.org/8345033/diff/36006/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_protection_service.h:127: // The
SafeBrowsingService owns us.
On 2011/10/26 20:08:29, mattm wrote:
> maybe add ", so we don't need to hold a reference to it."

Done.

[mattm, 2011-10-27 01:06:12]

LGTM

[commit-bot: I haz the power, 2011-10-27 02:44:55]

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

[commit-bot: I haz the power, 2011-10-27 04:18:11]

Change committed as 107528

[Timur Iskhodzhanov, 2011-10-27 09:33:00]

You probably know this, but just FTR
this patch has introduced a few leaks detected by Valgrind and HeapChecker on
the Memory FYI waterfall.

[Brian Ryner, 2011-10-28 00:05:38]

This last revision has two changes:

- Fix safe_browsing_blocking_page_unittest.cc to release the SafeBrowsingService
before the UI BrowserThread is destroyed.  The change to use DeleteOnUIThread
had been causing this to leak since there was no UI thread around to post a
DeleteTask to.

- Disabled download_protection_service_unittest on ChromeOS, since
SafeBrowsingService is #ifdef'd to not instantiate the DPS on ChromeOS.

Please take one more look.

[noelutz, 2011-10-28 00:09:16]

lgtm

[mattm, 2011-10-28 00:11:36]

lgtm

[Randy Smith (Not in Mondays), 2011-10-28 00:18:18]

lgtm (no changes in this patchset to chrome_download_manager_delegate).

[commit-bot: I haz the power, 2011-10-28 00:20:57]

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

[commit-bot: I haz the power, 2011-10-28 01:32:36]

Change committed as 107674