Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734863004:
wire PasswordProtectionService into PasswordReuseDetectionManager (Closed)
|
Created:
3 years, 9 months ago by Jialiu Lin Modified:
3 years, 9 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwire PasswordProtectionService into PasswordReuseDetectionManager
Wire PasswordProtectionService into PasswordReuseDetectionManager to
help log non-whitelisted password-reuses to UMA
BUG=691103
Review-Url: https://codereview.chromium.org/2734863004
Cr-Original-Commit-Position: refs/heads/master@{#455250}
Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23b1a193bbee83
Review-Url: https://codereview.chromium.org/2734863004
Cr-Commit-Position: refs/heads/master@{#455862}
Committed: https://chromium.googlesource.com/chromium/src/+/1d1cff37eca89741800bed9e0603bc20a79e186d
Patch Set 1 #Patch Set 2 : Add ifdefs for ios builds #Patch Set 3 : refine ifdefs #
Total comments: 8
Patch Set 4 : address comments from dvadym #Patch Set 5 : wire PasswordProtectionService into PasswordReuseDetectionManager #
Messages
Total messages: 45 (32 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 #3 (id:40001) has been deleted
jialiul@chromium.org changed reviewers: + dvadym@chromium.org
Hi dvadym@, I wired PasswordProtectionService into password manager related code through ChromePasswordManagerClient. Let me know if it makes sense to you. Some explanation of these #ifdef: Since safe browsing database is not available on ios, we need to exclude ios builds. In c++ files, this is achieved by using "#if defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)"; in the BUILD.gn file by using "if (!is_ios)". I'll gradually add more and more functions to PasswordProtectionService, and use a Finch feature to gate this new feature shortly.
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Hi Jialiu, this CL looks good. I have some small comments. https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:641: if (g_browser_process && g_browser_process->safe_browsing_service() && Nit: return g_browser_process && g_browser_process->...; https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.h:166: // password_reuse_detection_manager_. Nit: password_reuse_detection_manager_ -> |password_reuse_detection_manager_| https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:27: password_protection_service_ = nullptr; Nit: you can remove this line. WeekPtr's default constructor initialize it to nullptr. https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:77: password_protection_service_->RecordPasswordReuse(main_frame_url_); It looks like checking whether |password_protection_service_| is not null before call is required.
Thanks dvadym@! https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:641: if (g_browser_process && g_browser_process->safe_browsing_service() && On 2017/03/07 15:12:47, dvadym wrote: > Nit: return g_browser_process && g_browser_process->...; Done. https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2734863004/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.h:166: // password_reuse_detection_manager_. On 2017/03/07 15:12:48, dvadym wrote: > Nit: password_reuse_detection_manager_ -> |password_reuse_detection_manager_| Done. https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... File components/password_manager/core/browser/password_reuse_detection_manager.cc (right): https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:27: password_protection_service_ = nullptr; On 2017/03/07 15:12:48, dvadym wrote: > Nit: you can remove this line. WeekPtr's default constructor initialize it to > nullptr. Done. https://codereview.chromium.org/2734863004/diff/100001/components/password_ma... components/password_manager/core/browser/password_reuse_detection_manager.cc:77: password_protection_service_->RecordPasswordReuse(main_frame_url_); On 2017/03/07 15:12:48, dvadym wrote: > It looks like checking whether |password_protection_service_| is not null before > call is required. Done.
LGTM
The CQ bit was checked by jialiul@chromium.org
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": 110001, "attempt_start_ts": 1488911062049540,
"parent_rev": "84e8ccb78db8330156b6ad7211a186ac1530df25", "commit_rev":
"3b7571b5b5900a3d4582d62abe23b1a193bbee83"}
Message was sent while issue was closed.
Description was changed from ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 ========== to ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 Review-Url: https://codereview.chromium.org/2734863004 Cr-Commit-Position: refs/heads/master@{#455250} Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110001) as https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:110001) has been created in https://codereview.chromium.org/2738073002/ by jialiul@chromium.org. The reason for reverting is: This CL causes crashes crbug.com/699551.
Message was sent while issue was closed.
Description was changed from ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 Review-Url: https://codereview.chromium.org/2734863004 Cr-Commit-Position: refs/heads/master@{#455250} Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23... ========== to ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 Review-Url: https://codereview.chromium.org/2734863004 Cr-Commit-Position: refs/heads/master@{#455250} Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23... ==========
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: + vakh@chromium.org
Reopen this CL after revert. Previous revert was due to crashes on Pver4 population. The root cause is that PasswordProtectionService is initialized before safe browsing ServiceDelegate's initialization (a.k.a creation the creation of Pver4 database manager). This caused invalid access to undefined memory. The latest patch fixed the initialization sequence. +vakh@, could you take a look? Thanks!
Patchset #5 (id:130001) has been deleted
The CQ bit was checked by vakh@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org Link to the patchset: https://codereview.chromium.org/2734863004/#ps150001 (title: "wire PasswordProtectionService into PasswordReuseDetectionManager")
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
Try jobs failed on following builders: linux_chromium_chromeos_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
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": 150001, "attempt_start_ts": 1489091228132800,
"parent_rev": "464f699b05e37bcaf8de39e0aa6c8370ffe2511b", "commit_rev":
"1d1cff37eca89741800bed9e0603bc20a79e186d"}
Message was sent while issue was closed.
Description was changed from ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 Review-Url: https://codereview.chromium.org/2734863004 Cr-Commit-Position: refs/heads/master@{#455250} Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23... ========== to ========== wire PasswordProtectionService into PasswordReuseDetectionManager Wire PasswordProtectionService into PasswordReuseDetectionManager to help log non-whitelisted password-reuses to UMA BUG=691103 Review-Url: https://codereview.chromium.org/2734863004 Cr-Original-Commit-Position: refs/heads/master@{#455250} Committed: https://chromium.googlesource.com/chromium/src/+/3b7571b5b5900a3d4582d62abe23... Review-Url: https://codereview.chromium.org/2734863004 Cr-Commit-Position: refs/heads/master@{#455862} Committed: https://chromium.googlesource.com/chromium/src/+/1d1cff37eca89741800bed9e0603... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:150001) as https://chromium.googlesource.com/chromium/src/+/1d1cff37eca89741800bed9e0603... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
