Closed Bug 1120888 Opened 5 years ago Closed 5 years ago

Telemetry: Record the reasons auto-fill doesn't occur in a histogram

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
5

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

User Story

There are multiple reasons why we bail from auto-fill in _fillForm and we log many of them to the Browser Console. Record each reason in it's own bucket and making some more fine-grained will help track improvements to detection heuristics.

Examples:
* "Password not filled. None of the stored logins match the username already present." may be reduced when our username field heuristics improve.
* "Multiple logins for form, so not filling any." may be reduced when we improve our field detection heuristics as we will have less users having 2 logins for a site with different usernames (e.g. one email, one actual username) but with the same password.

We should leave room in the histogram to add more reasons in the future.

Attachments

(1 file, 1 obsolete file)

No description provided.
Flags: firefox-backlog+
Points: --- → 5
Flags: qe-verify-
Assignee: nobody → MattN+bmo
Priority: -- → P1
I have a WIP patch.
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Attached file MozReview Request: bz://1120888/MattN (obsolete) —
/r/3725 - Bug 1120888 - Record the result of password manager form autofill. r=dolske

Pull down this commit:

hg pull review -r c625619a7be962ecf642af95239102558aafc277
Attachment #8562534 - Flags: review?(dolske)
I manually tested all but 1 and 10. 1 would require scripting to remove the password field after DOMFormHasPassword which seems unlikely and 10 shouldn't happen unless we forget to add a didntFillReason in the future.

PWMGR_FORM_AUTOFILL_RESULT
38 samples, average = 4.7, sum = 177

 0 |########  2  5%
 2 |#########################  6  16%
 3 |#####################  5  13%
 4 |#####################  5  13%
 5 |#####################  5  13%
 6 |#####################  5  13%
 7 |#########################  6  16%
 8 |#############  3  8%
 9 |####  1  3%
10 |  0  0%
Hot shit, :MattN. I'm eager to see the results.
Attachment #8562534 - Flags: review?(dolske)
Comment on attachment 8562534 [details]
MozReview Request: bz://1120888/MattN

https://reviewboard.mozilla.org/r/3723/#review3147

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +    const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT");

Seems like the histogram fetch should just go into recordAutofillResult().

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +      recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD);

Let's add a log() here, just so our telemetry and console output help explain the same things. (There wasn't previously, because pre-DOMFormHasPassword fillForm() was called all the time on passwordless forms, now this basically should't ever happen.)

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);

Maybe worth a comment here that we're not log()ing, because this is the exceptionally common case of a login form without a saved login?

Also, we could probably just move this check up to the top of the function... If _fillForm() is called with foundLogins.length == 0, looks like we can just safely bail immediately without doing anything.

This would be good to do here, since otherwise the data will be a little skewed -- we'll be reporting NO_PASSWORD_FIELD and PASSWORD_DISABLED_READONLY for pages where the user doesn't even have a login to fill.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +      recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);

Maybe add a log() here too or in notifyFoundLogins()... Although now that I'm looking at this, I kinda think we should just kill notifyFoundLogins (in a separate bug). So maybe not bother with extra logging here.
Attachment #8562534 - Flags: review+
Comment on attachment 8562534 [details]
MozReview Request: bz://1120888/MattN

Approval Request Comment
[Feature/regressing bug #]: Improvements to password manager in 2015 - Bug 1121127
[User impact if declined]: Possibly less ideal decisions based on lack of data.
[Describe test coverage new/current, TreeHerder]: Existing password manager tests pass
[Risks and why]: Low risk since it's new code only run during the telemetry gathering phase.
[String/UUID change made/needed]: None
Attachment #8562534 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f39249695662
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Attachment #8562534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8562534 - Attachment is obsolete: true
Attachment #8619118 - Flags: review+
You need to log in before you can comment on or make changes to this bug.