Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349603003:
V4LDBM: Get response from GetHashManager, detect severest result (Closed)
|
Created:
4 years, 3 months ago by vakh (use Gerrit instead) Modified:
4 years, 2 months ago CC:
chromium-reviews, kcarattini Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionV4LDBM: Get response from GetHashManager, detect severest result
Also, some DCHECKS for ensuring methods run on expected threads.
BUG=543161
Committed: https://crrev.com/63654595ed31f9b5584de13aaee56c671bca7cf0
Cr-Commit-Position: refs/heads/master@{#419865}
Patch Set 1 : Remove DVLOG. Move severity methods into anon namespace. #Patch Set 2 : Add test for GetSeverestThreatTypeAndMetadata #
Total comments: 17
Patch Set 3 : Incorporate nparker@'s feedback #
Total comments: 6
Patch Set 4 : Incorporate nparker@'s feedback #Patch Set 5 : git pull #
Total comments: 8
Messages
Total messages: 44 (25 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
git pull and remove a DVLOG
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 #1 (id:1) has been deleted
Add test for GetSeverestThreatTypeAndMetadata and add store for UwS
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...
https://codereview.chromium.org/2349603003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:145: // // |full_hash_infos|. TODO(vakh): Indentation.
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
Remove store for UwS. Will add that as a separate CL.
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...
Nits
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:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks pretty good, with some nits and questions about naming. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:29: // GetSBThreatTypeForList Maybe you can enforce that with a compile-time assert or dcheck https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:38: if (list_id == GetChromeUrlApiId()) { nit: When this list gets longer, you might want an unordered map. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:246: for (size_t i = 0; i < full_hash_infos.size(); i++) { nit: for (const FullHashInfo& fi : full_hash_infos) https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:31: enum class CheckType { I think this name is a bit too general. Does this map to the use-case of the caller, and hence to the check function they called, like db_mananger->CheckFOO()? Or maybe there's an equivalency set of these three: 1) Which CheckFOO() function was called 2) which "CheckType" is used 3) Which client->callback_function should be used There are 12 Check/Match functions (including the two IsXXXKillSwitchOn()), only 5 of which take a Client* and so would have a callback. The others check synchronously, and maybe assume that the DB has 32-byte hashes for that list so no network call is possibly required. The enum could be named ClientCallbackType, since that's how it's used. Ultimately it'd have 5 values. An aside: In the long run, I'd be glad to get rid of Client* and just have the requestor pass in a callback to receive the results. But I don't think we want to switch that now, since you need a consistent DBManager API so you can swap the implementation cleanly. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:43: struct PendingCheck { Will this be used just for single URL checks, or other data types (list-of-URLs, digests, DLL-filenames, etc). And might as well make it private. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:56: GURL url; Should this be a vector, like the comments above imply? https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:157: // The set of clients that are waiting for a gull hash response from the nit: s/gull/full
Incorporate nparker@'s feedback
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 quick review. PTAL. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:29: // GetSBThreatTypeForList On 2016/09/20 17:51:09, Nathan Parker wrote: > Maybe you can enforce that with a compile-time assert or dcheck I am using the GetChromeUrlApiId store in tests but I can't add it here since we don't want to create a file on disk for that store. I've added a TODO for that here. I'll address it when I add code to handle stores that don't have an associated file on disk. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:38: if (list_id == GetChromeUrlApiId()) { On 2016/09/20 17:51:09, Nathan Parker wrote: > nit: When this list gets longer, you might want an unordered map. Acknowledged. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:246: for (size_t i = 0; i < full_hash_infos.size(); i++) { On 2016/09/20 17:51:09, Nathan Parker wrote: > nit: for (const FullHashInfo& fi : full_hash_infos) Done. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:31: enum class CheckType { On 2016/09/20 17:51:09, Nathan Parker wrote: > I think this name is a bit too general. Does this map to the use-case of the > caller, and hence to the check function they called, like > db_mananger->CheckFOO()? Or maybe there's an equivalency set of these three: > 1) Which CheckFOO() function was called > 2) which "CheckType" is used > 3) Which client->callback_function should be used > > > There are 12 Check/Match functions (including the two IsXXXKillSwitchOn()), only > 5 of which take a Client* and so would have a callback. The others check > synchronously, and maybe assume that the DB has 32-byte hashes for that list so > no network call is possibly required. > > The enum could be named ClientCallbackType, since that's how it's used. > Ultimately it'd have 5 values. Yes, it is supposed to reflect the function to call on the Client object. I've changed the name of the enum. > > > An aside: In the long run, I'd be glad to get rid of Client* and just have the > requestor pass in a callback to receive the results. But I don't think we want > to switch that now, since you need a consistent DBManager API so you can swap > the implementation cleanly. Ack. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:43: struct PendingCheck { On 2016/09/20 17:51:09, Nathan Parker wrote: > Will this be used just for single URL checks, or other data types (list-of-URLs, > digests, DLL-filenames, etc). URLs for now. Other types later. > > And might as well make it private. Done. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:56: GURL url; On 2016/09/20 17:51:09, Nathan Parker wrote: > Should this be a vector, like the comments above imply? Not now, later. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:157: // The set of clients that are waiting for a gull hash response from the On 2016/09/20 17:51:09, Nathan Parker wrote: > nit: s/gull/full Done.
lgtm https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:42: // synchronously. Can you add a todo or note that this will be extended to manage additional data types needed by the other callback types? This union/reuse of the same struct for different purposes is a pattern I'm not thrilled with (it is used in the existing pver3 db), but I don't know of a strong alternative. Maybe it'd help to document which fields are used for which callback types, when you add others. https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:57: // The URL that are being checked for being unsafe. nit: s/are/is https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:160: // SafeBrowsing service. Add what it's used for: For tracking when a client has canceled its request
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Incorporate nparker@'s feedback
git pull
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2349603003/#ps140001 (title: "git pull")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:42: // synchronously. On 2016/09/20 18:43:38, Nathan Parker wrote: > Can you add a todo or note that this will be extended to manage additional data > types needed by the other callback types? Done. > > This union/reuse of the same struct for different purposes is a pattern I'm not > thrilled with (it is used in the existing pver3 db), but I don't know of a > strong alternative. I'm not thrilled either. One way would be to have a base class that's extended for each Check* method but that seems like an overkill. Definitely not something I would like to do before launch. > Maybe it'd help to document which fields are used for which > callback types, when you add others. It might be more useful to add those details/comments when this code starts working for other methods. https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:57: // The URL that are being checked for being unsafe. On 2016/09/20 18:43:37, Nathan Parker wrote: > nit: s/are/is Done. https://codereview.chromium.org/2349603003/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:160: // SafeBrowsing service. On 2016/09/20 18:43:38, Nathan Parker wrote: > Add what it's used for: For tracking when a client has canceled its request Done.
https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:38: if (list_id == GetChromeUrlApiId()) { On 2016/09/20 17:51:09, Nathan Parker wrote: > nit: When this list gets longer, you might want an unordered map. This kind of stuff in the old code feels like the messy-polymorphism anti-pattern. Like maybe the selector thingie belongs in UpdateListIdentifier so it can be effectively optimized. https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:31: enum class CheckType { On 2016/09/20 17:51:09, Nathan Parker wrote: > An aside: In the long run, I'd be glad to get rid of Client* and just have the > requestor pass in a callback to receive the results. But I don't think we want > to switch that now, since you need a consistent DBManager API so you can swap > the implementation cleanly. OMG +++!!!1!111!!oneoneone https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:228: pending_clients_.insert(client); It looks to me like GetFullHashes() can return synchronously, which I think would "leak" this insert(). And possibly operate incorrectly? I dunno. Probably should have this line above GetFullHashes(). https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:276: RespondToClient(std::move(pending_check)); Shouldn't this also erase()? AFAICT, this if() clause isn't really improving anything. The empty case should be similar in performance with the (likely inlined) helper function. So maybe it could just be removed. Or the if() inverted and the Get*() put inside. https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.h:133: // avaialble for the URL that we requested. It determines the severest available :-). https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager_unittest.cc:158: std::reverse(std::begin(fhis), std::end(fhis)); I may not understand something, here, but a one-element list is the same forwards and backwards!
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== V4LDBM: Get response from GetHashManager, detect severest result Also, some DCHECKS for ensuring methods run on expected threads. BUG=543161 ========== to ========== V4LDBM: Get response from GetHashManager, detect severest result Also, some DCHECKS for ensuring methods run on expected threads. BUG=543161 Committed: https://crrev.com/63654595ed31f9b5584de13aaee56c671bca7cf0 Cr-Commit-Position: refs/heads/master@{#419865} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/63654595ed31f9b5584de13aaee56c671bca7cf0 Cr-Commit-Position: refs/heads/master@{#419865}
Message was sent while issue was closed.
Thanks for the review, Scott. I've uploaded a new CL to address you comments here: https://codereview.chromium.org/2353413002 https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:38: if (list_id == GetChromeUrlApiId()) { On 2016/09/20 21:55:55, Scott Hess wrote: > On 2016/09/20 17:51:09, Nathan Parker wrote: > > nit: When this list gets longer, you might want an unordered map. > > This kind of stuff in the old code feels like the messy-polymorphism > anti-pattern. Like maybe the selector thingie belongs in UpdateListIdentifier > so it can be effectively optimized. You're right. I did not put it there because the SB_THREAT_TYPE_ values are used only in the V4LocalDatabaseManager so I wasn't sure if I should add them to UpdateListIdentifier. However, I do realize that this if-else block is much more messy so I've changed ListIdentifier to also contain a member of type SBThreatType. https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:228: pending_clients_.insert(client); On 2016/09/20 21:55:55, Scott Hess wrote: > It looks to me like GetFullHashes() can return synchronously, which I think > would "leak" this insert(). And possibly operate incorrectly? I dunno. > Probably should have this line above GetFullHashes(). Good catch. Changed. https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:276: RespondToClient(std::move(pending_check)); On 2016/09/20 21:55:55, Scott Hess wrote: > Shouldn't this also erase()? > > AFAICT, this if() clause isn't really improving anything. The empty case should > be similar in performance with the (likely inlined) helper function. So maybe > it could just be removed. Or the if() inverted and the Get*() put inside. Done. https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager_unittest.cc:158: std::reverse(std::begin(fhis), std::end(fhis)); On 2016/09/20 21:55:56, Scott Hess wrote: > I may not understand something, here, but a one-element list is the same > forwards and backwards! The list has two elements: fhi_malware, fhi_api
Message was sent while issue was closed.
https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2349603003/diff/140001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager_unittest.cc:158: std::reverse(std::begin(fhis), std::end(fhis)); On 2016/09/21 17:43:40, vakh (Varun Khaneja) wrote: > On 2016/09/20 21:55:56, Scott Hess wrote: > > I may not understand something, here, but a one-element list is the same > > forwards and backwards! > > The list has two elements: fhi_malware, fhi_api I swear, I checked a couple times before making the comment, and it only had one element. But looking here, it clearly has more than one. *Shrug*. Guide me gently back to my desk if you find me wandering around. |
