Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747313002:
PasswordProtectionService verdict cache management (Closed)
|
Created:
3 years, 9 months ago by Jialiu Lin Modified:
3 years, 9 months ago CC:
asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, droger+watchlist_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, sdefresne+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPasswordProtectionService verdict cache management
Add functionalities to PasswordProtectionService to:
(1) cache pps verdict based on origin and cache expression in
HostContentSettingsMap
(2) Get cached verdict from HostContentSettingsMap
(3) Remove cached verdict upon URLs deleted from history service.
BUG=698899
Review-Url: https://codereview.chromium.org/2747313002
Cr-Commit-Position: refs/heads/master@{#458565}
Committed: https://chromium.googlesource.com/chromium/src/+/64fc03ab5fd993cc6573fa24266d177aaf8b1679
Patch Set 1 #Patch Set 2 : fix build #
Total comments: 8
Patch Set 3 : Address lpz's comments #
Total comments: 4
Patch Set 4 : partially address raymes's comments #Patch Set 5 : remove extra deps #
Total comments: 40
Patch Set 6 : comments addressed partially #
Total comments: 4
Patch Set 7 : address all comments #Patch Set 8 : nit #
Total comments: 2
Patch Set 9 : refine comment in csd.proto #
Total comments: 16
Patch Set 10 : address vakh's comments #Patch Set 11 : Cache verdict based on hostname #Messages
Total messages: 57 (35 generated)
The CQ bit was checked by jialiul@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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jialiul@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jialiul@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
jialiul@chromium.org changed reviewers: + lpz@chromium.org, nparker@chromium.org
nparker@, PTAL. lpz@, If you're interested in Phishguard project, I'll send you more CLs to review. :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Thanks Jialiu - yes, please send more CLs my way. Maybe I'll eventually have more to say than just style nits :) https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:109: it.key(), &verdict_entry)); nit: alignment? https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:42: // Looks up |settings, and returns the verdict of |url|. Can be called on any nit: trailing | on settings https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:74: // Overridden from history::HistoryServiceObserver. nit:empty line above? https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:87: // static nit: useless comment?
Thanks lpz@! https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:109: it.key(), &verdict_entry)); On 2017/03/15 15:14:58, lpz wrote: > nit: alignment? Acknowledged. git cl format insists this is the best alignment and gives presubmit warning if I align it in other ways..... https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:42: // Looks up |settings, and returns the verdict of |url|. Can be called on any On 2017/03/15 at 15:14:58, lpz wrote: > nit: trailing | on settings Done. https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:74: // Overridden from history::HistoryServiceObserver. On 2017/03/15 at 15:14:58, lpz wrote: > nit:empty line above? Done. https://codereview.chromium.org/2747313002/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:87: // static On 2017/03/15 at 15:14:58, lpz wrote: > nit: useless comment? Done.
jialiul@chromium.org changed reviewers: + raymes@chromium.org
+raymes@, could you take a look at my changes in components/content_settings? Let me know if I did everything needed for adding a new content settings type. Thanks!
lgtm but please update components/content_settings/core/common/content_settings.cc or it will break histograms. Thanks! https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_registry.cc:164: nullptr, WebsiteSettingsInfo::UNSYNCABLE, WebsiteSettingsInfo::LOSSY, Just checking: is it really ok for this to be LOSSY? It means that there is a chance that any updates are not written to disk immediately and may be lost. https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings.cc:58: CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, This should go right at the end but you may want to wait for https://codereview.chromium.org/2728303002/ which makes this clearer.
The CQ bit was checked by jialiul@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...
jialiul@chromium.org changed reviewers: + battre@chromium.org, sdefresne@chromium.org
Thanks raymes! I'll fix content_settings.cc after https://codereview.chromium.org/2728303002/ lands. +sdefresne@ for adding components/history into DEPS +battre@ for adding components/sync_preferences:test_support into DEPS https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... components/content_settings/core/browser/website_settings_registry.cc:164: nullptr, WebsiteSettingsInfo::UNSYNCABLE, WebsiteSettingsInfo::LOSSY, On 2017/03/16 at 00:38:37, raymes wrote: > Just checking: is it really ok for this to be LOSSY? It means that there is a chance that any updates are not written to disk immediately and may be lost. Thanks for pointing this out. Changed to NOT_LOSSY. https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2747313002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings.cc:58: CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, On 2017/03/16 at 00:38:37, raymes wrote: > This should go right at the end but you may want to wait for https://codereview.chromium.org/2728303002/ which makes this clearer. Sure. I'll wait for the https://codereview.chromium.org/2728303002.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm I have mostly nits and some brainstorming comments. Code looks good! https://codereview.chromium.org/2747313002/diff/100001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, Can we have it cache by hostname rather than origin? The ping and verdict aren't ambivalent to scheme and port. So if I visit a site w/ HTTPS and then with HTTP, it MIGHT be better to reuse the verdict and not rewarn. Lemme know your thoughts. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:85: if (!settings || !url.is_valid()) { nit: do you want to DCHECK on settings, or is there a case where it's valid to have it null? https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:109: &verdict_entry) && Is it an error if ParseVerdictEntry() returns false here? If so, that could also be a DCHECK (to help catch bugs) https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:113: !IsCacheExpired(cache_creation_time, cache_duration)) { Do you think we need to remove expired entries, or just wait for them to get refreshed? I think the latter might be sufficient. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:115: verdict_type_value); Something to think about: If we were to add new verdict types, or add more structure to the verdict, do we have a migration path? One option is to add a version number, but that seems like overkill. The key is that users might have very old verdicts that are getting read my a newer version of chrome. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:128: if (!verdict) Again, how about DCHECK instead? https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:145: // not overlapping with each other and are stable. Need to confirm with SB We could verify it on the client and do something predictable with it if there is an overlap. That way we'd notice it happening (UMA metrics) and wouldn't get undefined behaviour. Also, the notes here imply it might be overlapping: https://docs.google.com/a/google.com/document/d/13CW7q2_c1tUgAbZsCzVVnopKcJ4X... https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:240: for (const std::string& host : hosts) { I'm a little worried about CPU/latency here, since this inner loop is called a lot. When clearing history will we check every cleared URL against every cached entry for a given origin? It may not be a big deal, but something to keep an eye on. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:110: TEST_F(PasswordProtectionServiceTest, TestParseVerdictEntry) { nit: split valid/invalid cases into two tests, since they don't share data. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:147: GURL("https://www.google.com"), cache_expression)); How about w/ and w/o the trailing slash? Is there something there we should test? https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:190: password_protection_service_->CacheVerdict( nit: I find these a bit hard to reason about. Would be more clear if you grouped the verdict with the CacheVerdict() call, with blank lines between them? And can we do without the LoginReputationClientResponse:: prefix on verdict types? This implies it's in the safe_browsing namespace: https://cs.chromium.org/chromium/src/out/Debug/gen/components/safe_browsing/c... https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:206: GURL("http://www.unkown.com/index.html"))); (nit: www.unknown.com) https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:265: } This is a long test testing multiple different things. I think it would cleaner to split it, maybe into matching origins/paths, checking TTLs, and deleting entries. Or some such.
vakh@chromium.org changed reviewers: + vakh@chromium.org
https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:39: LoginReputationClientResponse* verdict, this can be const https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:81: PasswordProtectionService::GetCachedVerdict(HostContentSettingsMap* settings, const *settings? https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:124: HostContentSettingsMap* settings, Since |settings| is being set, it is an output, so it should be at the end of the list. See: https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:184: HostContentSettingsMap* setting_map, |setting_map| at the end https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:207: base::DictionaryValue* verdict_entry, |verdict_entry| const? https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:241: for (const std::string& path : paths) { Most code in this function is similar to V4ProtocolManagerUtil::UrlToFullHashes. Please consider making a common function that's called in both places and returns the combinations of canonical hosts and paths.
https://codereview.chromium.org/2747313002/diff/120001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2747313002/diff/120001/components/content_set... components/content_settings/core/common/content_settings.cc:58: CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, Sorry https://codereview.chromium.org/2728303002/ has been held up a bit and I'm going OOO so might be a bit slow on it. You may just simply want to remove this line for now and not collect data for it and we can add it in later. Thanks!
LGTM for adding //components/sync_preferences:test_support to DEPS https://codereview.chromium.org/2747313002/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/DEPS (right): https://codereview.chromium.org/2747313002/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/DEPS:7: "+components/sync_preferences", Is this used anywhere?
The CQ bit was checked by jialiul@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.
Thanks for all your comments! Weining and I agreed to introduce a boolean field in csd.proto to indicate if exact match is required. Therefore I changed how we search for cached verdicts (a.k.a handle exact match and suffix match differently). I added more tests accordingly. Changes in content_settings.* and histograms.xml were removed from this CL. They will be added back after https://crrev.com/2728303002/ land. https://codereview.chromium.org/2747313002/diff/100001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, On 2017/03/16 21:27:11, Nathan Parker wrote: > Can we have it cache by hostname rather than origin? The ping and verdict > aren't ambivalent to scheme and port. So if I visit a site w/ HTTPS and then > with HTTP, it MIGHT be better to reuse the verdict and not rewarn. Lemme know > your thoughts. I discussed this with Weining. "http vs. https is clearly a signal, so, it is possible that they might result in different result." Let's cache verdict by origin then. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:39: LoginReputationClientResponse* verdict, On 2017/03/16 22:04:30, vakh (Varun Khaneja) wrote: > this can be const Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:81: PasswordProtectionService::GetCachedVerdict(HostContentSettingsMap* settings, On 2017/03/16 22:04:29, vakh (Varun Khaneja) wrote: > const *settings? Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:85: if (!settings || !url.is_valid()) { On 2017/03/16 21:27:11, Nathan Parker wrote: > nit: do you want to DCHECK on settings, or is there a case where it's valid to > have it null? Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:109: &verdict_entry) && On 2017/03/16 21:27:11, Nathan Parker wrote: > Is it an error if ParseVerdictEntry() returns false here? If so, that could also > be a DCHECK (to help catch bugs) Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:113: !IsCacheExpired(cache_creation_time, cache_duration)) { On 2017/03/16 21:27:11, Nathan Parker wrote: > Do you think we need to remove expired entries, or just wait for them to get > refreshed? I think the latter might be sufficient. If the entry is expired, we are going to request a new verdict, which upon receipt will override the expired one. So we don't need to remove them here. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:115: verdict_type_value); On 2017/03/16 21:27:11, Nathan Parker wrote: > Something to think about: If we were to add new verdict types, or add more > structure to the verdict, do we have a migration path? One option is to add a > version number, but that seems like overkill. The key is that users might have > very old verdicts that are getting read my a newer version of chrome. Agreed. In the latest patch, I cache the serialized verdict proto instead of individual fields. The tricky part is, base::DictionaryValue only accept UTF8 strings, so I need to serialized proto as binary value (instead of string). Added a little bit conversions to handle that. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:124: HostContentSettingsMap* settings, On 2017/03/16 22:04:30, vakh (Varun Khaneja) wrote: > Since |settings| is being set, it is an output, so it should be at the end of > the list. See: > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:128: if (!verdict) On 2017/03/16 21:27:11, Nathan Parker wrote: > Again, how about DCHECK instead? Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:145: // not overlapping with each other and are stable. Need to confirm with SB On 2017/03/16 21:27:11, Nathan Parker wrote: > We could verify it on the client and do something predictable with it if there > is an overlap. That way we'd notice it happening (UMA metrics) and wouldn't get > undefined behaviour. > > Also, the notes here imply it might be overlapping: > https://docs.google.com/a/google.com/document/d/13CW7q2_c1tUgAbZsCzVVnopKcJ4X... You're right. I just confirmed with Weining. Overlapping could happen. This part if actually fine, I added more code in GetCachedVerdict to find the most specific cached verdict. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:184: HostContentSettingsMap* setting_map, On 2017/03/16 22:04:30, vakh (Varun Khaneja) wrote: > |setting_map| at the end Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:207: base::DictionaryValue* verdict_entry, On 2017/03/16 22:04:30, vakh (Varun Khaneja) wrote: > |verdict_entry| const? acknowledged. for some weird reason, base::DictionaryValue::Get(...) is not marked as const. So compiler will complain if I add a const to verdict_entry. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:240: for (const std::string& host : hosts) { On 2017/03/16 21:27:11, Nathan Parker wrote: > I'm a little worried about CPU/latency here, since this inner loop is called a > lot. When clearing history will we check every cleared URL against every cached > entry for a given origin? > > It may not be a big deal, but something to keep an eye on. Rewrote this function significantly. Now only one layer of loop is needed. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:241: for (const std::string& path : paths) { On 2017/03/16 22:04:30, vakh (Varun Khaneja) wrote: > Most code in this function is similar to V4ProtocolManagerUtil::UrlToFullHashes. > > Please consider making a common function that's called in both places and > returns the combinations of canonical hosts and paths. This function is rewritten significantly in the latest patch. Now it does not look like UrlToFullHashes anymore. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:110: TEST_F(PasswordProtectionServiceTest, TestParseVerdictEntry) { On 2017/03/16 21:27:11, Nathan Parker wrote: > nit: split valid/invalid cases into two tests, since they don't share data. Done. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:147: GURL("https://www.google.com"), cache_expression)); On 2017/03/16 21:27:11, Nathan Parker wrote: > How about w/ and w/o the trailing slash? Is there something there we should > test? Sure. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:190: password_protection_service_->CacheVerdict( On 2017/03/16 21:27:11, Nathan Parker wrote: > nit: I find these a bit hard to reason about. Would be more clear if you > grouped the verdict with the CacheVerdict() call, with blank lines between them? > And can we do without the LoginReputationClientResponse:: prefix on verdict > types? This implies it's in the safe_browsing namespace: > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/safe_browsing/c... Regrouped test case. There are multiple SAFE enum values defined in csd.proto, I have to keep the LoginReputationClientResponse prefix.. https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:206: GURL("http://www.unkown.com/index.html"))); On 2017/03/16 21:27:11, Nathan Parker wrote: > (nit: http://www.unknown.com) Good eye! https://codereview.chromium.org/2747313002/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:265: } On 2017/03/16 21:27:11, Nathan Parker wrote: > This is a long test testing multiple different things. I think it would cleaner > to split it, maybe into matching origins/paths, checking TTLs, and deleting > entries. Or some such. Done. Separated this one into 3 tests. https://codereview.chromium.org/2747313002/diff/120001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2747313002/diff/120001/components/content_set... components/content_settings/core/common/content_settings.cc:58: CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, On 2017/03/17 07:49:07, raymes (OOO until April 10) wrote: > Sorry https://codereview.chromium.org/2728303002/ has been held up a bit and I'm > going OOO so might be a bit slow on it. You may just simply want to remove this > line for now and not collect data for it and we can add it in later. Thanks! Sure. Removed for now. https://codereview.chromium.org/2747313002/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/DEPS (right): https://codereview.chromium.org/2747313002/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/DEPS:7: "+components/sync_preferences", On 2017/03/17 09:32:43, battre wrote: > Is this used anywhere? Yes, components/sync_preferences/testing_pref_service_syncable.h is used in password_protection_service_unittest Let me narrow the scope to "+components/symc_preferences/testing_pref_service_syncable.h"
https://codereview.chromium.org/2747313002/diff/100001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, On 2017/03/18 23:46:31, Jialiu Lin wrote: > On 2017/03/16 21:27:11, Nathan Parker wrote: > > Can we have it cache by hostname rather than origin? The ping and verdict > > aren't ambivalent to scheme and port. So if I visit a site w/ HTTPS and then > > with HTTP, it MIGHT be better to reuse the verdict and not rewarn. Lemme know > > your thoughts. > > I discussed this with Weining. "http vs. https is clearly a signal, so, it is > possible that they might result in different result." > Let's cache verdict by origin then. ok, though to be clear, the whitelist ignores scheme already. Do we know which way Weining compared history URLs when estimating ping rates? https://codereview.chromium.org/2747313002/diff/160001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2747313002/diff/160001/components/safe_browsi... components/safe_browsing/csd.proto:261: optional bool cache_expression_exact_match = 4; This isn't really "exact match," since we're ignoring the path in the cache expression (or assuming it has none?), correct? It _would_ be exact match if we ensured the candidate URL matched the expression up to both of their last /'s. Then dropbox.com/foo [exact] would match dropbox.com/foo dropbox.com/foo/bar.cgi NOT dropbox.com/foo/dog/cat.cgi I think that's a generalization of what you described -- it may or may not be useful. Alternately, we could change the name to "cache_expression_root_path" or some such and have it be only allowed when cache_expresion has no path. That'd be simpler.
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
https://codereview.chromium.org/2747313002/diff/100001/components/content_set... File components/content_settings/core/browser/website_settings_registry.cc (right): https://codereview.chromium.org/2747313002/diff/100001/components/content_set... components/content_settings/core/browser/website_settings_registry.cc:166: WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE, On 2017/03/20 15:55:35, Nathan Parker wrote: > On 2017/03/18 23:46:31, Jialiu Lin wrote: > > On 2017/03/16 21:27:11, Nathan Parker wrote: > > > Can we have it cache by hostname rather than origin? The ping and verdict > > > aren't ambivalent to scheme and port. So if I visit a site w/ HTTPS and > then > > > with HTTP, it MIGHT be better to reuse the verdict and not rewarn. Lemme > know > > > your thoughts. > > > > I discussed this with Weining. "http vs. https is clearly a signal, so, it is > > possible that they might result in different result." > > Let's cache verdict by origin then. > > ok, though to be clear, the whitelist ignores scheme already. Do we know which > way Weining compared history URLs when estimating ping rates? > Yes, whitelist ignores scheme. I believe he estimated ping rates based on site-chunk only. So we will probably end up with a ping rate higher than his estimation (but x2 at most). https://codereview.chromium.org/2747313002/diff/160001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2747313002/diff/160001/components/safe_browsi... components/safe_browsing/csd.proto:261: optional bool cache_expression_exact_match = 4; On 2017/03/20 15:55:35, Nathan Parker wrote: > This isn't really "exact match," since we're ignoring the path in the cache > expression (or assuming it has none?), correct? It _would_ be exact match if we > ensured the candidate URL matched the expression up to both of their last /'s. > Then > > dropbox.com/foo [exact] would match > dropbox.com/foo > dropbox.com/foo/bar.cgi > NOT dropbox.com/foo/dog/cat.cgi > > I think that's a generalization of what you described -- it may or may not be > useful. > > > Alternately, we could change the name to "cache_expression_root_path" or some > such and have it be only allowed when cache_expresion has no path. That'd be > simpler. > Ah, I copied this change from cl/150491510. I believe Weining tried to express the same thing. But somehow he used an example only has root path. Your examples are much better, much more generalized. I changed this comment according to your feedback. and I've forward your feedback to him too.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@, need your owner approval for adding "components/history/core/browser" to components/safe_browsing/password_protection/DEPS Could you take a look? Thanks!
https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/csd.proto:261: // "foo.com/bar/abc/edf.cgi" Can you also make it explicit that it won't match foo.com either? https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:104: DCHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion( This statement seems to be modifying verdict_entry. It seems to me that this shouldn't be a DCHECK since this would be compiled out on a release build. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:108: DCHECK(ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict)); Same here. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:174: return nullptr; Is this nullptr intentional? If so, why have this function? Is it missing a TODO? https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:217: base::DictionaryValue* verdict_entry, const verdict_entry https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:225: if (result) { nit: if (!result) { return false; } DCHECK() ... // This reduces the code indent and makes the exit condition very clear. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:303: if (verdict) { should this verdict check be a DCHECK instead? This is a private method so verdict == null seems like a programmer mistake in calling this method. The call site also has a DCHECK(verdict) already. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:306: // Because DictionaryValue cannot take none-UTF8 string, we need to store non
Thanks vakh@! https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/csd.proto:261: // "foo.com/bar/abc/edf.cgi" On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > Can you also make it explicit that it won't match http://foo.com either? Sure. Done. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:104: DCHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion( On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > This statement seems to be modifying verdict_entry. > It seems to me that this shouldn't be a DCHECK since this would be compiled out > on a release build. Changed to CHECK instead. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:108: DCHECK(ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict)); On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > Same here. Done. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:174: return nullptr; On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > Is this nullptr intentional? > If so, why have this function? > Is it missing a TODO? Yes, TODO is in the .h file. Added a corresponding TODO here too. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:217: base::DictionaryValue* verdict_entry, On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > const verdict_entry Acknowledged. for some weird reason, base::DictionaryValue::Get(...) is not marked as const. So compiler will complain if I add a const to verdict_entry. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:225: if (result) { On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > nit: > > if (!result) { > return false; > } > > DCHECK() > ... > > > > // This reduces the code indent and makes the exit condition very clear. Done. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:303: if (verdict) { On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > should this verdict check be a DCHECK instead? > This is a private method so verdict == null seems like a programmer mistake in > calling this method. > > The call site also has a DCHECK(verdict) already. you're right. No need to check here. Done. https://codereview.chromium.org/2747313002/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:306: // Because DictionaryValue cannot take none-UTF8 string, we need to store On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > non Done.
lgtm Thanks! On Mar 20, 2017 12:05, <jialiul@chromium.org> wrote: > Thanks vakh@! > > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/csd.proto > File components/safe_browsing/csd.proto (right): > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/csd.proto#newcode261 > components/safe_browsing/csd.proto:261: // > "foo.com/bar/abc/edf.cgi" > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > Can you also make it explicit that it won't match http://foo.com > either? > > Sure. Done. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc > File > components/safe_browsing/password_protection/password_ > protection_service.cc > (right): > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode104 > components/safe_browsing/password_protection/password_ > protection_service.cc:104: > DCHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion( > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > This statement seems to be modifying verdict_entry. > > It seems to me that this shouldn't be a DCHECK since this would be > compiled out > > on a release build. > > Changed to CHECK instead. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode108 > components/safe_browsing/password_protection/password_ > protection_service.cc:108: > DCHECK(ParseVerdictEntry(verdict_entry, &verdict_received_time, > &verdict)); > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > Same here. > > Done. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode174 > components/safe_browsing/password_protection/password_ > protection_service.cc:174: > return nullptr; > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > Is this nullptr intentional? > > If so, why have this function? > > Is it missing a TODO? > Yes, TODO is in the .h file. Added a corresponding TODO here too. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode217 > components/safe_browsing/password_protection/password_ > protection_service.cc:217: > base::DictionaryValue* verdict_entry, > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > const verdict_entry > Acknowledged. for some weird reason, base::DictionaryValue::Get(...) is > not > marked as const. So compiler will complain if I add a const to > verdict_entry. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode225 > components/safe_browsing/password_protection/password_ > protection_service.cc:225: > if (result) { > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > nit: > > > > if (!result) { > > return false; > > } > > > > DCHECK() > > ... > > > > > > > > // This reduces the code indent and makes the exit condition very > clear. > > Done. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode303 > components/safe_browsing/password_protection/password_ > protection_service.cc:303: > if (verdict) { > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > should this verdict check be a DCHECK instead? > > This is a private method so verdict == null seems like a programmer > mistake in > > calling this method. > > > > The call site also has a DCHECK(verdict) already. > > you're right. No need to check here. Done. > > https://codereview.chromium.org/2747313002/diff/180001/ > components/safe_browsing/password_protection/password_ > protection_service.cc#newcode306 > components/safe_browsing/password_protection/password_ > protection_service.cc:306: > // Because DictionaryValue cannot take none-UTF8 string, we need to > store > On 2017/03/20 18:36:00, vakh (Varun Khaneja) wrote: > > non > > Done. > > https://codereview.chromium.org/2747313002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
DEPS on components/history/core lgtm
Based on our discussion this morning, keep the password protection content settings at the REQUESTING_ORIGIN_ONLY_SCOPE. And use hostname (ignoring scheme, port) as its effective matching pattern.
The CQ bit was checked by jialiul@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpz@chromium.org, raymes@chromium.org, nparker@chromium.org, battre@chromium.org, sdefresne@chromium.org, vakh@chromium.org Link to the patchset: https://codereview.chromium.org/2747313002/#ps220001 (title: "Cache verdict based on hostname")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490130357709320,
"parent_rev": "9eac47f0718d298884db899138e463da03a9ee7c", "commit_rev":
"64fc03ab5fd993cc6573fa24266d177aaf8b1679"}
Message was sent while issue was closed.
Description was changed from ========== PasswordProtectionService verdict cache management Add functionalities to PasswordProtectionService to: (1) cache pps verdict based on origin and cache expression in HostContentSettingsMap (2) Get cached verdict from HostContentSettingsMap (3) Remove cached verdict upon URLs deleted from history service. BUG=698899 ========== to ========== PasswordProtectionService verdict cache management Add functionalities to PasswordProtectionService to: (1) cache pps verdict based on origin and cache expression in HostContentSettingsMap (2) Get cached verdict from HostContentSettingsMap (3) Remove cached verdict upon URLs deleted from history service. BUG=698899 Review-Url: https://codereview.chromium.org/2747313002 Cr-Commit-Position: refs/heads/master@{#458565} Committed: https://chromium.googlesource.com/chromium/src/+/64fc03ab5fd993cc6573fa24266d... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/64fc03ab5fd993cc6573fa24266d... |
