Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895233002:
Measure how often HTTPS credentials cannot be filled into HTTP forms. (Closed)
DescriptionMeasure how often HTTPS credentials cannot be filled into HTTP forms.
When no matching HTTP credentials exist for a non-secure origin, but there are
credentials for the HTTPS version of that origin (which are obviously not filled
and hence called `suppressed` credentials), that could indicate:
-- a premature `move-to-HTTPS` migration,
-- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP,
-- some of its sign-in forms are served over HTTPS, some over HTTP.
BUG=720599
Review-Url: https://codereview.chromium.org/2895233002
Cr-Commit-Position: refs/heads/master@{#474708}
Committed: https://chromium.googlesource.com/chromium/src/+/3f5e66ed12f91be9c51916fa72f59e98cc927d1b
Patch Set 1 #Patch Set 2 : Polish. #Patch Set 3 : More polish. #
Total comments: 10
Patch Set 4 : Addressed all comments. #Patch Set 5 : Rebase. #Patch Set 6 : Fix password_manager_unittests. #
Total comments: 2
Patch Set 7 : Update tests to reflect crrev.com/474284. #Patch Set 8 : Fix histogram suffixes. #Patch Set 9 : Fix UAF in unittest. #
Dependent Patchsets: Messages
Total messages: 40 (26 generated)
The CQ bit was checked by engedy@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 ========== Measure how often HTTPS credentials cannot be filled on HTTP sites. When no matching HTTP credentials exist for a non-secure origin, but there are suppressed HTTPS credentials, that could indicate a premature `move-to-HTTPS` migration, or simply that the site serves its sign-up or some of its sign-in forms over HTTPS, while others still over HTTP. BUG=720599 ========== to ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are suppressed HTTPS credentials, that could indicate a premature `move-to-HTTPS` migration, or simply that the site serves its sign-up or some of its sign-in forms over HTTPS, while others still over HTTP. BUG=720599 ==========
engedy@chromium.org changed reviewers: + dvadym@chromium.org, kolos@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are suppressed HTTPS credentials, that could indicate a premature `move-to-HTTPS` migration, or simply that the site serves its sign-up or some of its sign-in forms over HTTPS, while others still over HTTP. BUG=720599 ========== to ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are credentials for the HTTPS version of that origin, which are obviusly not filled (hence called `suppressed` credentials), that could indicate: -- a premature `move-to-HTTPS` migration, -- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP, -- some of its sign-in forms are served over HTTPS, some over HTTP. BUG=720599 ==========
Description was changed from ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are credentials for the HTTPS version of that origin, which are obviusly not filled (hence called `suppressed` credentials), that could indicate: -- a premature `move-to-HTTPS` migration, -- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP, -- some of its sign-in forms are served over HTTPS, some over HTTP. BUG=720599 ========== to ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are credentials for the HTTPS version of that origin (which are obviously not filled and hence called `suppressed` credentials), that could indicate: -- a premature `move-to-HTTPS` migration, -- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP, -- some of its sign-in forms are served over HTTPS, some over HTTP. BUG=720599 ==========
engedy@chromium.org changed reviewers: + rkaplow@chromium.org
@Maxim, Vadym, please take a look at the whole CL. @Robert, please take a look at t/m/histograms.
LGTM with two comments. https://codereview.chromium.org/2895233002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2895233002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:336: // it was not filled because it had an origin that almost, but not quite I believe "that almost, but not quite matched" is not a good explanation. Please clarify what it means technically. https://codereview.chromium.org/2895233002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:339: enum SuppressedAccountType { SuppressedAccountType => SuppressedAccountMatch. WDYT?
LGTM, thanks!
@Robert, friendly ping.
https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50178: +<histogram name="PasswordManager.SuppressedAccountsWereChecked" enum="Boolean"> is this supposed to be QueryingSuppressedAccountsFinished? https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50183: + PasswordManager.SuppressedAccount. histogram samples could be computed. extra . https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90502: +<histogram_suffixes name="PasswordManagerSuppressedAccountReason"> unclear to me why this is needed if there is only a single prefix. Are you planning to extend this?
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
@Robert, please take another look. https://codereview.chromium.org/2895233002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2895233002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:336: // it was not filled because it had an origin that almost, but not quite On 2017/05/23 09:50:11, kolos1 wrote: > I believe "that almost, but not quite matched" is not a good explanation. Please > clarify what it means technically. Done. https://codereview.chromium.org/2895233002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:339: enum SuppressedAccountType { On 2017/05/23 09:50:11, kolos1 wrote: > SuppressedAccountType => SuppressedAccountMatch. WDYT? Actually, how about SuppressedAccountExistence? https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50178: +<histogram name="PasswordManager.SuppressedAccountsWereChecked" enum="Boolean"> On 2017/05/23 20:13:43, rkaplow wrote: > is this supposed to be QueryingSuppressedAccountsFinished? Hmm, yes. Fixed. How did the PRESUBMIT check not catch this? https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50183: + PasswordManager.SuppressedAccount. histogram samples could be computed. On 2017/05/23 20:13:43, rkaplow wrote: > extra . Done. https://codereview.chromium.org/2895233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90502: +<histogram_suffixes name="PasswordManagerSuppressedAccountReason"> On 2017/05/23 20:13:43, rkaplow wrote: > unclear to me why this is needed if there is only a single prefix. Are you > planning to extend this? Yep, in a follow-up CL, and I wanted to start using the final names for this histogram already. Would you rather I just hard-coded this prefix into the histogram name for now?
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_tsan_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 engedy@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_tsan_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 engedy@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...
lgtm https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:90841: + <affected-histogram name="PasswordManager.SuppressedAccount"/> this should have both Generated and Manual here
https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:90841: + <affected-histogram name="PasswordManager.SuppressedAccount"/> On 2017/05/24 17:40:37, rkaplow wrote: > this should have both Generated and Manual here Do you mean this? <histogram_suffixes name="PasswordManagerSuppressedAccountReason"> <suffix name="HTTPSNotHTTP" label="The credential was suppressed because it was for an HTTPS origin whereas the observed form was for an HTTP origin."/> <affected-histogram name="PasswordManager.SuppressedAccount.Generated"/> <affected-histogram name="PasswordManager.SuppressedAccount.Manual"/> </histogram_suffixes> That gives me a PRESUBMIT check failure (regardless of the order of suffix definitions): ERROR:root:histogram_suffixes PasswordManagerSuppressedAccountReason is missing its dependency PasswordManager.SuppressedAccount.Generated
On 2017/05/24 17:46:59, engedy wrote: > https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:90841: + <affected-histogram > name="PasswordManager.SuppressedAccount"/> > On 2017/05/24 17:40:37, rkaplow wrote: > > this should have both Generated and Manual here > > Do you mean this? > > <histogram_suffixes name="PasswordManagerSuppressedAccountReason"> > <suffix name="HTTPSNotHTTP" > label="The credential was suppressed because it was for an HTTPS origin > whereas the observed form was for an HTTP origin."/> > <affected-histogram name="PasswordManager.SuppressedAccount.Generated"/> > <affected-histogram name="PasswordManager.SuppressedAccount.Manual"/> > </histogram_suffixes> > > That gives me a PRESUBMIT check failure (regardless of the order of suffix > definitions): > > ERROR:root:histogram_suffixes PasswordManagerSuppressedAccountReason is missing > its dependency PasswordManager.SuppressedAccount.Generated Couldn't see it until asvitkine@ pointed it out to me - there's a seperator missing, and the default separator is not '.' so it won't match. Try with seperators.
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org, dvadym@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2895233002/#ps140001 (title: "Fix histogram suffixes.")
On 2017/05/24 18:05:15, rkaplow wrote: > On 2017/05/24 17:46:59, engedy wrote: > > > https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2895233002/diff/100001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:90841: + <affected-histogram > > name="PasswordManager.SuppressedAccount"/> > > On 2017/05/24 17:40:37, rkaplow wrote: > > > this should have both Generated and Manual here > > > > Do you mean this? > > > > <histogram_suffixes name="PasswordManagerSuppressedAccountReason"> > > <suffix name="HTTPSNotHTTP" > > label="The credential was suppressed because it was for an HTTPS origin > > whereas the observed form was for an HTTP origin."/> > > <affected-histogram name="PasswordManager.SuppressedAccount.Generated"/> > > <affected-histogram name="PasswordManager.SuppressedAccount.Manual"/> > > </histogram_suffixes> > > > > That gives me a PRESUBMIT check failure (regardless of the order of suffix > > definitions): > > > > ERROR:root:histogram_suffixes PasswordManagerSuppressedAccountReason is > missing > > its dependency PasswordManager.SuppressedAccount.Generated > > Couldn't see it until asvitkine@ pointed it out to me - there's a seperator > missing, and the default separator is not '.' so it won't match. > Try with seperators. Ahh, great catch, thanks. Fixed!
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_asan_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 engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org, rkaplow@chromium.org, dvadym@chromium.org Link to the patchset: https://codereview.chromium.org/2895233002/#ps160001 (title: "Fix UAF in unittest.")
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": 160001, "attempt_start_ts": 1495730759339010,
"parent_rev": "e7e2da67f4d4193974751e59ab69325bb72195aa", "commit_rev":
"3f5e66ed12f91be9c51916fa72f59e98cc927d1b"}
Message was sent while issue was closed.
Description was changed from ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are credentials for the HTTPS version of that origin (which are obviously not filled and hence called `suppressed` credentials), that could indicate: -- a premature `move-to-HTTPS` migration, -- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP, -- some of its sign-in forms are served over HTTPS, some over HTTP. BUG=720599 ========== to ========== Measure how often HTTPS credentials cannot be filled into HTTP forms. When no matching HTTP credentials exist for a non-secure origin, but there are credentials for the HTTPS version of that origin (which are obviously not filled and hence called `suppressed` credentials), that could indicate: -- a premature `move-to-HTTPS` migration, -- the site serves its sign-up form over HTTPS, while sign-in forms over HTTP, -- some of its sign-in forms are served over HTTPS, some over HTTP. BUG=720599 Review-Url: https://codereview.chromium.org/2895233002 Cr-Commit-Position: refs/heads/master@{#474708} Committed: https://chromium.googlesource.com/chromium/src/+/3f5e66ed12f91be9c51916fa72f5... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/3f5e66ed12f91be9c51916fa72f5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
