More explicit thread checking in SafeBrowsingDatabase.

More explicit thread checking in SafeBrowsingDatabase.

As part of trying to understand threading rules for SafeBrowsingDatabase
I added these explicit checks which helps documenting the expected flow.

Also replaced old checks with modern Chromium synchronization constructs
and explicitly mark SafeBrowsingStoreFile non-thread-safe.

These new checks helped catch issue 438754 and issue 442891.

More robust thread safety coming in https://codereview.chromium.org/790703003/

BUG=438754, 440517, 338486
Committed: https://crrev.com/d330bbbc6523984e903baaa41ce208f4f72691c4
Cr-Commit-Position: refs/heads/master@{#308673}

[gab, 2014-11-20 21:18:56]

Patchset #1 (id:1) has been deleted

[gab, 2014-12-03 21:23:15]

Patchset #1 (id:20001) has been deleted

[gab, 2014-12-03 21:28:37]

Patchset #1 (id:40001) has been deleted

[gab, 2014-12-03 21:29:17]

Patchset #1 (id:40001) has been deleted

[gab, 2014-12-03 21:29:43]

gab@chromium.org changed reviewers:
+ mattm@chromium.org

[gab, 2014-12-03 21:29:43]

Matt, PTAL.

Thanks!
Gab

[mattm, 2014-12-03 22:57:09]

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:326: //
DCHECK_CURRENTLY_ON(BrowserThread::UI);
SafeBrowsingDatabaseManager is refcounted, so this can't really be enforced
unless you change it to DeleteOnUIThread
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

but it's not clear to me that it does need to destruct on the UI thread

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:679:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
The comments in the header say it's safe to call these methods on any thread. In
general I'm not so sure about the "these are thread-safe but dcheck for how they
are currently used" changes in this file.

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1742:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Actually this function does not look safe. It should be holding lookup_lock_
before accessing csd_whitelist_. oops. Would you mind adding that while you're
here?

[gab, 2014-12-04 19:57:27]

Replies below along with a new patch set.

PTAL, thanks!
Gab

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/database_manager.cc (right):

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/database_manager.cc:326: //
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2014/12/03 22:57:08, mattm wrote:
> SafeBrowsingDatabaseManager is refcounted, so this can't really be enforced
> unless you change it to DeleteOnUIThread
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)
> 
> but it's not clear to me that it does need to destruct on the UI thread

Well simply because it's constructed on the UI thread; otherwise we have to
verify that it's safe to destroy every member on a thread different from which
it was allocated on (taking a quick look now this "seems" fine today, but how do
we ensure this remains the case?).

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:679:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2014/12/03 22:57:08, mattm wrote:
> The comments in the header say it's safe to call these methods on any thread.
In
> general I'm not so sure about the "these are thread-safe but dcheck for how
they
> are currently used" changes in this file.

Ah, right, in fact I just updated all of those comments to say this (there was a
discrepancy between some saying some calls were IO thread only and some others
saying it was any threads when really all calls that properly lock are
any-thread friendly -- though doing so in practice would increase contention...
if we consider contention an issue then we should enforce IO thread only checks
via the API).

I still like the explicit checks for now as it helps document how things behave.
I do not want to put that in the API however as this is not how this class is
meant to behave.

In fact I was thinking of removing those checks once we have an explicit design
in-place which prevents developer mistakes (and thus alleviates the need to
remind developers of which methods may run on which threads -- as-is with this
CL every single method is documented with one DCHECK or another which adds to
readability in the mean time IMO, as proven by you noticing a missing lock
below!).

And even more so if we one-day think of componentizing safe_browsing (would make
sense as none of these DB's are really chrome/ specific) at which point the IO
thread would become a foreign concept.

https://codereview.chromium.org/744183002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1742:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2014/12/03 22:57:08, mattm wrote:
> Actually this function does not look safe. It should be holding lookup_lock_
> before accessing csd_whitelist_. oops. Would you mind adding that while you're
> here?

Ah ah, indeed, this is exactly why I want to alter the design further in a
follow-up to enforce the concurrency requirements rather than rely on
developer/reviewer not making hard-to-spot mistakes.

[gab, 2014-12-09 21:18:16]

Hey Matt, PTAL.

I'm in the progress of coming up with a thread-safe design for
SafeBrowsingDatabaseManager.

I'd still like to add these checks temporarily to make everything explicit, but
they'll be removed as soon as the design guarantees thread-safety naturally.

Cheers!
Gab

[gab, 2014-12-10 16:47:20]

Patchset #5 (id:140001) has been deleted

[mattm, 2014-12-12 23:50:44]

https://codereview.chromium.org/744183002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/744183002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:669:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Think I still don't see the benefit of these documentation checks, especially if
they're just going to be removed in the next CL.

[gab, 2014-12-15 21:58:00]

Patchset #6 (id:180001) has been deleted

[gab, 2014-12-15 22:04:40]

Patchset #6 (id:200001) has been deleted

[gab, 2014-12-15 22:05:37]

Removed undesired checks, PTAL.

Thanks,
Gab

https://codereview.chromium.org/744183002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/744183002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_database.cc:669:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2014/12/12 23:50:44, mattm wrote:
> Think I still don't see the benefit of these documentation checks, especially
if
> they're just going to be removed in the next CL.

Ok, agreed, they mostly helped me understanding how all of this was hooked
together and I thought it would be useful to a future reader (i.e., have every
method annotated), but the ThreadSafeStateManager will make this
self-documenting anyways.

Removed.

[mattm, 2014-12-15 23:30:21]

lgtm

[gab, 2014-12-16 20:43:39]

Patchset #8 (id:260001) has been deleted

[gab, 2014-12-16 20:47:32]

The CQ bit was checked by gab@chromium.org

[gab, 2014-12-16 20:48:27]

The CQ bit was unchecked by gab@chromium.org

[gab, 2014-12-16 20:54:09]

On 2014/12/15 23:30:21, mattm wrote:
> lgtm

Thanks, extracted one-line race condition fix to
https://codereview.chromium.org/793663003/ for ease of merging if desired.

[gab, 2014-12-16 20:56:01]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-12-16 20:56:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/744183002/300001

[gab, 2014-12-16 21:01:09]

Patchset #10 (id:320001) has been deleted

[gab, 2014-12-16 21:01:20]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-12-16 21:01:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/744183002/300001

[commit-bot: I haz the power, 2014-12-16 21:50:25]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2014-12-16 21:51:20]

Patchset 9 (id:??) landed as
https://crrev.com/d330bbbc6523984e903baaa41ce208f4f72691c4
Cr-Commit-Position: refs/heads/master@{#308673}