New issue
Advanced search Search tips

Issue 910121 link

Starred by 0 users
Status: Assigned
Owner:
dvadym@chromium.org
Cc:
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Wrong data reported in UKM PasswordForm.ParsingComparison

Project Member Reported by vabr@chromium.org, Nov 29 
Chrome Version       : 72.0.3626.0

What steps will reproduce the problem?
1. Go to https://rsolomakhin.github.io/autofill/
2. Type something in the password field
3. Observe the reported value of UKM PasswordForm.ParsingComparison

What is the expected result?
The value should be kSame.

What happens instead of that?
It is not kSame.

To explain:

The metric should report differences in how the old and the new parser parsed the password form.
During filling, PasswordManager creates the NewPasswordFormManagers (NPFM) for observed forms, uses NewPasswordFormManager::set_old_parsing_result to store the old parser's result, and then the NPFM compare it in NewPasswordFormManager::RecordMetricOnCompareParsingResult with the new parser's result.
However, during saving (happens on the first keystroke, for the manual saving prompt), the NPFM is cloned from the old one, and the cloning misses copying the old parsing result. So in those cloned NPFM, the new parser is always compared to "no parser" instead of "old parser".

Comment 1 by vabr@chromium.org, Nov 29

Cc: -dvadym@chromium.org
Owner: dvadym@chromium.org
Status: Assigned (was: Started)
Good news is that no bad data actually hit the UKM server because of this:
The PasswordFormMetricsRecorder is shared between the original and the cloned NPFM. When the original NPFM calls RecordParsingsComparisonResult on the recorder, it stores the (correct) value in its UKM builder, which stores it in a base::flat_map, using emplace(), which boils down to base::flat_tree::emplace_key_args, which seems to not do anything if there already is a value stored with that key. So when later the cloned NPFM stores the wrong value, this is not sent over once the recorder is destroyed and contacts the UKM server.

I also verified this by looking at about:ukm.

It is still a mistake to call RecordMetricOnCompareParsingResult when NPFM::old_parsing_result_ is not set.

Once the related bug 910137 is fixed, I think it would be best to continue not setting old_parsing_result_ in Clone(), but to DCHECK that it is set when RecordMetricOnCompareParsingResult is called.

I leave this decision up to you, Vadym. Feel free to close this straight away if you don't think the DCHECK or any other action is needed, or keep it open to track the needed action.
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35260f389bcfaa55f1e63a8ccf0d37d76da166cb

commit 35260f389bcfaa55f1e63a8ccf0d37d76da166cb
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Fri Dec 28 10:51:03 2018

Stop sending parsing comparison metrics on manual fallback.

This CL stops sending parsing comparison metrics when |driver_| is null.
It can happen only for saving manual fallback case. Because of the
current logic these metrics in manual fallback case is broken anyway.

Bug: 831123, 910121

Change-Id: I214644e0e0fe5f82028fb1c03293e6c187e4b7fd
Reviewed-on: https://chromium-review.googlesource.com/c/1391679
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619142}
[modify] https://crrev.com/35260f389bcfaa55f1e63a8ccf0d37d76da166cb/components/password_manager/core/browser/new_password_form_manager.cc
►

Sign in to add a comment