Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2225613002:
V4LDM: Add check for HashPrefix match and some tests (Closed)
|
Created:
4 years, 4 months ago by vakh (use Gerrit instead) Modified:
4 years, 4 months ago CC:
woz, chromium-reviews, noé, palmer, francois_mozilla.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionV4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods.
TODOs:
1. Queue checks if the DB isn't ready yet.
2. Send full hash requests if the prefix is found in the DB.
BUG=543161
Committed: https://crrev.com/30ae1c8a8f9f169652afe6feb85453201a9e489f
Cr-Commit-Position: refs/heads/master@{#413031}
Patch Set 1 #Patch Set 2 : Add test for disabled v4_local_db_manager_ #Patch Set 3 : rebase #
Total comments: 18
Patch Set 4 : Remove QueuedChecks. CR feedback (nparker@ and shess@) #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : Move declaration of stores_to_look close to site of use. #
Dependent Patchsets: Messages
Total messages: 47 (29 generated)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Add tests for CheckBrowseUrl and mocking framework
Remove reset_success_ and SetupLocalDatabaseManager in SetUp
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add initial check for HashPrefix match in V4Database and tests for V4LDBM BUG= ========== to ========== V4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods. TODOs: 1. Queue checks if the DB isn't ready yet. 2. Send full hash requests if the prefix is found in the DB. BUG=543161 ==========
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
Add test for disabled v4_local_db_manager_
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rebase
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
(I haven't gotten through the test code yet). https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:162: DCHECK((matched_stores.empty() && matched_hash_prefixes.empty()) || DECHECK_EQ(matched_stores.empty(), matched_hash_prefixes.empty()) https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:164: // TODO(vakh): Fetch full hashes for the matching hash prefixes. You need to invoke the client's callback asynchronously if returning false. Maybe it's better to just return true until then. I'm not sure the value of splitting matched_mash_prefix_map into two hash_sets. It'd be better to add those once you've got a use for them, like when fetching full hashes or invoking the client->callback (does it need them?). https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:62: struct QueuedCheck { It'd be better to add queuing all at once in a separate CL, since I can't tell what the full impl will be. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:18: MatchedHashPrefixMap matched_hash_prefix_map_; If this is the only member, you could just pass that w/o the wrapper class. Less code is better. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:32: for (const auto it : config_.matched_hash_prefix_map_) { I think the whole loop could be: matched_hash_prefix_map->erase(); *matched_hash_prefix_map = config_.matched_hash_prefix_map_;
https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:62: struct QueuedCheck { On 2016/08/15 19:06:41, Nathan Parker wrote: > It'd be better to add queuing all at once in a separate CL, since I can't tell > what the full impl will be. I agree. The queued checks in the prior version felt bolted-on to me, I always wondered if there were a better way to handle them. That kind of thing is probably best to handle all at once, rather than dealing with it after you've landed a bunch of scaffolding. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:32: for (const auto it : config_.matched_hash_prefix_map_) { On 2016/08/15 19:06:41, Nathan Parker wrote: > I think the whole loop could be: > > matched_hash_prefix_map->erase(); > *matched_hash_prefix_map = config_.matched_hash_prefix_map_; Wouldn't the erase() be redundant? https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:100: EXPECT_TRUE(v4_local_database_manager_->CanCheckUrl(GURL("http://a/"))); Just wondering ... is there any possibility that an unqualified hostname like this would lead to odd special cases? It might be reasonable to have a legit FQDN, like example.com or abc.xyz. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:120: v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr)); Looking at this line ... it appears non-self-documenting. Like maybe "CheckBrowseURL" should return whether the act of checking was successful or not, with a by-reference return for the results of the check. But if it's a check with built-in error handling, it should be something else with a yes/no response, like "IsBrowseUrlSafe" or "IsBrowseUrlUnsafe". Just IMHO, I might be overthinking it. Also, I'm not entirely clear how HashPrefix("aaaa") leads to this check matching, since "aaaa" and "a" aren't obviously related. For all I can tell, a check against "http://b/" would return the same thing.
Remove QueuedChecks. CR feedback (nparker@ and shess@)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Thanks for the review. PTAL. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:162: DCHECK((matched_stores.empty() && matched_hash_prefixes.empty()) || On 2016/08/15 at 19:06:41, Nathan Parker wrote: > DECHECK_EQ(matched_stores.empty(), matched_hash_prefixes.empty()) Done. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:164: // TODO(vakh): Fetch full hashes for the matching hash prefixes. On 2016/08/15 at 19:06:41, Nathan Parker wrote: > You need to invoke the client's callback asynchronously if returning false. Maybe it's better to just return true until then. > This method is not being called from product code so the return value is only useful in unit tests right now. That's why I have implemented it so and added a TODO. > I'm not sure the value of splitting matched_mash_prefix_map into two hash_sets. It'd be better to add those once you've got a use for them, like when fetching full hashes or invoking the client->callback (does it need them?). That's what the replacement for TODO would do. This is a step in that direction. The set is needed to get unique values for matching hash prefixes and stores/lists. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:62: struct QueuedCheck { On 2016/08/15 at 20:57:39, Scott Hess wrote: > On 2016/08/15 19:06:41, Nathan Parker wrote: > > It'd be better to add queuing all at once in a separate CL, since I can't tell > > what the full impl will be. > > I agree. The queued checks in the prior version felt bolted-on to me, I always wondered if there were a better way to handle them. That kind of thing is probably best to handle all at once, rather than dealing with it after you've landed a bunch of scaffolding. Done. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:18: MatchedHashPrefixMap matched_hash_prefix_map_; On 2016/08/15 at 19:06:41, Nathan Parker wrote: > If this is the only member, you could just pass that w/o the wrapper class. Less code is better. I think we might need more fields later, but since I am not adding right now, I have removed the wrapper class. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:32: for (const auto it : config_.matched_hash_prefix_map_) { On 2016/08/15 at 20:57:40, Scott Hess wrote: > On 2016/08/15 19:06:41, Nathan Parker wrote: > > I think the whole loop could be: > > > > matched_hash_prefix_map->erase(); > > *matched_hash_prefix_map = config_.matched_hash_prefix_map_; > > Wouldn't the erase() be redundant? Done. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:100: EXPECT_TRUE(v4_local_database_manager_->CanCheckUrl(GURL("http://a/"))); On 2016/08/15 at 20:57:39, Scott Hess wrote: > Just wondering ... is there any possibility that an unqualified hostname like this would lead to odd special cases? It might be reasonable to have a legit FQDN, like example.com or abc.xyz. Done. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:120: v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr)); On 2016/08/15 at 20:57:40, Scott Hess wrote: > Looking at this line ... it appears non-self-documenting. Like maybe "CheckBrowseURL" should return whether the act of checking was successful or not, with a by-reference return for the results of the check. But if it's a check with built-in error handling, it should be something else with a yes/no response, like "IsBrowseUrlSafe" or "IsBrowseUrlUnsafe". Just IMHO, I might be overthinking it. > I agree with that sentiment, but at the moment, I want to make the new DBManager have the exact same API as the one it is replacing so that the clients don't need to change. Once the migration is complete, I'll consider changing the API name. I may not get to it soon, but it is not urgent, IMO. > Also, I'm not entirely clear how HashPrefix("aaaa") leads to this check matching, since "aaaa" and "a" aren't obviously related. For all I can tell, a check against "http://b/" would return the same thing. That's true. I am injecting a fake return value from a dependency (V4Database) and testing that the code in the module under test (V4LDBM) does what's expected. It does not matter if the fake value from the dependency module is the exact value this code will encounter in production.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rebase
rebase
Patchset #5 (id:120001) has been deleted
lgtm https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:120: v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr)); On 2016/08/17 18:23:06, vakh wrote: > On 2016/08/15 at 20:57:40, Scott Hess wrote: > > Also, I'm not entirely clear how HashPrefix("aaaa") leads to this check > > matching, since "aaaa" and "a" aren't obviously related. For all I can tell, a > > check against "http://b/" would return the same thing. > > That's true. I am injecting a fake return value from a dependency (V4Database) > and testing that the code in the module under test (V4LDBM) does what's > expected. It does not matter if the fake value from the dependency module is the > exact value this code will encounter in production. Do you mean that the fake will always hit? OK, I guess, though it seems like the kind of thing where later you're going to have to adjust all of these things when the fake becomes more precise. https://codereview.chromium.org/2225613002/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2225613002/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:144: {GetUrlMalwareId(), GetUrlSocEngId()}); This can be in the if() block.
Move declaration of stores_to_look close to site of use.
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the lgtm, shess@. https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2225613002/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager_unittest.cc:120: v4_local_database_manager_->CheckBrowseUrl(GURL("http://a/"), nullptr)); On 2016/08/17 at 23:04:38, Scott Hess wrote: > On 2016/08/17 18:23:06, vakh wrote: > > On 2016/08/15 at 20:57:40, Scott Hess wrote: > > > Also, I'm not entirely clear how HashPrefix("aaaa") leads to this check > > > matching, since "aaaa" and "a" aren't obviously related. For all I can tell, a > > > check against "http://b/" would return the same thing. > > > > That's true. I am injecting a fake return value from a dependency (V4Database) > > and testing that the code in the module under test (V4LDBM) does what's > > expected. It does not matter if the fake value from the dependency module is the > > exact value this code will encounter in production. > > Do you mean that the fake will always hit? Yes, that's the case this test is targeting. > OK, I guess, though it seems like the kind of thing where later you're going to have to adjust all of these things when the fake becomes more precise. Perhaps. But it gives me confidence that the code I am committing today works as intended and no other changes break it until it is replaced with more precise code. https://codereview.chromium.org/2225613002/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2225613002/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:144: {GetUrlMalwareId(), GetUrlSocEngId()}); On 2016/08/17 at 23:04:38, Scott Hess wrote: > This can be in the if() block. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm [though shess's is sufficient]
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org Link to the patchset: https://codereview.chromium.org/2225613002/#ps160001 (title: "Move declaration of stores_to_look close to site of use.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== V4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods. TODOs: 1. Queue checks if the DB isn't ready yet. 2. Send full hash requests if the prefix is found in the DB. BUG=543161 ========== to ========== V4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods. TODOs: 1. Queue checks if the DB isn't ready yet. 2. Send full hash requests if the prefix is found in the DB. BUG=543161 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== V4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods. TODOs: 1. Queue checks if the DB isn't ready yet. 2. Send full hash requests if the prefix is found in the DB. BUG=543161 ========== to ========== V4LocalDatabaseManager: Add check for HashPrefix match from V4Database and tests for CheckBrowseUrl and some of the other trivial methods. TODOs: 1. Queue checks if the DB isn't ready yet. 2. Send full hash requests if the prefix is found in the DB. BUG=543161 Committed: https://crrev.com/30ae1c8a8f9f169652afe6feb85453201a9e489f Cr-Commit-Position: refs/heads/master@{#413031} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/30ae1c8a8f9f169652afe6feb85453201a9e489f Cr-Commit-Position: refs/heads/master@{#413031} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
