Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794273002:
Introduce SafeBrowsingDatabase::ThreadSafeStateManager. (Closed)
DescriptionIntroduce SafeBrowsingDatabase::ThreadSafeStateManager.
This enforces thread-safety by design via transactions for any
implementation code accessing shared SafeBrowsingDatabase members.
Previously this was enforced by assuming everybody would make proper
usage of |lookup_lock_| but this was deemed insufficient as programming
mistakes proved easy to make twice.
One of the subtleties of this transactional model (which made the previous
ad-hoc locking even harder to prove (and keep) correct) is that only the
main database thread is allowed to modify this state, allowing for
unlocked reads on the main thread which are important to avoid
contention when flushing to disk.
BUG=440517
Committed: https://crrev.com/b2f71d56ad092f81ac20f5fc72a12098866cad50
Cr-Commit-Position: refs/heads/master@{#309611}
Patch Set 1 : #
Total comments: 17
Patch Set 2 : bring main-thread-checks back #Patch Set 3 : merge #Patch Set 4 : const ReadTransactions/mutable cache #Patch Set 5 : Move helpers/Augment WriteTransaction's functionality, reducing exposure of ThreadSafeStateManager'… #Patch Set 6 : private constructors w/ friend #Patch Set 7 : inline even more transactions #
Total comments: 6
Patch Set 8 : fix memory management #
Messages
Total messages: 20 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Matt, here's the thread-safety-by-design I've been talking about, please take a look. This is based on top of https://codereview.chromium.org/790703003/ (which has to go first) and https://codereview.chromium.org/744183002/ (which could be merged in this CL but I thought it was nice to keep this CL as focused as possible since it's already fairly big). Thanks very much for your time once again (and please let me know if I should divert some of the review load towards Noe or Scott). Cheers! Gab https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (left): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:509: // DCHECK(thread_checker_.CalledOnValidThread()); This DCHECK is still covered by making SafeBrowsingStore NonThreadSafe in https://codereview.chromium.org/744183002/ (although it currently has to be disabled for the same reason as this one).
gab@chromium.org changed reviewers: + mattm@chromium.org
Whoops, actually adding Matt, see below :-)! On 2014/12/11 19:39:20, gab wrote: > Matt, here's the thread-safety-by-design I've been talking about, please take a > look. > > This is based on top of https://codereview.chromium.org/790703003/ (which has to > go first) and https://codereview.chromium.org/744183002/ (which could be merged > in this CL but I thought it was nice to keep this CL as focused as possible > since it's already fairly big). > > Thanks very much for your time once again (and please let me know if I should > divert some of the review load towards Noe or Scott). > > Cheers! > Gab > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > File chrome/browser/safe_browsing/safe_browsing_database.cc (left): > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_database.cc:509: // > DCHECK(thread_checker_.CalledOnValidThread()); > This DCHECK is still covered by making SafeBrowsingStore NonThreadSafe in > https://codereview.chromium.org/744183002/ (although it currently has to be > disabled for the same reason as this one).
https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (left): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:760: DCHECK(thread_checker_.CalledOnValidThread()); Why are all these removed? https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:475: ReadTransaction(ThreadSafeStateManager* outer, Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to make it even more explicit? (would need to make the lock mutable) https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:779: // Wants to acquire the lock itself. Why not make WhitelistEverything a method on WriteTransation? https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.h:605: bool change_detected_; Should all the non-thread-safe members be moved to a another sub-class which is NonThreadSafe? Otherwise there is still risk of accessing those ones improperly.
Patchset #2 (id:60001) has been deleted
Some tweaks, but mostly replies with some questions below. Thanks once again! Gab https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (left): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:760: DCHECK(thread_checker_.CalledOnValidThread()); On 2014/12/12 23:20:31, mattm wrote: > Why are all these removed? Because |thread_checker_| moved to ThreadSafeStateManager which now automatically enforces this where required. And for SafeBrowsingStore accesses we're also safe as it was made NonThreadSafe in https://codereview.chromium.org/744183002/. I just realized however that this doesn't cover the two bool members of this class. I'll add them back, plus they're nice for documentation purposes. https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:475: ReadTransaction(ThreadSafeStateManager* outer, On 2014/12/12 23:20:30, mattm wrote: > Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to make > it even more explicit? (would need to make the lock mutable) It could but |prefix_gethash_cache_| would also have to be mutable, WDYT? https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:779: // Wants to acquire the lock itself. On 2014/12/12 23:20:31, mattm wrote: > Why not make WhitelistEverything a method on WriteTransation? I debated that in lengths while writing this and here was my initial conclusion (actually what I'd precisely debated was having WhitelistEverything take a WriteTransaction* -- in which case your suggestion of making it a method of WriteTransaction is potentially even nicer -- but the reasoning remains the same): Doing so means that any caller of WhitelistEverything needs a transaction. This makes the specific call-site highlighted by this comment much cleaner off-course, but it makes some other call-sites very ugly unless we also make LoadWhitelist() take a WriteTransaction* (rather than obtain one for itself). We however can't make LoadWhitelist() take a transaction as it does some computation we'd rather do prior to holding the lock (i.e. it only needs the lock for the final in-place swap). One option I considered was having some sort of WriteTransaction::TemporaryRelease() method which would return a scoped_ptr<base::AutoUnlock> to allow unlocking in the middle of a transaction to perform some work, but I felt this was overly complicated and could be misused and instead settled for the current implementation of WhitelistEverything()/LoadWhitelist() -- which, FWIW, locks just as much as the previous implementation did so it's at least not worse and we could make trickier improvements in smaller follow-up changes if desired. Slight tangent: I made LoadPrefixSet() take a WriteTransaction* but it's different as it's only called during Init() so it doesn't matter whereas LoadWhitelist() is called on update as well (I stressed this new requirement in the updated documentation for LoadPrefixSet() in this CL). https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.h:605: bool change_detected_; On 2014/12/12 23:20:31, mattm wrote: > Should all the non-thread-safe members be moved to a another sub-class which is > NonThreadSafe? Otherwise there is still risk of accessing those ones improperly. Good point, do you think the SafeBrowsingStores should be in such a NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or just these two bools (in which case I can easily make a small utils class for the two bools).
Patchset #3 (id:100001) has been deleted
@mattm: ping :-) Hey Matt, could you please take a look at the replies/questions below when you have a minute. The small debate on the precursor CL (https://codereview.chromium.org/790703003/) doesn't really matter as far as how we want to resolve the top-level questions on this CL. Thanks! Gab On 2014/12/15 23:02:30, gab wrote: > Some tweaks, but mostly replies with some questions below. > > Thanks once again! > Gab > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > File chrome/browser/safe_browsing/safe_browsing_database.cc (left): > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_database.cc:760: > DCHECK(thread_checker_.CalledOnValidThread()); > On 2014/12/12 23:20:31, mattm wrote: > > Why are all these removed? > > Because |thread_checker_| moved to ThreadSafeStateManager which now > automatically enforces this where required. > > And for SafeBrowsingStore accesses we're also safe as it was made NonThreadSafe > in https://codereview.chromium.org/744183002/. > > I just realized however that this doesn't cover the two bool members of this > class. > > I'll add them back, plus they're nice for documentation purposes. > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > File chrome/browser/safe_browsing/safe_browsing_database.cc (right): > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_database.cc:475: > ReadTransaction(ThreadSafeStateManager* outer, > On 2014/12/12 23:20:30, mattm wrote: > > Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to > make > > it even more explicit? (would need to make the lock mutable) > > It could but |prefix_gethash_cache_| would also have to be mutable, WDYT? > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_database.cc:779: // Wants to acquire > the lock itself. > On 2014/12/12 23:20:31, mattm wrote: > > Why not make WhitelistEverything a method on WriteTransation? > > I debated that in lengths while writing this and here was my initial conclusion > (actually what I'd precisely debated was having WhitelistEverything take a > WriteTransaction* -- in which case your suggestion of making it a method of > WriteTransaction is potentially even nicer -- but the reasoning remains the > same): > > > Doing so means that any caller of WhitelistEverything needs a transaction. This > makes the specific call-site highlighted by this comment much cleaner > off-course, but it makes some other call-sites very ugly unless we also make > LoadWhitelist() take a WriteTransaction* (rather than obtain one for itself). > > We however can't make LoadWhitelist() take a transaction as it does some > computation we'd rather do prior to holding the lock (i.e. it only needs the > lock for the final in-place swap). > > One option I considered was having some sort of > WriteTransaction::TemporaryRelease() method which would return a > scoped_ptr<base::AutoUnlock> to allow unlocking in the middle of a transaction > to perform some work, but I felt this was overly complicated and could be > misused and instead settled for the current implementation of > WhitelistEverything()/LoadWhitelist() -- which, FWIW, locks just as much as the > previous implementation did so it's at least not worse and we could make > trickier improvements in smaller follow-up changes if desired. > > > Slight tangent: I made LoadPrefixSet() take a WriteTransaction* but it's > different as it's only called during Init() so it doesn't matter whereas > LoadWhitelist() is called on update as well (I stressed this new requirement in > the updated documentation for LoadPrefixSet() in this CL). > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > File chrome/browser/safe_browsing/safe_browsing_database.h (right): > > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_database.h:605: bool > change_detected_; > On 2014/12/12 23:20:31, mattm wrote: > > Should all the non-thread-safe members be moved to a another sub-class which > is > > NonThreadSafe? Otherwise there is still risk of accessing those ones > improperly. > > Good point, do you think the SafeBrowsingStores should be in such a > NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or just > these two bools (in which case I can easily make a small utils class for the two > bools).
https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:475: ReadTransaction(ThreadSafeStateManager* outer, On 2014/12/15 23:02:30, gab wrote: > On 2014/12/12 23:20:30, mattm wrote: > > Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to > make > > it even more explicit? (would need to make the lock mutable) > > It could but |prefix_gethash_cache_| would also have to be mutable, WDYT? Hm, good point. It kinda seems like prefix_gethash_cache_ should actually go on WriteTransaction, though then you get the issue of wanting things from ReadTransaction and WriteTransaction in the same transaction. The one in PrefixSetContainsUrlHashes doesn't seem too egregious since you could argue that expiring old entries makes sense as sort of an internal detail that would fit with mutable. The use of ReadTransaction in CacheHashResults feels odd though. I guess this isn't a huge deal either way so you could put that as maybe something to improve later. https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:779: // Wants to acquire the lock itself. On 2014/12/15 23:02:30, gab wrote: > On 2014/12/12 23:20:31, mattm wrote: > > Why not make WhitelistEverything a method on WriteTransation? > > I debated that in lengths while writing this and here was my initial conclusion > (actually what I'd precisely debated was having WhitelistEverything take a > WriteTransaction* -- in which case your suggestion of making it a method of > WriteTransaction is potentially even nicer -- but the reasoning remains the > same): > > > Doing so means that any caller of WhitelistEverything needs a transaction. This > makes the specific call-site highlighted by this comment much cleaner > off-course, but it makes some other call-sites very ugly unless we also make > LoadWhitelist() take a WriteTransaction* (rather than obtain one for itself). Well, you could one-line it like state_manager_.BeginWriteTransaction()->WhitelistEverything(whitelist_id); which is only sort-of ugly. But I get your point. > We however can't make LoadWhitelist() take a transaction as it does some > computation we'd rather do prior to holding the lock (i.e. it only needs the > lock for the final in-place swap). > > One option I considered was having some sort of > WriteTransaction::TemporaryRelease() method which would return a > scoped_ptr<base::AutoUnlock> to allow unlocking in the middle of a transaction > to perform some work, but I felt this was overly complicated and could be > misused and instead settled for the current implementation of > WhitelistEverything()/LoadWhitelist() -- which, FWIW, locks just as much as the > previous implementation did so it's at least not worse and we could make > trickier improvements in smaller follow-up changes if desired. > > > Slight tangent: I made LoadPrefixSet() take a WriteTransaction* but it's > different as it's only called during Init() so it doesn't matter whereas > LoadWhitelist() is called on update as well (I stressed this new requirement in > the updated documentation for LoadPrefixSet() in this CL). https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.h:605: bool change_detected_; On 2014/12/15 23:02:30, gab wrote: > On 2014/12/12 23:20:31, mattm wrote: > > Should all the non-thread-safe members be moved to a another sub-class which > is > > NonThreadSafe? Otherwise there is still risk of accessing those ones > improperly. > > Good point, do you think the SafeBrowsingStores should be in such a > NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or just > these two bools (in which case I can easily make a small utils class for the two > bools). I guess to make it more consistent putting them in the nonthreadsafe class would be preferred, even if it's not necessary. Guess it depends how it ends up looking though.
Patchset #6 (id:180001) has been deleted
Hey Matt, responded below and made a couple of tweaks to improve the overall design. Let me know what you think. Thanks! Gab https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:475: ReadTransaction(ThreadSafeStateManager* outer, On 2014/12/23 02:09:45, mattm wrote: > On 2014/12/15 23:02:30, gab wrote: > > On 2014/12/12 23:20:30, mattm wrote: > > > Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to > > make > > > it even more explicit? (would need to make the lock mutable) > > > > It could but |prefix_gethash_cache_| would also have to be mutable, WDYT? > > Hm, good point. It kinda seems like prefix_gethash_cache_ should actually go on > WriteTransaction, though then you get the issue of wanting things from > ReadTransaction and WriteTransaction in the same transaction. > > The one in PrefixSetContainsUrlHashes doesn't seem too egregious since you could > argue that expiring old entries makes sense as sort of an internal detail that > would fit with mutable. The use of ReadTransaction in CacheHashResults feels odd > though. > > I guess this isn't a huge deal either way so you could put that as maybe > something to improve later. Agree that it's kind-of weird to write to the cache from a ReadTransaction, but the reason it is this way right now is that WriteTransactions are special in that they are only ever allowed on the main thread (where as the cache can be written to from any thread). At first this feels like it's calling for a special type of transactions for the cache, but this would be problematic as transactions can't be nested (the lock is not re-entrant) and we also can't use a different lock for these transactions or we'd have to enforce locking order (as without locking order, we'd introduce a potential for deadlocks). (the above argument also applies to why we can't move cache writes to WriteTransactions in the current design) I think I still like making everything const and the cache mutable though as it at least strongly highlights this somewhat weird design decision. (Done.) We could also add a special BeginCacheOnlyWriteTransaction() to alleviate weird usage of a ReadTransaction in CacheHashResults(), but since the cache already has to be mutable for expiring in PrefixSetContainsUrlHashes() this would, in practice, only add logic/code without providing additional functionality. Overall perhaps it's just the use of "Read"/"Write" nomenclature that is confusing? i.e. it's more "AnyThread"/"MainThread", but I'm not sure that's clearer to the average reader... Can you think of a better idea for clear nomenclature? Another idea (not in this CL but long term) would be to make the cache non-thread-safe (i.e. strictly used on the IO thread) as the only reason the main thread needs access is to clear it (which could potentially be done through a posted task) -- that would impose a thread restriction on cache users however (whereas currently it can be accessed from "any thread" though in practice it's already only accessed from the IO thread). https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:779: // Wants to acquire the lock itself. On 2014/12/23 02:09:45, mattm wrote: > On 2014/12/15 23:02:30, gab wrote: > > On 2014/12/12 23:20:31, mattm wrote: > > > Why not make WhitelistEverything a method on WriteTransation? > > > > I debated that in lengths while writing this and here was my initial > conclusion > > (actually what I'd precisely debated was having WhitelistEverything take a > > WriteTransaction* -- in which case your suggestion of making it a method of > > WriteTransaction is potentially even nicer -- but the reasoning remains the > > same): > > > > > > Doing so means that any caller of WhitelistEverything needs a transaction. > This > > makes the specific call-site highlighted by this comment much cleaner > > off-course, but it makes some other call-sites very ugly unless we also make > > LoadWhitelist() take a WriteTransaction* (rather than obtain one for itself). > > Well, you could one-line it like > state_manager_.BeginWriteTransaction()->WhitelistEverything(whitelist_id); > which is only sort-of ugly. But I get your point. Actually, I kind of like that and used it for a few other things which allows a little more inlining in WriteTransaction (i.e. no longer need to expose raw non-const pointers of ThreadSafeStateManager's members -- apart from the cache). > > > We however can't make LoadWhitelist() take a transaction as it does some > > computation we'd rather do prior to holding the lock (i.e. it only needs the > > lock for the final in-place swap). > > > > One option I considered was having some sort of > > WriteTransaction::TemporaryRelease() method which would return a > > scoped_ptr<base::AutoUnlock> to allow unlocking in the middle of a transaction > > to perform some work, but I felt this was overly complicated and could be > > misused and instead settled for the current implementation of > > WhitelistEverything()/LoadWhitelist() -- which, FWIW, locks just as much as > the > > previous implementation did so it's at least not worse and we could make > > trickier improvements in smaller follow-up changes if desired. > > > > > > Slight tangent: I made LoadPrefixSet() take a WriteTransaction* but it's > > different as it's only called during Init() so it doesn't matter whereas > > LoadWhitelist() is called on update as well (I stressed this new requirement > in > > the updated documentation for LoadPrefixSet() in this CL). > https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.h:605: bool change_detected_; On 2014/12/23 02:09:45, mattm wrote: > On 2014/12/15 23:02:30, gab wrote: > > On 2014/12/12 23:20:31, mattm wrote: > > > Should all the non-thread-safe members be moved to a another sub-class which > > is > > > NonThreadSafe? Otherwise there is still risk of accessing those ones > > improperly. > > > > Good point, do you think the SafeBrowsingStores should be in such a > > NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or > just > > these two bools (in which case I can easily make a small utils class for the > two > > bools). > > I guess to make it more consistent putting them in the nonthreadsafe class would > be preferred, even if it's not necessary. Guess it depends how it ends up > looking though. SG, how about we do this in a follow-up CL since this CL is already getting pretty big? Here's a proposal: https://codereview.chromium.org/814993003/
https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.cc:475: ReadTransaction(ThreadSafeStateManager* outer, On 2014/12/23 21:39:54, gab wrote: > On 2014/12/23 02:09:45, mattm wrote: > > On 2014/12/15 23:02:30, gab wrote: > > > On 2014/12/12 23:20:30, mattm wrote: > > > > Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager > to > > > make > > > > it even more explicit? (would need to make the lock mutable) > > > > > > It could but |prefix_gethash_cache_| would also have to be mutable, WDYT? > > > > Hm, good point. It kinda seems like prefix_gethash_cache_ should actually go > on > > WriteTransaction, though then you get the issue of wanting things from > > ReadTransaction and WriteTransaction in the same transaction. > > > > The one in PrefixSetContainsUrlHashes doesn't seem too egregious since you > could > > argue that expiring old entries makes sense as sort of an internal detail that > > would fit with mutable. The use of ReadTransaction in CacheHashResults feels > odd > > though. > > > > I guess this isn't a huge deal either way so you could put that as maybe > > something to improve later. > > Agree that it's kind-of weird to write to the cache from a ReadTransaction, but > the reason it is this way right now is that WriteTransactions are special in > that they are only ever allowed on the main thread (where as the cache can be > written to from any thread). > > At first this feels like it's calling for a special type of transactions for the > cache, but this would be problematic as transactions can't be nested (the lock > is not re-entrant) and we also can't use a different lock for these transactions > or we'd have to enforce locking order (as without locking order, we'd introduce > a potential for deadlocks). > > (the above argument also applies to why we can't move cache writes to > WriteTransactions in the current design) > > I think I still like making everything const and the cache mutable though as it > at least strongly highlights this somewhat weird design decision. > (Done.) > > > We could also add a special BeginCacheOnlyWriteTransaction() to alleviate weird > usage of a ReadTransaction in CacheHashResults(), but since the cache already > has to be mutable for expiring in PrefixSetContainsUrlHashes() this would, in > practice, only add logic/code without providing additional functionality. > > > Overall perhaps it's just the use of "Read"/"Write" nomenclature that is > confusing? i.e. it's more "AnyThread"/"MainThread", but I'm not sure that's > clearer to the average reader... Can you think of a better idea for clear > nomenclature? Not coming up with anything.. I guess this is okay. > > > Another idea (not in this CL but long term) would be to make the cache > non-thread-safe (i.e. strictly used on the IO thread) as the only reason the > main thread needs access is to clear it (which could potentially be done through > a posted task) -- that would impose a thread restriction on cache users however > (whereas currently it can be accessed from "any thread" though in practice it's > already only accessed from the IO thread). Hm, interesting. Although, the code already needs to hold the lock at the point the cache is used in PrefixSetContainsUrlHashes, so CacheHashResults is the only place that would really benefit. I think the current revision is fine. https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/794273002/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_database.h:605: bool change_detected_; On 2014/12/23 21:39:54, gab wrote: > On 2014/12/23 02:09:45, mattm wrote: > > On 2014/12/15 23:02:30, gab wrote: > > > On 2014/12/12 23:20:31, mattm wrote: > > > > Should all the non-thread-safe members be moved to a another sub-class > which > > > is > > > > NonThreadSafe? Otherwise there is still risk of accessing those ones > > > improperly. > > > > > > Good point, do you think the SafeBrowsingStores should be in such a > > > NonThreadSafeStateManager (since they're already NonThreadSafe anyways) or > > just > > > these two bools (in which case I can easily make a small utils class for the > > two > > > bools). > > > > I guess to make it more consistent putting them in the nonthreadsafe class > would > > be preferred, even if it's not necessary. Guess it depends how it ends up > > looking though. > > SG, how about we do this in a follow-up CL since this CL is already getting > pretty big? > Here's a proposal: https://codereview.chromium.org/814993003/ separate CL sounds good https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:543: scoped_ptr<std::vector<SBFullHash>> new_whitelist) { why not just pass "std::vector<SBFullHash>* new_whitelist" ? https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:546: whitelist->first.swap(*new_whitelist.release()); This is a leak isn't it? https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:552: outer_->ip_blacklist_.swap(*new_blacklist.release()); same as above
Oops, memory management mistakes fixed, good catch! Thanks, Gab https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:543: scoped_ptr<std::vector<SBFullHash>> new_whitelist) { On 2014/12/24 00:29:54, mattm wrote: > why not just pass "std::vector<SBFullHash>* new_whitelist" ? Because I try to always have methods that take ownership use a scoped_ptr for strong semantics. Though I just realized that this doesn't actually transfer ownership, it swaps ownership; and, as you pointed out, my current approach results in leaking the old content of the container. Thanks for catching this, fixed. https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:546: whitelist->first.swap(*new_whitelist.release()); On 2014/12/24 00:29:53, mattm wrote: > This is a leak isn't it? Done. https://codereview.chromium.org/794273002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:552: outer_->ip_blacklist_.swap(*new_blacklist.release()); On 2014/12/24 00:29:54, mattm wrote: > same as above Done.
lgtm, thanks for taking this on!
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794273002/240001
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b2f71d56ad092f81ac20f5fc72a12098866cad50 Cr-Commit-Position: refs/heads/master@{#309611} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
