Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773483003:
Create PasswordProtectionRequest to handle password pings (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, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, timvolodine, vakh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate PasswordProtectionRequest to handle password pings
PasswordProtectionRequest will check the following conditions to
determin if a ping will be sent out:
(1) only send ping with SBER user (for now)
(2) only send ping if not in incognito mode (for now)
(3) only send ping for non-whitelisted URL
(4) only send ping if no cached verdict available
BUG=698899
Review-Url: https://codereview.chromium.org/2773483003
Cr-Commit-Position: refs/heads/master@{#459611}
Committed: https://chromium.googlesource.com/chromium/src/+/1ac305b92081fe7f56daa698d738a040317d9785
Patch Set 1 #Patch Set 2 : fix BUILD file #Patch Set 3 : histogram #Patch Set 4 : fix SB cookie test #
Total comments: 58
Patch Set 5 : address nparker's comments #Patch Set 6 : nits #
Total comments: 12
Patch Set 7 : Add one more unittest #
Total comments: 15
Patch Set 8 : Address lpz's comments #Messages
Total messages: 49 (36 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...)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Description was changed from ========== Create PasswordProtectionRequest to handle password pings PasswordProtectionRequest will check the following conditions to determin if a ping will send out: (1) only send with SBER user (for now) (2) only send if not in incognito mode (for now) (3) only send for non-whitelisted URL (4) only send if no cached verdict available BUG=698899 ========== to ========== Create PasswordProtectionRequest to handle password pings PasswordProtectionRequest will check the following conditions to determin if a ping will be sent out: (1) only send ping with SBER user (for now) (2) only send ping if not in incognito mode (for now) (3) only send ping for non-whitelisted URL (4) only send ping if no cached verdict available BUG=698899 ==========
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
jialiul@chromium.org changed reviewers: + lpz@chromium.org, nparker@chromium.org
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Exciting! Looks like a good architecture to me. I haven't looked at the tests yet. https://codereview.chromium.org/2773483003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2773483003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:381: password_protection_service_.reset(); Maybe this should go before services_delegate since that way it's the opposite order of creation. PPS might depend on things in services_delegate_. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:62: if (fetcher_.get()) { nit: You can .reset() unconditionally https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:67: Finish(timed_out ? TIMEDOUT : CANCELED, nullptr); Is the expectation that Cancel() be followed by destroying this object? Otherwise, a pending CheckCsdWhitelistOnIOThread could return and call Finish a second time. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:74: // If request is not done withing 10 milliseconds, we cancel this request. nit: 10 seconds https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:93: base::TimeTicks::Now() - timeout_start_time_); Is this recorded time any different from PasswordProtection.RequestNetworkDuration? https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:97: if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) { How about a switch statement with no default, to ensure we have handled all possible verdicts? As I understand it, if the server sends a verdict with an enum value that Chrome doesn't know about, the proto will fail to unpack, so then we shouldn't expect any weird values here. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:97: if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) { I thought this was going to be LOW_REPUTATION -- maybe we changed plans after the proto was writte. Should we rename it? (but also ensure it matches the histogram names) https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:118: if (!password_protection_service_) { Is this possible? The PPS is shut down on the UI thread, so it can't be destroyed between Start() and here. I think this is a safe habit, but it's good to be clear on the flow -- this could be a DCHECK if it shouldn't be possible. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:162: Finish(RequestOutcome::FETCH_FAILED, nullptr); Add todo: log to UMA the httpResponseOrErrorCode like we do elsewhere https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:184: if (!password_protection_service_) { This was already checked in CheckCachedVerdicts, w/o intervening thread hops, so it should still be valid. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:21: // The outcome of the request. These values are used for UMA. I'd attach values to each of these, so you can compare them to histograms.xml values. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:24: UNKOWN, UNKNOWN https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:51: Some comments on these functions would be useful here. Does this send the request? And does cancel call Finish? https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:68: GURL main_frame_url() const { return main_frame_url_; } Could any of these be private? https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:30: // Request times out in 10 ms. nit: The comment here doesn't add anything. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:31: const int kRequestTimeoutMS = 10; Should be 10000 ms. Or you could call it seconds and leave it 10. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:155: // If it doesn't require exact match, we need to fine the most specific nit: find https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:268: stored_verdict_counts_[content_setting_map] = 0U; I think this should go above the if(), so next time this is called you'll have the 0 cached. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:272: base::DictionaryValue::From(content_setting_map->GetWebsiteSetting( Depending on how much work this function does, this could be an expensive loop. Is there any way to iterate once through all the URLs, rather than grabbing all the URLs and then looking them up one at a time? I assume it'll be run ~once when the user visits their first login page so it's probably not too bad. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:337: stored_verdict_counts_[setting_map] = GetStoredVerdictCount() - verdict_cnt; Is it possible for the setting_map here to differ from the one used in GetStoredVerdictCount? https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:341: DCHECK_LE(0U, stored_verdict_counts_[setting_map]); This is guaranteed to never fire, since they're unsigned. Instead, you could DCHECK before you subtract up above. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:38: scoped_refptr<net::URLRequestContextGetter> request_content_getter); nit: request_context_getter https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:54: // any thread. The orig comment on VERDICT_TYPE_UNSPECIFIED still seems useful https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:67: void StartRequest(const GURL& main_frame_url, Add comment. (Is this only used after we check the cache?) https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:161: // The context we use to issue network requests. (Could add a note that we do this because we need to use the Safe Browisng cookie store.) https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:166: std::unordered_map<PasswordProtectionRequest*, How about just a vector of unique_ptrs? If we assume it's uncommon to have more than one, it'd be more efficient and simpler code.
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 #5 (id:100001) has been deleted
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Thanks nparker! https://codereview.chromium.org/2773483003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2773483003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.cc:381: password_protection_service_.reset(); On 2017/03/23 at 20:45:49, Nathan Parker wrote: > Maybe this should go before services_delegate since that way it's the opposite order of creation. PPS might depend on things in services_delegate_. Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:62: if (fetcher_.get()) { On 2017/03/23 at 20:45:50, Nathan Parker wrote: > nit: You can .reset() unconditionally Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:67: Finish(timed_out ? TIMEDOUT : CANCELED, nullptr); On 2017/03/23 at 20:45:50, Nathan Parker wrote: > Is the expectation that Cancel() be followed by destroying this object? Otherwise, a pending CheckCsdWhitelistOnIOThread could return and call Finish a second time. Yes, and Finish(..) will destroy this object by asking PasswordProtectionService to reset its corresponding unique_ptr, therefore the weakptr bind to OnWhitelistCheckDone will be invalided and call dropped. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:74: // If request is not done withing 10 milliseconds, we cancel this request. On 2017/03/23 at 20:45:49, Nathan Parker wrote: > nit: 10 second Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:93: base::TimeTicks::Now() - timeout_start_time_); On 2017/03/23 at 20:45:49, Nathan Parker wrote: > Is this recorded time any different from PasswordProtection.RequestNetworkDuration? NetworkDuration measures the time from SendRequest() to OnURLFetchComplete(). So if request is canceled due to whitlist match, cached verdict, etc, we don't log this NetworkDuration. RequestDuration includes everything from Start() to Finish(), the time it measures include whitelist checking, cache checking and other things. And it is logged for every single request no matter it is successful or not. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:97: if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) { On 2017/03/23 at 20:45:50, Nathan Parker wrote: > How about a switch statement with no default, to ensure we have handled all possible verdicts? > > As I understand it, if the server sends a verdict with an enum value that Chrome doesn't know about, the proto will fail to unpack, so then we shouldn't expect any weird values here. Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:97: if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) { On 2017/03/23 20:45:50, Nathan Parker wrote: > I thought this was going to be LOW_REPUTATION -- maybe we changed plans after > the proto was writte. Should we rename it? (but also ensure it matches the > histogram names) UNFAMILIAR_LOGIN_PAGE is the request trigger type (a.k.a whether it is triggered by unfamiliar login page or password reuse event). LOW_REPUTATION is a verdict (together with SAFE, and PHISHING). Here I log verdicts from different trigger type separately. PasswordProtection.UnfamiliarLoginPageVerdict -- SAFE -- PHISHING -- LOW_REPUTATION PasswordProtection.PasswordReuseEventVerdict -- SAFE -- PHISHING -- LOW_REPUTATION https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:118: if (!password_protection_service_) { On 2017/03/23 at 20:45:50, Nathan Parker wrote: > Is this possible? The PPS is shut down on the UI thread, so it can't be destroyed between Start() and here. I think this is a safe habit, but it's good to be clear on the flow -- this could be a DCHECK if it shouldn't be possible. Just a safe measure, since PPS is a weakptr. But in reality it shouldn't happen. Let me change it to DCHECK. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:162: Finish(RequestOutcome::FETCH_FAILED, nullptr); On 2017/03/23 at 20:45:50, Nathan Parker wrote: > Add todo: log to UMA the httpResponseOrErrorCode like we do elsewhere Done. UMA metrics added. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:184: if (!password_protection_service_) { On 2017/03/23 at 20:45:50, Nathan Parker wrote: > This was already checked in CheckCachedVerdicts, w/o intervening thread hops, so it should still be valid. Right. removed this check. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:21: // The outcome of the request. These values are used for UMA. On 2017/03/23 at 20:45:50, Nathan Parker wrote: > I'd attach values to each of these, so you can compare them to histograms.xml values. Agree. Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:24: UNKOWN, On 2017/03/23 at 20:45:50, Nathan Parker wrote: > UNKNOWN er..... second time... Fixed. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:51: On 2017/03/23 at 20:45:50, Nathan Parker wrote: > Some comments on these functions would be useful here. Does this send the request? And does cancel call Finish? Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:68: GURL main_frame_url() const { return main_frame_url_; } On 2017/03/23 at 20:45:50, Nathan Parker wrote: > Could any of these be private? Yes, moved a bunch of them into private. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:30: // Request times out in 10 ms. On 2017/03/23 20:45:51, Nathan Parker wrote: > nit: The comment here doesn't add anything. Removed. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:31: const int kRequestTimeoutMS = 10; On 2017/03/23 20:45:50, Nathan Parker wrote: > Should be 10000 ms. Or you could call it seconds and leave it 10. Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:155: // If it doesn't require exact match, we need to fine the most specific On 2017/03/23 20:45:51, Nathan Parker wrote: > nit: find Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:268: stored_verdict_counts_[content_setting_map] = 0U; On 2017/03/23 20:45:51, Nathan Parker wrote: > I think this should go above the if(), so next time this is called you'll have > the 0 cached. Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:272: base::DictionaryValue::From(content_setting_map->GetWebsiteSetting( On 2017/03/23 20:45:50, Nathan Parker wrote: > Depending on how much work this function does, this could be an expensive loop. > Is there any way to iterate once through all the URLs, rather than grabbing all > the URLs and then looking them up one at a time? > > I assume it'll be run ~once when the user visits their first login page so it's > probably not too bad. Acknowledged. Unfortunately there is no function exposed by HostContentSettingsMap to get everything at once. That's why I choose to cache this counts in a map. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:337: stored_verdict_counts_[setting_map] = GetStoredVerdictCount() - verdict_cnt; On 2017/03/23 20:45:51, Nathan Parker wrote: > Is it possible for the setting_map here to differ from the one used in > GetStoredVerdictCount? setting_map is per profile. My next CL will make sure GetStoredVerdictCount() will get the correct profile. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:341: DCHECK_LE(0U, stored_verdict_counts_[setting_map]); On 2017/03/23 20:45:51, Nathan Parker wrote: > This is guaranteed to never fire, since they're unsigned. Instead, you could > DCHECK before you subtract up above. Actually I don't need this DCHECK, since size_t type cannot take negative number. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:38: scoped_refptr<net::URLRequestContextGetter> request_content_getter); On 2017/03/23 20:45:51, Nathan Parker wrote: > nit: request_context_getter Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:54: // any thread. On 2017/03/23 20:45:51, Nathan Parker wrote: > The orig comment on VERDICT_TYPE_UNSPECIFIED still seems useful Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:67: void StartRequest(const GURL& main_frame_url, On 2017/03/23 20:45:51, Nathan Parker wrote: > Add comment. (Is this only used after we check the cache?) Actual no, Cache checking is a part of PasswordProtectionRequest logic. This function creates a new PassProtectionRequest instance and call PassProtectionRequest::Start(). https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:161: // The context we use to issue network requests. On 2017/03/23 20:45:51, Nathan Parker wrote: > (Could add a note that we do this because we need to use the Safe Browisng > cookie store.) Done. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:166: std::unordered_map<PasswordProtectionRequest*, On 2017/03/23 20:45:51, Nathan Parker wrote: > How about just a vector of unique_ptrs? If we assume it's uncommon to have more > than one, it'd be more efficient and simpler code. Yep, using a std::unordered_set instead.
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: 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...
Looks great! A few small comments, otherwise LGTM One thought: I assume there won't be normally a case where we'd send multiple requests for the same page or origin, but it's something to be aware of. Several could be in flight at the same time if I open multiple tabs to, say, different URLs on a site that all have a login form up in the corner. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:67: Finish(timed_out ? TIMEDOUT : CANCELED, nullptr); On 2017/03/23 22:42:59, Jialiu Lin wrote: > On 2017/03/23 at 20:45:50, Nathan Parker wrote: > > Is the expectation that Cancel() be followed by destroying this object? > Otherwise, a pending CheckCsdWhitelistOnIOThread could return and call Finish a > second time. > > Yes, and Finish(..) will destroy this object by asking PasswordProtectionService > to reset its corresponding unique_ptr, therefore the weakptr bind to > OnWhitelistCheckDone will be invalided and call dropped. Can you add a comment on Cancel() in the .h to that effect? https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:93: base::TimeTicks::Now() - timeout_start_time_); On 2017/03/23 22:42:59, Jialiu Lin wrote: > On 2017/03/23 at 20:45:49, Nathan Parker wrote: > > Is this recorded time any different from > PasswordProtection.RequestNetworkDuration? > > NetworkDuration measures the time from SendRequest() to OnURLFetchComplete(). So > if request is canceled due to whitlist match, cached verdict, etc, we don't log > this NetworkDuration. > > RequestDuration includes everything from Start() to Finish(), the time it > measures include whitelist checking, cache checking and other things. And it is > logged for every single request no matter it is successful or not. I suspect the only delay > 1 ms is the network call (whitelist check is synchronous, other than the thread scheduling). If you want to measure performance of the bits you mentioned, you might need better than 1 ms resolution. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:97: if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) { On 2017/03/23 22:42:59, Jialiu Lin wrote: > On 2017/03/23 20:45:50, Nathan Parker wrote: > > I thought this was going to be LOW_REPUTATION -- maybe we changed plans after > > the proto was writte. Should we rename it? (but also ensure it matches the > > histogram names) > > UNFAMILIAR_LOGIN_PAGE is the request trigger type (a.k.a whether it is triggered > by unfamiliar login page or password reuse event). LOW_REPUTATION is a verdict > (together with SAFE, and PHISHING). Here I log verdicts from different trigger > type separately. > PasswordProtection.UnfamiliarLoginPageVerdict > -- SAFE > -- PHISHING > -- LOW_REPUTATION > PasswordProtection.PasswordReuseEventVerdict > -- SAFE > -- PHISHING > -- LOW_REPUTATION AH yes, nm. https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:341: DCHECK_LE(0U, stored_verdict_counts_[setting_map]); On 2017/03/23 22:43:02, Jialiu Lin wrote: > On 2017/03/23 20:45:51, Nathan Parker wrote: > > This is guaranteed to never fire, since they're unsigned. Instead, you could > > DCHECK before you subtract up above. > > Actually I don't need this DCHECK, since size_t type cannot take negative > number. ok, but you could DCHECK(GetStoredVerdictCount() >= verdict_cnt) above, if you're concerned they could end up with different counts. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:158: if (source->GetStatus().is_success()) { Can put both error code and http status in the same metric? That's handy since it counts all requests then. Something like: https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/protocol_ma... https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:36: MAX_OUTCOME Don't forget histograms.xml https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:69: // Start a timeout to cancel the request if it takes too long. nit: s/timeout/timer https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:72: // |this| will be gone after calling this function. s/gone/destroyed https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:90: bool is_extended_reporting_; Can some of these be const? https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:584: } Can you test tearing down when there are pending requests?
Patchset #7 (id:160001) has been deleted
jialiul@chromium.org changed reviewers: + holte@chromium.org, zhongyi@chromium.org
Thanks nparker! +holte@ for reviewing histograms.xml +zhongyi@ for owner review (adding "+net" to components/safe_browsing/password_protection/DEPS) https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:67: Finish(timed_out ? TIMEDOUT : CANCELED, nullptr); On 2017/03/24 00:41:50, Nathan Parker wrote: > On 2017/03/23 22:42:59, Jialiu Lin wrote: > > On 2017/03/23 at 20:45:50, Nathan Parker wrote: > > > Is the expectation that Cancel() be followed by destroying this object? > > Otherwise, a pending CheckCsdWhitelistOnIOThread could return and call Finish > a > > second time. > > > > Yes, and Finish(..) will destroy this object by asking > PasswordProtectionService > > to reset its corresponding unique_ptr, therefore the weakptr bind to > > OnWhitelistCheckDone will be invalided and call dropped. > > Can you add a comment on Cancel() in the .h to that effect? Already did. :-) https://codereview.chromium.org/2773483003/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:93: base::TimeTicks::Now() - timeout_start_time_); On 2017/03/24 00:41:50, Nathan Parker wrote: > On 2017/03/23 22:42:59, Jialiu Lin wrote: > > On 2017/03/23 at 20:45:49, Nathan Parker wrote: > > > Is this recorded time any different from > > PasswordProtection.RequestNetworkDuration? > > > > NetworkDuration measures the time from SendRequest() to OnURLFetchComplete(). > So > > if request is canceled due to whitlist match, cached verdict, etc, we don't > log > > this NetworkDuration. > > > > RequestDuration includes everything from Start() to Finish(), the time it > > measures include whitelist checking, cache checking and other things. And it > is > > logged for every single request no matter it is successful or not. > > I suspect the only delay > 1 ms is the network call (whitelist check is > synchronous, other than the thread scheduling). If you want to measure > performance of the bits you mentioned, you might need better than 1 ms > resolution. you're right. The differences will be tiny. Let me remove this metrics. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:158: if (source->GetStatus().is_success()) { On 2017/03/24 00:41:50, Nathan Parker wrote: > Can put both error code and http status in the same metric? That's handy since > it counts all requests then. Something like: > > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/protocol_ma... Done. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:36: MAX_OUTCOME On 2017/03/24 00:41:50, Nathan Parker wrote: > Don't forget histograms.xml histograms enums are always numbered. :-) https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:69: // Start a timeout to cancel the request if it takes too long. On 2017/03/24 00:41:50, Nathan Parker wrote: > nit: s/timeout/timer Done. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:72: // |this| will be gone after calling this function. On 2017/03/24 00:41:50, Nathan Parker wrote: > s/gone/destroyed Done. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:90: bool is_extended_reporting_; On 2017/03/24 00:41:50, Nathan Parker wrote: > Can some of these be const? Oh yes! Done. https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service_unittest.cc (right): https://codereview.chromium.org/2773483003/diff/140001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service_unittest.cc:584: } On 2017/03/24 00:41:50, Nathan Parker wrote: > Can you test tearing down when there are pending requests? Done.
lgtm https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:55: StartTimeout(); This timer will also count the time to look at the local db and cache, not just the remote request, right? Just checking if that's intended. I guess one possible benefit of timing just the SendRequest call is that it's easier to correlate to SLA params on the backend? https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:80: void PasswordProtectionRequest::Finish( nit: reorder these functions to more-closely match the code flow https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:173: base::TimeTicks::Now() - request_start_time_); ...further to my other comment, this will also include the local processing time, not solely the network time? https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:19: class PasswordProtectionRequest : public net::URLFetcherDelegate { add class comments nit: and maybe doc the rest of the functions and members too https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:30: const int kRequestTimeoutMs = 10000; general question: is there any benefit to tuning this on the fly, or is a constant usually good enough? https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:189: // Increases stored verdict count if we haven't seen this its cache expression nit:-its https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:340: size_t verdict_cnt = verdict_dictionary->size(); supernit: cnt->count
net lgtm
Thanks zhongyi@ and lpz@! https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:55: StartTimeout(); On 2017/03/24 15:40:48, lpz wrote: > This timer will also count the time to look at the local db and cache, not just > the remote request, right? Just checking if that's intended. I guess one > possible benefit of timing just the SendRequest call is that it's easier to > correlate to SLA params on the backend? Agree, it makes more sense to only time SendRequest. Moved this function down to SendRequest. https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:80: void PasswordProtectionRequest::Finish( On 2017/03/24 15:40:48, lpz wrote: > nit: reorder these functions to more-closely match the code flow Done. https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:173: base::TimeTicks::Now() - request_start_time_); On 2017/03/24 15:40:48, lpz wrote: > ...further to my other comment, this will also include the local processing > time, not solely the network time? request_start_time is set in SendRequest() function right before fetcher_->Start() is called. So it excludes local processing time. https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:19: class PasswordProtectionRequest : public net::URLFetcherDelegate { On 2017/03/24 15:40:48, lpz wrote: > add class comments > nit: and maybe doc the rest of the functions and members too Done. https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:30: const int kRequestTimeoutMs = 10000; On 2017/03/24 15:40:48, lpz wrote: > general question: is there any benefit to tuning this on the fly, or is a > constant usually good enough? Acknowledged. We use constants for other SB pings. Let's bring up this question to SB team next time to see if they have any thought. Thanks! https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:189: // Increases stored verdict count if we haven't seen this its cache expression On 2017/03/24 15:40:48, lpz wrote: > nit:-its Done. https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.cc:340: size_t verdict_cnt = verdict_dictionary->size(); On 2017/03/24 15:40:48, lpz wrote: > supernit: cnt->count Done.
https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:55: StartTimeout(); On 2017/03/24 18:10:18, Jialiu Lin wrote: > On 2017/03/24 15:40:48, lpz wrote: > > This timer will also count the time to look at the local db and cache, not > just > > the remote request, right? Just checking if that's intended. I guess one > > possible benefit of timing just the SendRequest call is that it's easier to > > correlate to SLA params on the backend? > > Agree, it makes more sense to only time SendRequest. Moved this function down to > SendRequest. ..and just one other thought - is it safe to assume that the db/cache operations won't block forever for some reason? That's one thing that the old approach would handle better since it could cancel those types of requests.
On 2017/03/24 18:55:58, lpz wrote: > https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... > File components/safe_browsing/password_protection/password_protection_request.cc > (right): > > https://codereview.chromium.org/2773483003/diff/180001/components/safe_browsi... > components/safe_browsing/password_protection/password_protection_request.cc:55: > StartTimeout(); > On 2017/03/24 18:10:18, Jialiu Lin wrote: > > On 2017/03/24 15:40:48, lpz wrote: > > > This timer will also count the time to look at the local db and cache, not > > just > > > the remote request, right? Just checking if that's intended. I guess one > > > possible benefit of timing just the SendRequest call is that it's easier to > > > correlate to SLA params on the backend? > > > > Agree, it makes more sense to only time SendRequest. Moved this function down > to > > SendRequest. > > ..and just one other thought - is it safe to assume that the db/cache operations > won't block forever for some reason? That's one thing that the old approach > would handle better since it could cancel those types of requests. The only db call here is a synchronized call, so db lookup should be fine. Not sure how HostContentSettingsMap handles corner cases. I'll investigate and address this in the next CL.
histograms lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, lpz@chromium.org, zhongyi@chromium.org Link to the patchset: https://codereview.chromium.org/2773483003/#ps200001 (title: "Address lpz's comments")
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": 200001, "attempt_start_ts": 1490398191962890,
"parent_rev": "015b482288344aacfd96680e234d064ad79e3635", "commit_rev":
"1ac305b92081fe7f56daa698d738a040317d9785"}
Message was sent while issue was closed.
Description was changed from ========== Create PasswordProtectionRequest to handle password pings PasswordProtectionRequest will check the following conditions to determin if a ping will be sent out: (1) only send ping with SBER user (for now) (2) only send ping if not in incognito mode (for now) (3) only send ping for non-whitelisted URL (4) only send ping if no cached verdict available BUG=698899 ========== to ========== Create PasswordProtectionRequest to handle password pings PasswordProtectionRequest will check the following conditions to determin if a ping will be sent out: (1) only send ping with SBER user (for now) (2) only send ping if not in incognito mode (for now) (3) only send ping for non-whitelisted URL (4) only send ping if no cached verdict available BUG=698899 Review-Url: https://codereview.chromium.org/2773483003 Cr-Commit-Position: refs/heads/master@{#459611} Committed: https://chromium.googlesource.com/chromium/src/+/1ac305b92081fe7f56daa698d738... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/1ac305b92081fe7f56daa698d738... |
