1548878 - Add telemetry for when a generated password is first filled for a site

Status: VERIFIED FIXED

(Toolkit :: Password Manager, task, P2)

Description

[Matthew N. [:MattN]]

We want to know if people are using our generated passwords.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Tentative plan is to register a new UI event and record it (from the parent process)

category: pwmgr
methods: autocomplete_field
object: generatedpassword

.. then in bug 1548880 we can use something like method: edit_field, object: generatedpassword

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1548878 - Add telemetry for when a generated password is first filled. r?MattN

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: bug-1548878-data-review-form.md

This is just for the event telemetry for filling a field with a generated password. We have a different bug for user edits of that value (bug 1548880.) It goes without saying (but I'll say anyway) that the event recorded only records the fact that a generated password was filled, not what that password was.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19abd28b8dcd44326e7f8cfd7fa7ee6674519160

Comment 5

[Chris H-C :chutten]

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Load-balancing redirect to :tdsmith

Comment 6

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

* Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

The documentation in Events.yaml implies that an event is sent any time a login field is autocompleted with any content, which I don't think is the intent of this patch. Can you limit the description of the event to the data collection you're implementing in this bug?

Can you also clarify whether you intend to send an event the first time a generated password is filled (like the title of the bug suggests) or any time a generated password is filled (like the event description would suggest)?

Comment 7

[Sam Foster [:sfoster] (he/him)]

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #6)

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Thanks, I'm working on an updated patch which moves the event recording to the content process and I can clarify both these questions in the new patch.

• Is there or will there be documentation that describes the schema for the
  ultimate data set in a public, complete, and accurate way?

I'm happy to put this together. I'm not sure where it would live if not in the Events.yaml itself - do we have something similar for the in-product telemetry for other features I can look at?

Comment 8

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Great, thanks!

I think I created some confusion—the bulleted question is the stock question from the data collection review form that I was responding to; sorry! A sentence or two in the description string in Events.yaml is the right place and right amount of documentation; I just want to be sure that it matches the collection.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

The data review form hasn't changed, but the patch has. I've clarified in the description field in which circumstances we'll record this event. The significance of the "first filled" distinction is that we don't want to record 2 events when the result of selecting the generated password from the autocomplete dropdown is to fill 2 fields e.g a "new password" and "confirm password" inputs. 

Also changed: this event is now recorded from the main/parent process.

Comment 10

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Thanks!

1) Is there or will there be **documentation** that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, in Events.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, the [Firefox telemetry opt-out](https://support.mozilla.org/en-US/kb/share-data-mozilla-help-improve-firefox).

3) If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, sfoster.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, interaction data.

5) Is the data collection request for default-on or default-off?

Default-on.

6) Does the instrumentation include the addition of **any *new* identifiers**?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.


8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent collection.

9) Does the data collection use a third-party collection tool?

No.

—

result: data-review+

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/323de6d7d422
Add telemetry for when a generated password is first filled. r=MattN

Comment 12

[Alexandru Michis [:malexandru]]

Backed out changeset 323de6d7d422 for causing xpcshell failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/df727b0ee1680c73e31113da1206c8713a3f5f28

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=323de6d7d42282f98ca8ab1d5be9788d83fa7581&selectedJob=251968997

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251968997&repo=autoland&lineNumber=2773

[task 2019-06-14T23:29:24.831Z] 23:29:24 INFO - TEST-PASS | toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js | test_getGeneratedPassword - [test_getGeneratedPassword : 35] 1 added to cache - 1 == 1
[task 2019-06-14T23:29:24.832Z] 23:29:24 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js | test_getGeneratedPassword - [test_getGeneratedPassword : 36] Cache key and value - {"value":"7Hf8PNvaearVtmg","filled":false} == "7Hf8PNvaearVtmg"
[task 2019-06-14T23:29:24.832Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js:test_getGeneratedPassword:36
[task 2019-06-14T23:29:24.833Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:run_next_test/_run_next_test/<:1437
[task 2019-06-14T23:29:24.833Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_run_next_test:1437
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:run:688
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:227
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:529
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - -e:null:1
[task 2019-06-14T23:29:24.835Z] 23:29:24 INFO - exiting test
[task 2019-06-14T23:29:24.836Z] 23:29:24 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2)

Comment 13

[Sam Foster [:sfoster] (he/him)]

(In reply to Alexandru Michis [:malexandru] from comment #12)

Backed out changeset 323de6d7d422 for causing xpcshell failures.

Doh thanks for the backout, I had forgotten about that test.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/218e6f863de3
Add telemetry for when a generated password is first filled. r=MattN

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 218e6f863de3 (Bug 1548878) for test_autocomplete_new_password.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=218e6f863de3c5bd8a435b5a3da0d340f31eac59&tochange=39798e18e83ed891cf760e6d1e3269dc6edb83e7&selectedJob=252418253

Backout link: https://hg.mozilla.org/integration/autoland/rev/39798e18e83ed891cf760e6d1e3269dc6edb83e7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252418253&repo=autoland&lineNumber=1462

06:44:42 INFO - TEST-START | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html
06:44:42 INFO - GECKO(2212) | ++DOMWINDOW == 8 (07D30C00) [pid = 1648] [serial = 10] [outer = 07F33B80]
06:44:42 INFO - GECKO(2212) | [Parent 3996, Main Thread] WARNING: 'aRv.Failed()', file z:/build/build/src/dom/ipc/StructuredCloneData.cpp, line 120
06:44:42 INFO - GECKO(2212) | TEST-PASS | https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/parent_utils.js | Got autocomplete popup - [object XULPopupElement] == true
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 7 (00862400) [pid = 1648] [serial = 3] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 6 (0A861400) [pid = 1648] [serial = 7] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 16 (138D6C00) [pid = 3996] [serial = 8] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 15 (1593DC00) [pid = 3996] [serial = 16] [outer = 00000000] [url = about:blank]
06:44:45 INFO - GECKO(2212) | --DOMWINDOW == 2 (01269400) [pid = 3524] [serial = 2] [outer = 00000000] [url = about:blank]
06:44:45 INFO - GECKO(2212) | --DOMWINDOW == 1 (08B46160) [pid = 3524] [serial = 1] [outer = 00000000] [url = chrome://gfxsanity/content/sanitytest.html]
06:44:45 INFO - TEST-INFO | started process screenshot
06:44:45 INFO - TEST-INFO | screenshot: exit 0
06:44:45 INFO - <snipped 3 output lines - if you need more context, please use SimpleTest.requestCompleteLog() in your test>
06:44:45 INFO - Buffered messages logged at 06:44:42
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
...
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - Buffered messages logged at 06:44:45
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - Buffered messages finished
06:44:45 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html | Wait for 0 telemetry events - timed out after 50 tries. - Should not throw any errors
06:44:45 INFO - nextTick/<@https://example.com/tests/SimpleTest/SimpleTest.js:1801:26
06:44:45 INFO - asyncnextTick@https://example.com/tests/SimpleTest/SimpleTest.js:1809:11
06:44:45 INFO - setTimeout handlerSimpleTest_setTimeoutShim@https://example.com/tests/SimpleTest/SimpleTest.js:684:43
06:44:45 INFO - add_task@https://example.com/tests/SimpleTest/SimpleTest.js:1753:7
06:44:45 INFO - @https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html:80:1
06:44:45 INFO - GECKO(2212) | Removing 1 popup notifications.
06:44:45 INFO - GECKO(2212) | MEMORY STAT | vsize 503MB | vsizeMaxContiguous 794MB | residentFast 87MB | heapAllocated 10MB
06:44:45 INFO - TEST-OK | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html | took 3155ms

Comment 16

[Sam Foster [:sfoster] (he/him)]

Thanks for the backout (again). The TV failures look persistent and indicative of at least a problem with that test, if not a problem with the implementation itself. I'm looking into it.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Ok I have that test fixed. As test-verify re-runs the test, it was getting tripped up as the implementation here carefully avoids recording new telemetry events for re-use of a generated password. I now clear that map and also clear telemetry events at the start of the test.

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=937680030f87c9816a62388d590b6e47d9f3a942

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/292c5fa99131
Add telemetry for when a generated password is first filled. r=MattN

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/292c5fa99131

Comment 20

[Daniel Bodea [:danibodea]]

I have verified this implementation on Windows 10, Mac OS 10.14.5 and Ubuntu 18.04 on Nightly v70.0a1 from 2019-08-06.
It appears that password generations and generated password edits are being tracked correctly with instant telemetry events, but only ONCE for ANY SITE in EVERY ONE SESSION. Is this intended? If yes, then this implementation bug can be marked as verified. Thanks!

Comment 21

[Daniel Bodea [:danibodea]]

Attachment: password generated and password edit actions telemetry events.png

Are the events correctly displayed? Is there no "Value" or "Extra" information needed?

Comment 22

[Matthew N. [:MattN]]

(In reply to Bodea Daniel [:danibodea] from comment #20)

I have verified this implementation on Windows 10, Mac OS 10.14.5 and Ubuntu 18.04 on Nightly v70.0a1 from 2019-08-06.
It appears that password generations and generated password edits are being tracked correctly with instant telemetry events, but only ONCE for ANY SITE in EVERY ONE SESSION. Is this intended? If yes, then this implementation bug can be marked as verified. Thanks!

Yes, that is correct, we only record the first fill or edit for a site within a session.

(In reply to Bodea Daniel [:danibodea] from comment #21)

Are the events correctly displayed? Is there no "Value" or "Extra" information needed?

Yes, we don't have any other data to record.