1124895 - Add password manager usage data to FHR

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Chris Karlof [:ckarlof]]

For the majority of measurements discussed in Bug 1122728, we're going to wait for the unified Telemetry/FHR in 38, but we'd like to get this one measurement in FHR as soon as possible because we currently have no measurement of password manager use in FHR.

Thoughts, :bsmedberg?

Comment 1

[Chris Karlof [:ckarlof]]

or :gfritzsche?

Comment 2

[Georg Fritzsche [:gfritzsche]]

If you want this before 38, certainly go ahead.
Do you have any specific concerns you wanted input on?

Comment 3

[Chris Karlof [:ckarlof]]

> Do you have any specific concerns you wanted input on?

Nope, thanks!

Comment 4

[Benjamin Smedberg]

In terms of a data review, please see https://wiki.mozilla.org/Firefox/Data_Collection for the questions that would be helpful to answer. In terms of data sensitivity, the only particular thing I'd be concerned about is that having many saved passwords may paint a target on users.

In some ways, I'd be a lot more comfortable recording usage (# of saves, # of fills) than the total counts of saved passwords.

Comment 5

[Chris Karlof [:ckarlof]]

I have some more context around this request.

Since Password Manager is a core 2015 effort, we need an Executive Dashboard (i.e., dashboard for the E-team). The requirements for the PM executive dashboard are that it must address the following questions:

1) What is the size of the total addressable market? (broken down by usage type: heavy, medium, light, lapsed)
2) Who are the users of the password manager?
3) How much are the users using password manager?
4) How well is the password manager working?

-----------------------------------------------

Proposal for each:

1) All Desktop and Android installs.
2) I define a user of the PM to be a profile where the PM is enabled and has at least one saved password.
3) I propose we measure how much a user (i.e., a profile with the PM enabled and with at least one saved password) is using the password manager by recording how many saved passwords the profile has.
4) I propose we measure how well the password manager is working by recording the weekly rate of new saved passwords and the weekly rate of password fills on web pages. 

In summary, this proposal implies we need to add the following data to FHR:

1) Whether the PM is enabled.
2) If the PM is enabled, the number of saved passwords.
3) The weekly count of new saved passwords introduced by password capture heuristics.
4) The weekly count of passwords filled by the password manager in Web pages.

Comment 6

[Chris Karlof [:ckarlof]]

> In terms of a data review, please see https://wiki.mozilla.org/Firefox/Data_Collection for the questions that would be helpful to answer. 

> is the data collection necessary for Firefox to function properly? For example, the automatic update check must be sent in order to keep Firefox up to date.

no.

> Is there a specific user-visible function planned for the data?

Yes, it is a 2015 goal to improve and increase adoption of the password manager in Firefox. There is currently very little data on password manager usage, and we need a basic way to measure the impact of our improvements. 

> Population: Is it necessary to take a measurement from all users? Or is it sufficient to measure only prerelease users?

I don't know. I prefer measuring release users, because I have no evidence that suggests password manager usage on prerelease channels anyway corresponds with usage on release channel. At the least, I need to correlate the data with usage hours, because we cannot draw useful conclusions about the password manager by aggregating data from heavy Fx users and with those of 10mins of weekly usage.

> Sampling: is it necessary to get data from all users, or is it sufficient to collect data from a smaller sample?

I suspect sampling would be sufficient.


> Will data submission be automatic, or will there be opt-in UI?

Automatic.

> Analysis and Reporting:

> who will be analyzing the data?

Metrics team.

> Will the data that's being collected answer the questions we have?

Yes.

> Will it be a single or periodic report?

Periodic for the Executive Dashboard on Password Manager.

> Is it desirable to track data changes over time? With what frequency? With what latency?

Yes, weekly.

> Will the data reporting be private or public?

Private.

> Will the raw data being collected be private or public?

Private.

> Is it necessary to keep the measurement forever, or is it sufficient to run a short-term experiment/single report?

There is no particular need to keep the raw data forever, but I'd prefer to keep the aggregated reports indefinitely.

>Privacy (and Legal):

> Does the data contain sensitive or personal information?

FHR reports themselves contain sensitive information, but IMO the information this request would add doesn't increase the users' risk in any significant way.


> Can the data be used in combination with other measurements to identify a particular person?

No, it does not increase the risk of this any significant way.

> What kind of users controls will be exposed to control data submission?

The existing controls around FHR.

> Will users be able to see their own data before or after it has been submitted, either within Firefox or from the server?

Yes, via about:healthreport

> Does the data conform to the existing Mozilla privacy principles, the Mozilla Privacy Policy, and the Firefox privacy notice?

IMO, yes.

> Does this data collection represent any unusual privacy or legal risk to users or Mozilla?

IMO, no.

Comment 7

[Chris Karlof [:ckarlof]]

> In terms of data sensitivity, the only particular thing I'd be concerned about is that having many saved passwords may paint a target on users.

Can you clarify the events that would need to occur for a user to experience a negative outcome from FHR recording the number of saved passwords in her profile? IMO, it seems highly unlikely.

Comment 8

[Chris Karlof [:ckarlof]]

:joy does the proposal in comment 5 satisfy our requirements for the password manager Executive Dashboard?

Comment 9

["Saptarshi Guha[:joy]"]

> 1) Whether the PM is enabled.
Okay

> 2) If the PM is enabled, the number of saved passwords.
Okay

> 3) The weekly count of new saved passwords introduced by password capture
> heuristics.

Capture this at the session level. So count of new saved passwords introduced by password capture
euristics *every* session

> 4) The weekly count of passwords filled by the password manager in Web pages.
Same. Capture at the session level.

Also from this, "I define a user of the PM to be a profile where the PM is enabled and has at least one saved password." can be inferred from whether any of the session counts (filled/saved) >0.

If this data is collected it will meet the needs of the exec dashboards.
a) PM enabled
b)

Comment 10

[Benjamin Smedberg]

What does "the PM is enabled" mean in this context? As far as I know, the password manager is always enabled, in that we offer to save passwords any time we detect submission.

> > Is there a specific user-visible function planned for the data?
> Yes, it is a 2015 goal to improve and increase adoption of the password manager in Firefox. There is 
> currently very little data on password manager usage, and we need a basic way to measure the impact of 
> our improvements. 

That's not what this question means. This question means specifically how the data is going to visible to the user. e.g. will it show up in about:healthreport? Do we plan to drive self-support tips from the data? To put something in FHR, we need to be able to reasonbly tie the data collection to some user-visible benefit: this can either be direct benefit or improving the product, but it's not sufficient to just have a business need.

Comment 11

[Chris Karlof [:ckarlof]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> What does "the PM is enabled" mean in this context? As far as I know, the
> password manager is always enabled, in that we offer to save passwords any
> time we detect submission.
> 

There is an option in the preferences UI to "Remember passwords for sites". If this is checked, I consider the PM to be enabled. If it isn't, I consider it disabled. Screenshot attached.

Comment 12

[Chris Karlof [:ckarlof]]

Attachment: password-prefs.png

Comment 13

[Chris Karlof [:ckarlof]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> What does "the PM is enabled" mean in this context? As far as I know, the
> password manager is always enabled, in that we offer to save passwords any
> time we detect submission.
> 
> > > Is there a specific user-visible function planned for the data?
> > Yes, it is a 2015 goal to improve and increase adoption of the password manager in Firefox. There is 
> > currently very little data on password manager usage, and we need a basic way to measure the impact of 
> > our improvements. 
> 
> That's not what this question means. This question means specifically how
> the data is going to visible to the user. e.g. will it show up in
> about:healthreport?

Yes.

> Do we plan to drive self-support tips from the data?

Unlikely.

> To put something in FHR, we need to be able to reasonbly tie the data
> collection to some user-visible benefit: this can either be direct benefit
> or improving the product, but it's not sufficient to just have a business
> need.

Yes, ultimately, this is for improving the password manager. We are devoting to resources to improving the password manager this year, and we need some high level metrics on release channel to prioritize and measure the impact our work. I view the data in this request on par with the high level data on other browser features currently in FHR, e.g., size of places database, bookmark count, number of successful syncs, etc.

Comment 14

[Bill Maggs (bmaggs)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> What does "the PM is enabled" mean in this context? As far as I know, the
> password manager is always enabled, in that we offer to save passwords any
> time we detect submission.
> 
> > > Is there a specific user-visible function planned for the data?
> > Yes, it is a 2015 goal to improve and increase adoption of the password manager in Firefox. There is 
> > currently very little data on password manager usage, and we need a basic way to measure the impact of 
> > our improvements. 
> 
> That's not what this question means. This question means specifically how
> the data is going to visible to the user. e.g. will it show up in
> about:healthreport? Do we plan to drive self-support tips from the data? To
> put something in FHR, we need to be able to reasonbly tie the data
> collection to some user-visible benefit: this can either be direct benefit
> or improving the product, but it's not sufficient to just have a business
> need.

These are the specific asks, and the user benefit associated with each. 

1) Whether the PM is enabled

This information can be looked at as part of overall status of the browser. A convenient place to see if this is turned on alongside other FHR data. 

2) If the PM is enabled, the number of saved passwords

The benefit here is understanding how Firefox is doing on helping you log in to sites faster and more efficiently, as well as getting an idea of how often passwords are used to access web sites. 

3) The weekly count of new saved passwords introduced by password capture heuristics

A more detailed data point about how PM is doing at capture and fill on an ongoing basis. It could answer the question "Is Firefox's PM doing a good job for me?"

4) The weekly count of passwords filled by the password manager in Web pages

Useful for quantifying to the user how much logging in to sites is important to their use of the Web, and just how often you use it. This could be data that helps the user to decide whether to use PM more or less or even to seek out a third party password manager.

Comment 15

[Benjamin Smedberg]

Brief conversation today which led to a decision: there's a clear user benefit to collecting this data for a limited time to understand user behavior and drive short-term improvements. We don't intend to build specific support tips or about:healthreport UI from this data during that period, but I do think the password manager team should prepare blog posts explaining what the data shows us.

So the data as requested in comment 14 will be collected for 6 months, and then after we have a better understanding about the kinds of user support issues as well as what kind of data is needed for long-term product quality, we will revisit/revise.

Comment 16

[Allison Naaktgeboren :ally]

I understand that you & Bill are rethinking what to collect in light of the conversation and Bill requested I hold off starting until you two had a chance to think it over. Ideally I'd like to start during the hackday tomorrow.

Comment 17

[Allison Naaktgeboren :ally]

waiting for details from ckarlof(action item from thursday hack day) and info from georg about unification effort in 38.

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #17)
> waiting for details from ckarlof(action item from thursday hack day) and
> info from georg about unification effort in 38.

What do you need from me? Info about the current status etc.?

Comment 19

[Chris Karlof [:ckarlof]]

Ally, in the category of data we collect "for a limited time to understand user behavior and drive short-term improvements", Bill and I would also like to add:

1) A count of the number of login forms the user encounters. I believe this corresponds to the number of times "onFormPassword" in LoginManagerContent.jsm is called. 
2) A measure approximating the amount time the user spends logging in (i.e., the time between when the login form is visible and it submits), broken out by whether we filled saved credentials or not for the login. One of the goals of the password manager is to save the user time logging in. Going foward, I believe this metric can have direct visible user benefit, e.g., we can show her in the UI how much faster it is to use the password manager to log in. If this one is trickier to measure, we could break it out separately from this bug.

Comment 20

[Allison Naaktgeboren :ally]

I am waiting to hear if the telemetry-fhr unification made it into 38(So I know what format the probes will be in). Today 38 should be merging from m-c to m-a.

Comment 21

[Allison Naaktgeboren :ally]

For those playing along at home: most of the unification has landed, but not all of it, and there will be a go/no go decision about turning it on late in 38's beta cycle.

PM really wants this, so I'll be doing both the old-style and the new type of FHR probes. 

With the old style patch we are hoping for uplift to 37 (beta).

38(aurora) might have to have both types.

39(nightly) will have new type.

Comment 22

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes - wip

I believe I got all the boiler plate for the old style probes. I am confused about the enqueue storage operation found in the recordTreatmentTag of felipe's patch. Is that places only code? There's also a suspiciously empty function relating to serialization. TODOs are noted in the patch

for those playing along at home:

example probes
Felipe's UItour probe: https://bug1101790.bugzilla.mozilla.org/attachment.cgi?id=8526127
Mark Hammond's sync migration probe https://bug1097406.bugzilla.mozilla.org/attachment.cgi?id=8555026
Sync probes in general http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/healthreport.jsm

reference
https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/services/healthreport/healthreport/dataformat.html
https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/services/healthreport/healthreport/architecture.html

Comment 23

[Allison Naaktgeboren :ally]

to be clear, the patch only contains the first 2 probes: is the pm disabled and how many saved passwords there are.

Note that it is possible to have saved passwords and the pm disabled. Sync & addons can also store passwords there.

Comment 24

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes wip 2

Talked with felipe about his patch, and now have a better understanding of his and a better understanding of mine. 

Hoping georg has some time to look over the current patch before his long vacation.

While there is an entry point to record() in the patch, I'm pretty sure its the wrong spot, collecting the wrong number but I needed something to test the FHR bits.

Comment 25

[Allison Naaktgeboren :ally]

Since there isn't any good documentation on debugging original FHR, let's record it here.
- Start browser
- attach browser toolbox (remote debugging)
- open about:healthreport
- click raw data
- felipe notes that ctrl+F on that page will show you the payload

Error signs:
Raw Data won't load -> probably a js error with probe
about:healthreport blank -> Same
Issues registering providers: will show up in the error blob at the bottom of the fhr probe on the raw data age.

Currently I am stuck at
"errors": [
    "Provider error: LoginParentMetricsProvider: Error registering provider from category manager: TypeError: type is undefined (resource://gre/modules/HealthReport.jsm:705:8) JS Stack trace: 
    this.ProviderManager.prototype<[e-mail]:705:9 < this.ProviderManager.prototype<[e-mail]:632:23 < 
    [e-mail]:4466:15 < 
    [e-mail]:314:40 < [e-mail]:275:3 < createAsyncFunction/[e-mail]:249:14 < AbstractHealthReporter.prototype<.init/<@HealthReport.jsm:4398:15 < [e-mail]:314:40 < [e-mail]:393:7 < [e-mail]:322:13 < [e-mail]:867:23 < [e-mail]:746:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:688:37"
  ]

So far we have ruled out the .manifests & the build system as being the reason it can't find the LoginParentMetricsProvider.  

My best guess is some sort of exception occurs when loading LoginManagerParent.jsm. I also note that "TypeError: LoginManagerParent is undefined1 nsBrowserGlue.js:677:4" appears in the console on startup, and if none of the code in that file loads, then the catagory manager code would be unable to find it to load it. 

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/UpdaterHealthProvider.jsm seems to be the most straightforward probe we have, but I have been unable to spot any differences so far.

I have been unable to break in healthreport.jsm (which is actually a concat of lots of files for a js compartment hack. The error is actually here http://mxr.mozilla.org/mozilla-central/source/services/metrics/providermanager.jsm#201) The concat nature seems to mess up the debugger, bug 1137520.

Comment 26

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes -wp - unable to find new provider

see previous comment. I'm pretty sure something is throwing an exception when loading LoginManagerParent.jsm. Might have to resort to backing it all out and adding it back in one line at a time. :/

Comment 27

[Allison Naaktgeboren :ally]

aha! I got it to spit out actual errors!

Failed to load module resource://gre/modules/LoginManagerParent.jsm.

Comment 28

[Allison Naaktgeboren :ally]

missing " in exported symbols list. >.<

Comment 29

[Allison Naaktgeboren :ally]

boilerplate almost beaten

        "org.mozilla.PasswordManager.PasswordManager": {
          "_v": 2,
          "isDisabled": [
            "true",
            "true"
          ],
          "numSavedPasswords": [
            15
          ]
        },

Comment 30

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes - results in payload 2/6

provider, promise and json type errors have been beaten \o/

It seems I might need to do one db write per record() and break them out for FHR to record both consistently. Will need to figure out before I write the next 4 probes in this series.

Comment 31

[Allison Naaktgeboren :ally]

debugging tip: It is not enough to rebuild toolkit, for fhr you will also need to rebuild services (due to the js compartment overhead hack)
./mach build toolkit services

Comment 32

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes - correct probes, dummy values

boolean is now a 1/0 instead of a text string. record function works for any daily numerics. 

TODO:
sub in correct values
name remaining 4 fields so they're in the provider.

I am starting to understand the negative feelings around the js compartment hack...

Comment 33

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes

Feel free to rip it to shreds.

I however ran back into this sql mismatch on brand new & old profiles.
 isDisabled was a text field in the original draft.

./mach toolkit services no longer seems to fix it. I wonder if a clobber is needed. :/ 

14:44:07.260 A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full Message: Error: Field type does not match the expected for this operation. Actual: daily-discrete-text; Expected: daily-discrete-numeric
Full Stack: MetricsStorageSqliteBackend.prototype<._ensureFieldType@resource://gre/modules/HealthReport.jsm:3128:1
MetricsStorageSqliteBackend.prototype<.addDailyDiscreteNumericFromFieldID@resource://gre/modules/HealthReport.jsm:3713:5
recordKnownField@resource://gre/modules/LoginManagerParent.jsm:112:16

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8571685 [details] [diff] [review]
oldFHRprobes

Review of attachment 8571685 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/HealthReportComponents.manifest
@@ +10,5 @@
>  category healthreport-js-provider-default ProfileMetadataProvider resource://gre/modules/HealthReport.jsm
>  category healthreport-js-provider-default SearchesProvider resource://gre/modules/HealthReport.jsm
>  category healthreport-js-provider-default SessionsProvider resource://gre/modules/HealthReport.jsm
>  category healthreport-js-provider-default SysInfoProvider resource://gre/modules/HealthReport.jsm
> +category healthreport-js-provider-default LoginParentMetricsProvider resource://gre/modules/LoginManagerParent.jsm

I think this should be in passwordmgr.manifest to be closer to the code.

::: services/healthreport/docs/dataformat.rst
@@ +1899,5 @@
>          "foobar-value"
>        ]
>      }
> +
> +org.mozilla.PasswordManager.LoginParent

Nit: org.mozilla.passwordmgr.passwordmgr
(this more closely aligns with existing sections in case and name).
"Parent" is not relevant in this context IMO

@@ +1912,5 @@
> +
> +numSavedPasswords
> +    number of passwords saved in the Password Manager
> +
> +isDisabled

Why not `enabled` like we have for org.mozilla.sync.sync and org.mozilla.appInfo.update? `isDisabled: 1` is less clear than `enabled: 0` IMO.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +60,5 @@
> +  * Public API to be called by the LoginParent code
> +  */
> +const DAILY_DISCRETE_NUMERIC_FIELD = Metrics.Storage.FIELD_DAILY_DISCRETE_NUMERIC;
> +
> +const LoginParentHealthReport = {

I think these changes are better suited to nsLoginManager.js to keep the XPCOM stuff there and since this isn't related to communication with the content process.

@@ +159,5 @@
> +  
> +    // if the user has disabled the PM entirely. 
> +    // rememberSignons defaults to true
> +    if (!Services.prefs.getBoolPref("signon.rememberSignons")) {
> +      LoginParentHealthReport.record("isDisabled", 1);

I don't think you should be doing any work upon init as you're going to force a DB query on android. I think data can simply be returned lazily from Services.logins when requested by FHR in collectDailyData.

@@ +163,5 @@
> +      LoginParentHealthReport.record("isDisabled", 1);
> +    }
> +    let logins;
> +      try {
> +        logins = Services.logins.getAllLogins();

You want countLogins("", "", "") to avoid a MP prompt and to be more efficient. See PWMGR_NUM_SAVED_PASSWORDS.

Comment 35

[Allison Naaktgeboren :ally]

thank you MattN for the feedback.

(In reply to Matthew N. [:MattN] from comment #34)
> @@ +159,5 @@
> > +  
> > +    // if the user has disabled the PM entirely. 
> > +    // rememberSignons defaults to true
> > +    if (!Services.prefs.getBoolPref("signon.rememberSignons")) {
> > +      LoginParentHealthReport.record("isDisabled", 1);
> 
> I don't think you should be doing any work upon init as you're going to
> force a DB query on android. I think data can simply be returned lazily from
> Services.logins when requested by FHR in collectDailyData.
> 

Please remember that FHR only grabs data from its personal sql table. We are responsible for recording data to that table for whenever a payload is generated. The db operations to that table in record() are asynchronous.

Comment 36

[Allison Naaktgeboren :ally]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #35)
> thank you MattN for the feedback.
> 
> (In reply to Matthew N. [:MattN] from comment #34)
> > @@ +159,5 @@
> > > +  
> > > +    // if the user has disabled the PM entirely. 
> > > +    // rememberSignons defaults to true
> > > +    if (!Services.prefs.getBoolPref("signon.rememberSignons")) {
> > > +      LoginParentHealthReport.record("isDisabled", 1);
> > 
> > I don't think you should be doing any work upon init as you're going to
> > force a DB query on android. I think data can simply be returned lazily from
> > Services.logins when requested by FHR in collectDailyData.
> > 
> 
> Please remember that FHR only grabs data from its personal sql table. We are
> responsible for recording data to that table for whenever a payload is
> generated. The db operations to that table in record() are asynchronous.

I also note that android's FHR is entirely separate from this one, totally in java. We'll need an entirely separate patch for that.

Comment 37

[:Margaret Leibovic]

Comment on attachment 8571685 [details] [diff] [review]
oldFHRprobes

Review of attachment 8571685 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +115,5 @@
> +
> +    // Otherwise, we first need to create the field.
> +    return this.enqueueStorageOperation(function recordField() {
> +      // This function has to return a promise.
> +      return Task.spawn(function () {

Drive-by: If you want a function that just returns a Task.spawn, you can use Task.async (a convenience method created for exactly this use-case :).

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8571685 [details] [diff] [review]
oldFHRprobes

Review of attachment 8571685 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/docs/dataformat.rst
@@ +1900,5 @@
>        ]
>      }
> +
> +org.mozilla.PasswordManager.LoginParent
> +----------------------------------

The lines surrounding section titles should be equivalent length to the section title.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +109,5 @@
> +                                             DAILY_DISCRETE_NUMERIC_FIELD)) {
> +      let fieldID = this.storage.fieldIDFromMeasurement(m.id, field);
> +      return this.enqueueStorageOperation(function recordKnownField() {
> +        return this.storage.addDailyDiscreteNumericFromFieldID(fieldID, aNum);
> +      }.bind(this));

This could probably do with a nice sprinkling of fat arrow functions.

@@ +115,5 @@
> +
> +    // Otherwise, we first need to create the field.
> +    return this.enqueueStorageOperation(function recordField() {
> +      // This function has to return a promise.
> +      return Task.spawn(function () {

Generator functions should be declared with function* (unless things have changed in the 6 months since I last wrote JS). But as Margaret says, use Task.async().

@@ +117,5 @@
> +    return this.enqueueStorageOperation(function recordField() {
> +      // This function has to return a promise.
> +      return Task.spawn(function () {
> +        let fieldID = yield this.storage.registerField(m.id, field,
> +                                                       DAILY_DISCRETE_NUMERIC_FIELD);

You shouldn't need to register fields here. If the provider and measurement is properly defined, the fields will get registered when FHR loads. You should be able to obtain a reference to the measurement and call functions on it. See e.g. https://dxr.mozilla.org/mozilla-central/source/services/healthreport/providers.jsm#1757

@@ +131,5 @@
> +
> +PasswordsMeasurement1.prototype = Object.freeze({
> +  __proto__: Metrics.Measurement.prototype,
> +  name: "PasswordManager",
> +  version: 2,  

You cargo culted "2". This should be 1.

@@ +170,5 @@
> +        debug("Master password permissions error: " + e);
> +        LoginParentHealthReport.record("numSavedPasswords", -1);
> +        return;
> +      }
> +      LoginParentHealthReport.record("numSavedPasswords", logins.length);

So you've implemented this as a push-based provider. I don't think this is the right approach.

My main objection to this is that by putting recording in LoginManagerParent.init(), you will force FHR to initialize whenever the login manager initializes. This *may* force FHR to initialize earlier than it otherwise would have.

Back in the day, I spent a lot of effort to ensure that FHR didn't initialize until something like 30s after the profile was fully initialized so it didn't interfere with startup performance. This work may have been invalidated since (I'm not sure what all providers exist these days).

Now, there is nothing in this code that seems to require FHR recording living in Login Manager initialization code. The pref reading can certainly be moved to a daily collection point. Reading Services.logins.getAllLogins() is a bit more difficult, as there is a dependency between the logins service being available and initialized and FHR collecting data. But I think it should be possible to move.

What justification do you have for this being a push-based provider?

Comment 39

[Allison Naaktgeboren :ally]

(In reply to Gregory Szorc [:gps] from comment #38)
> > +
> > +PasswordsMeasurement1.prototype = Object.freeze({
> > +  __proto__: Metrics.Measurement.prototype,
> > +  name: "PasswordManager",
> > +  version: 2,  
> 
> You cargo culted "2". This should be 1.

Er, the documentation said desktop was on version 2, and android on version 3. That's where I got the 2 from. Does this 'version' field refer to a different versioning system inside fhr?
> 
> @@ +170,5 @@
> > +        debug("Master password permissions error: " + e);
> > +        LoginParentHealthReport.record("numSavedPasswords", -1);
> > +        return;
> > +      }
> > +      LoginParentHealthReport.record("numSavedPasswords", logins.length);
> 
> So you've implemented this as a push-based provider. I don't think this is
> the right approach.

I was not aware there were any other kinds. 

> 
> My main objection to this is that by putting recording in
> LoginManagerParent.init(), you will force FHR to initialize whenever the
> login manager initializes. This *may* force FHR to initialize earlier than
> it otherwise would have.

fair.
> 
> Back in the day, I spent a lot of effort to ensure that FHR didn't
> initialize until something like 30s after the profile was fully initialized
> so it didn't interfere with startup performance. This work may have been
> invalidated since (I'm not sure what all providers exist these days).

I was going off of the ui tour & sync migration fhr providers if that helps.

If it's been invalidated, then the push provider should not be a big deal?
> 
> Now, there is nothing in this code that seems to require FHR recording
> living in Login Manager initialization code. The pref reading can certainly
> be moved to a daily collection point. 

Daily collection point? What is that? I did not see a technical definition for that in the docs.

Reading Services.logins.getAllLogins()
> is a bit more difficult, as there is a dependency between the logins service
> being available and initialized and FHR collecting data. But I think it
> should be possible to move.
> 
> What justification do you have for this being a push-based provider?

I'm open to options, that was just structure of providers I found in code.

If your work to delay startup has been invalidated, is it that horrible to risk delay for 1-2 releases(37, it is still undecided which fhr system 38 will use)? This'll get yanked by 39.

Comment 40

[Gregory Szorc [:gps]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #39)
> (In reply to Gregory Szorc [:gps] from comment #38)
> > > +
> > > +PasswordsMeasurement1.prototype = Object.freeze({
> > > +  __proto__: Metrics.Measurement.prototype,
> > > +  name: "PasswordManager",
> > > +  version: 2,  
> > 
> > You cargo culted "2". This should be 1.
> 
> Er, the documentation said desktop was on version 2, and android on version
> 3. That's where I got the 2 from. Does this 'version' field refer to a
> different versioning system inside fhr?

This is the version of the measurement. The version starts at 1, corresponds to the type name, and is bumped whenever a new field is introduced or semantics change.

> > @@ +170,5 @@
> > > +        debug("Master password permissions error: " + e);
> > > +        LoginParentHealthReport.record("numSavedPasswords", -1);
> > > +        return;
> > > +      }
> > > +      LoginParentHealthReport.record("numSavedPasswords", logins.length);
> > 
> > So you've implemented this as a push-based provider. I don't think this is
> > the right approach.
> 
> I was not aware there were any other kinds. 

There are push-based and pull-based providers.

Push-based providers are providers that record data as it is produced. This is typically event counts.

Pull-based are passive and they run collection at periodic intervals. Pull-based is preferred.

See the crashes provider for a good example of a pull-based provider. https://dxr.mozilla.org/mozilla-central/source/services/healthreport/providers.jsm#1191. Note the collectDailyData function.

> > Back in the day, I spent a lot of effort to ensure that FHR didn't
> > initialize until something like 30s after the profile was fully initialized
> > so it didn't interfere with startup performance. This work may have been
> > invalidated since (I'm not sure what all providers exist these days).
> 
> I was going off of the ui tour & sync migration fhr providers if that helps.

I didn't review these and am not familiar with the code. It's entirely possible they are doing things sub-optimally.
 
> If it's been invalidated, then the push provider should not be a big deal?

If you can prove FHR is initiated immediately during profile startup due to an existing provider "misbehaving," then it isn't a big deal.

> > Now, there is nothing in this code that seems to require FHR recording
> > living in Login Manager initialization code. The pref reading can certainly
> > be moved to a daily collection point. 
> 
> Daily collection point? What is that? I did not see a technical definition
> for that in the docs.

collectDailyData().

> Reading Services.logins.getAllLogins()
> > is a bit more difficult, as there is a dependency between the logins service
> > being available and initialized and FHR collecting data. But I think it
> > should be possible to move.
> > 
> > What justification do you have for this being a push-based provider?
> 
> I'm open to options, that was just structure of providers I found in code.
> 
> If your work to delay startup has been invalidated, is it that horrible to
> risk delay for 1-2 releases(37, it is still undecided which fhr system 38
> will use)? This'll get yanked by 39.

Going with the current implementation is acceptable if the horses have left the stable. If not, please convert to use collectDailyData.

(I don't think it is much work to convert to collectDailyData.)

Comment 41

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes

"Provider error: PasswordsMetricsProvider: Error registering provider from category manager: SyntaxError: illegal character (resource://gre/modules/nsLoginManager.js:188) JS Stack trace: this.ProviderManager.prototype<[e-mail]:630:9 < [e-mail]:4466:15 < [e-mail]:314:40 < [e-mail]:275:3 < 

line 188 is #ifdef ANDROID

which JS should never see because that is supposed to be pre-processed out by the build system.

I have messed with the moz.build and jar.mn & clobber builds and not yet found the right incantation. I think it is time to put this down for tonight. Maybe the answer will be obvious after a good night's sleep.

Comment 42

[Matthew N. [:MattN]]

Comment on attachment 8572430 [details] [diff] [review]
oldFHRprobes

Review of attachment 8572430 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/moz.build
@@ +35,5 @@
>      'InsecurePasswordUtils.jsm',
>      'LoginHelper.jsm',
>      'LoginManagerContent.jsm',
>      'LoginManagerParent.jsm',
> +    'nsLoginManager.js',

This isn't a module (not a JSM extension) and you don't need this.

::: toolkit/components/passwordmgr/passwordmgr.manifest
@@ +14,5 @@
>  contract @mozilla.org/login-manager/storage/json;1 {c00c432d-a0c9-46d7-bef6-9c45b4d07341}
>  #endif
>  component {dc6c2976-0f73-4f1f-b9ff-3d72b4e28309} crypto-SDR.js
>  contract @mozilla.org/login-manager/crypto/SDR;1 {dc6c2976-0f73-4f1f-b9ff-3d72b4e28309}
> +category healthreport-js-provider-default PasswordsMetricsProvider resource://gre/modules/nsLoginManager.js

I think "resource://gre/modules/nsLoginManager.js" should be "@mozilla.org/login-manager;1" since this isn't a JSM. I think that will fix the preprocessing.

Comment 43

[Allison Naaktgeboren :ally]

"@mozilla.org/login-manager;1"  results in a MALFORMED URI exception during provider registration. :/

Comment 44

[Allison Naaktgeboren :ally]

registration this way is currently impossible.

https://mxr.mozilla.org/mozilla-central/source/services/metrics/providermanager.jsm#108

 registerProvidersFromCategoryManager() can't handle XPCOM, so we'd have to fix this to be able to do the registration properly. 

Talked it over with MattN, moving provider back to LoginManagerParent.js

Comment 45

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobes 1/2

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8573627 [details] [diff] [review]
oldFHRprobes 1/2

Review of attachment 8573627 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +73,5 @@
> +  collectDailyData: function () {
> +    return this.storage.enqueueTransaction(this._recordDailyPasswordData.bind(this));
> +  },
> +
> +  _recordDailyPasswordData: function() {

Generators should be declared with function*. Fortunately, I believe SpiderMonkey is currently forgiving.

Comment 47

[Allison Naaktgeboren :ally]

part 1/2:  https://hg.mozilla.org/integration/fx-team/rev/dcf73ba1d6bc

please leave open for part 2.

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/acae51c40eea for apparently breaking every android test suite.

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=dcf73ba1d6bc&filter-searchStr=android
https://treeherder.mozilla.org/logviewer.html#?job_id=2203274&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=2203339&repo=fx-team

Comment 49

[Richard Newman [:rnewman]]

If I had to guess:

19:18:45 INFO - 03-09 19:12:42.578 W/GeckoConsole( 2282): [JavaScript Error: "Failed to load module resource://gre/modules/Metrics.jsm." {file: "resource://gre/modules/XPCOMUtils.jsm" line: 256}]
19:18:45 INFO - 03-09 19:12:42.578 W/GeckoConsole( 2282): [JavaScript Error: "Failed to load module resource://gre/modules/LoginManagerParent.jsm." {file: "resource://gre/modules/XPCOMUtils.jsm" line: 256}]
19:18:45 INFO - 03-09 19:12:42.578 W/GeckoConsole( 2282): [JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 254}] 

is your culprit.

Bear in mind that most of the desktop FHR modules (correctly) aren't packaged on Android, so you can't unconditionally refer to them from inside shared stuff like LoginManagerParent.

All of your uses of FHR modules should either by try-catch guarded to implement runtime behavior switching, or should be guarded by a desktop FHR build-time conditional.

Giving this kind of cross-platform stuff a run through Try would probably be a time-saver in the long run.

Comment 50

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ec242415b4 suggests that I've found the appropriate preprocessor fix, but let's do a full try run just to make sure everything is covered.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eda58c8857a

Comment 51

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2/2 - 4 more probes - payload layout

patch #2 is the 4 remaining probes. When I originally split up the work we still had a push provider and these were going to be 1-2 line additions. Now however, we have a pull provider per gps' request and these look like event driven probes.

We are looking at the following probes

#3 - # of new passwords saved this session
#4 - # of passwords filled in for user this session
#5 - total # of login fields encountered this session
#6a - Time(ms) from login visible to submission when the PM autofills
#6b - Time(ms) from login visible to submission when the PM does nothing

Example
^^^^^^^
::
    "org.mozilla.passwordmgr.passwordmgr": {
      "_v": 1,
      "numSavedPasswords": 32,
      "enabled": 1,
      "numNewSavedPasswords": 5,
      "numSuccessfulFills": 11,
      "totalLoginsEncountered": 23,
      "timeToCompletionAutofilled": 3200
      "timeToCompletionNofill": 6000
    }

Please feel free to weigh in about the payload structure now.

Comment 52

[Allison Naaktgeboren :ally]

full try run is not finished but looking really green (whereas I am only looking slightly green)

https://hg.mozilla.org/integration/fx-team/rev/db5005bdc4be

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db5005bdc4be

Comment 54

[Allison Naaktgeboren :ally]

Gps,
So the next set of probes for the password manager all look event driven. The next set of probes are documented in comment 51. I have spent the day looking for examples of event driven probes + pull providers combining in the desktop code base, but I have not been able to find any evidence that these can be combined.

My understanding, such as it is, is that for this next patch I will need to implement a push provider for them, I can't extend the existing pull provider.

Does this understanding match yours?

Comment 55

[Gregory Szorc [:gps]]

No, you can use the same type to have an event-based and pull-based provider. Just don't set the "pullOnlyProvider" property.

See https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/healthreport.jsm for an example that serves both roles.

Comment 56

[Allison Naaktgeboren :ally]

Comment on attachment 8573627 [details] [diff] [review]
oldFHRprobes 1/2

Approval Request Comment
[Feature/regressing bug #]: Password Manager Project 2015, Q1 goals
[User impact if declined]: User impact none. Project impact high. We need a baseline to determine what the login landscape looks like, how the password manager is currently doing, and whether or not our work has any effect.

[Describe test coverage new/current, TreeHerder]:

[Risks and why]:  

[String/UUID change made/needed]: no

Comment 57

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8573627 [details] [diff] [review]
oldFHRprobes 1/2

This is a self contained change to FHR that supports a high priority desktop initiative. dolske confirmed that this has privacy approval and has been reviewed with bsmedberg. Let's take this today in Beta 6. Beta+ Aurora+

Comment 58

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ec7f3ce97d2

Comment 59

[Justin Dolske [:Dolske]]

Derp.

Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/42877284c697, and relanded in https://hg.mozilla.org/releases/mozilla-beta/rev/fe5932c2b378

I pushed the wrong patch (last attachment here, instead of what actually landed in comment 52. At least it failed the same way as it did in comment 47-50, so I am optimistic that this will take care of things.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0aa24399b11b

I guess Fx37 is as fixed as it's going to be given where we are in the release cycle. I'm leaving 38/39 set to affected, though, under the assumption that Part 2 needs to land on both still.

Comment 61

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2 - push & pull. test push in init()

both push & pull probes show up in the payload. 

need to do the recording in proper places.

Comment 62

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2

needs more through testing, but lets get feedback while I'm doing that.

Comment 63

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2

Comment 64

[Justin Dolske [:Dolske]]

Comment on attachment 8579029 [details] [diff] [review]
oldFHRprobesPart2

Review of attachment 8579029 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +213,5 @@
>          this.doAutocompleteSearch(data, msg.target);
>          break;
>        }
> +
> +      case "RemoteLogins:LoginFillSuccessful": {

Let's call test LoginStats:foo or something like that, to make it clear these are not actually for core functionality.

@@ +225,5 @@
>  
>    findLogins: function(showMasterPassword, formOrigin, actionOrigin,
>                         requestId, target) {
> +    // how many logins are in the wild?
> +    PasswordsHealthReport.recordCounter( "totalLoginsEncountered", Metrics.Storage.FIELD_DAILY_COUNTER);

This is triggered by the content process's _asyncFindLogins(), but that's call from both onFormPassword() and onUsernameInput(). So measuring both here would be off.

I think you'll need to explicitly signal this with a message manager call. Do we want this before the gEnabledCheck?

@@ +456,5 @@
>          propBag.setProperty("timesUsedIncrement", 1);
>          Services.logins.modifyLogin(existingLogin, propBag);
>        }
>  
> +      PasswordsHealthReport.recordCounter( "numNewSavedPasswords", Metrics.Storage.FIELD_DAILY_COUNTER);

This should actually go in nsLoginManagerPrompter.js, in _showSaveLoginNotification(). (In the callback with the existing addLogin() call)

Comment 65

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #64)

> This is triggered by the content process's _asyncFindLogins(), but that's
> call from both onFormPassword() and onUsernameInput(). So measuring both
> here would be off.
> 
> I think you'll need to explicitly signal this with a message manager call.
> Do we want this before the gEnabledCheck?

To be clear, I'm talking about onFormPassword() in the content process.

Comment 66

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #64)

> This should actually go in nsLoginManagerPrompter.js, in
> _showSaveLoginNotification(). (In the callback with the existing addLogin()
> call)

(From discussion:)

Easiest way to do this is probably just to fire a notification in nsLoginManagerPrompter, and observe it in LoginManagerParent.

Comment 67

[Allison Naaktgeboren :ally]

rawr try is closed!

Comment 68

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2/2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b67a4dcbe051

Comment 69

[Allison Naaktgeboren :ally]

Comment on attachment 8579534 [details] [diff] [review]
oldFHRprobesPart2/2

nit: in the example it says v: 1 and it should say v 2. fixed locally

Comment 70

[Justin Dolske [:Dolske]]

Comment on attachment 8579534 [details] [diff] [review]
oldFHRprobesPart2/2

Review of attachment 8579534 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't really look at the  FHR-specific bits at the top of LoginManangerParent.jsm, someone familiar with FHR should. But the rest looks fine!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +255,5 @@
> +
> +    let doc = form.ownerDocument;
> +    let win = doc.defaultView;
> +    let messageManager = messageManagerFromWindow(win);
> +    messageManager.sendAsyncMessage("LoginStats:LoginEncountered");

Tiny belated nit: "FormEncountered" or "LoginFormEncountered" might be a little more clearer / self-documenting, but I'd also take this as-is.

Comment 71

[Allison Naaktgeboren :ally]

Comment on attachment 8579534 [details] [diff] [review]
oldFHRprobesPart2/2

Word has come back that it would be too risky to take for the final beta tomorrow, so this is bound for m-c  aurora

Comment 72

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8573627 [details] [diff] [review]
oldFHRprobes 1/2

This patch landed already.

Comment 73

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8579534 [details] [diff] [review]
oldFHRprobesPart2/2

Review of attachment 8579534 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/docs/dataformat.rst
@@ +1960,5 @@
>        "enabled": 0,
>      }
> +
> +Version 2
> +^^^^^^^^^

Describe in one line what this version adds?

@@ +1962,5 @@
> +
> +Version 2
> +^^^^^^^^^
> +numNewSavedPasswords
> +    number of passwords saved to the password manager this session

The naming could be more descriptive, numSavedPasswordsInSession maybe?
Nit: Upper-case start of sentence, end with '.'. Dito the two cases below.

@@ +1967,5 @@
> +
> +numSuccessfulFills
> +    number of times the password manager filled in password fields for user this session
> +
> +totalLoginsEncountered

Consistency suggests that this should be prefixed with num too?

@@ +1975,5 @@
> +^^^^^^^
> +
> +::
> +    "org.mozilla.passwordmgr.passwordmgr": {
> +      "_v": 1,

This should be 2.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +53,3 @@
>  
>  #ifndef ANDROID
>  #ifdef MOZ_SERVICES_HEALTHREPORT

|#if !defined(ANDROID) && defined(MOZ_SERVICES_HEALTHREPORT)|?
Dito below.

@@ +57,5 @@
>                                    "resource://gre/modules/Metrics.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");
>  
> +const PasswordsHealthReport = {

Nit: This only holds one function, i don't think we need to stick it in an object.
Maybe make it just a top-level utility function?

@@ +58,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");
>  
> +const PasswordsHealthReport = {
> +  recordCounter: function(aField, fieldType) {

Only FIELD_DAILY_COUNTER is passed as fieldType below.
You could just remove that argument and rename this to recordDailyCounter.

@@ +59,5 @@
>                                    "resource://gre/modules/Task.jsm");
>  
> +const PasswordsHealthReport = {
> +  recordCounter: function(aField, fieldType) {
> +    Task.spawn(function*() {

I only see one yield below, so we don't need a Task here - i.e.:
let reporter = ...
...
reporter.onInit().then(() => reporter.getProvider...

@@ +106,5 @@
>      yield m.setDailyLastNumeric("numSavedPasswords", loginsCount);
>  
>    },
> +
> +  recordCounter: function(aField, fieldType) {

Only FIELD_DAILY_COUNTER is passed as fieldType, make it recordDailyCounter?

@@ +109,5 @@
> +
> +  recordCounter: function(aField, fieldType) {
> +    let m = this.getMeasurement(PasswordsMeasurement2.prototype.name,
> +                                PasswordsMeasurement2.prototype.version);
> +    let field = aField;

Nit: What is the temporary variable needed for?

@@ +113,5 @@
> +    let field = aField;
> +    if (this.storage.hasFieldFromMeasurement(m.id, field,
> +                                             fieldType)) {
> +      let fieldID = this.storage.fieldIDFromMeasurement(m.id, field);
> +      return this.enqueueStorageOperation(function recordKnownField() {

Just make it an arrow function and drop the .bind(this):
return this.enqueueStorageOperation(() => m.incrementDailyCounter(field));

@@ +119,5 @@
> +      }.bind(this));
> +    }
> +
> +    // Otherwise, we first need to create the field.
> +    return this.enqueueStorageOperation(function recordField() {

You can just make it an arrow function.

@@ +121,5 @@
> +
> +    // Otherwise, we first need to create the field.
> +    return this.enqueueStorageOperation(function recordField() {
> +      // This function has to return a promise.
> +      return Task.spawn(function () {

This should be a generator, function*().
Or better, just return a promise here, which seems straight-forward:
return this.storage.registerField(m.id, field, fieldType)
           .then(() => m.incrementDailyCounter(field));

@@ +141,5 @@
>    version: 1,
>    fields: {
>      enabled: {type: Metrics.Storage.FIELD_DAILY_LAST_NUMERIC},
>      numSavedPasswords: {type: Metrics.Storage.FIELD_DAILY_LAST_NUMERIC},
> +    },

Nit: Fix indentation.

Comment 74

[Allison Naaktgeboren :ally]

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +53,3 @@
>  
>  #ifndef ANDROID
>  #ifdef MOZ_SERVICES_HEALTHREPORT

|#if !defined(ANDROID) && defined(MOZ_SERVICES_HEALTHREPORT)|?
Dito below.

You're thinking of the c++ preprocessor, not our artisanal inhouse build system preprocessor, which is not nearly as smart. I had a lovely conversation with #build, but it doesn't sound like our preprocessor is going to get any smarter anytime soon.

Comment 75

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobesPart2

with respect to 
> +const PasswordsHealthReport = {
>Nit: This only holds one function, i don't think we need to stick it in an object.
>Maybe make it just a top-level utility function?

I tried that, and FHR stopped finding my provider and the push probes stopped working. I don't pretend to understand why.

Comment 76

[Georg Fritzsche [:gfritzsche]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #74)
> ::: toolkit/components/passwordmgr/LoginManagerParent.jsm
> @@ +53,3 @@
> >  
> >  #ifndef ANDROID
> >  #ifdef MOZ_SERVICES_HEALTHREPORT
> 
> |#if !defined(ANDROID) && defined(MOZ_SERVICES_HEALTHREPORT)|?
> Dito below.
> 
> You're thinking of the c++ preprocessor, not our artisanal inhouse build
> system preprocessor, which is not nearly as smart. I had a lovely
> conversation with #build, but it doesn't sound like our preprocessor is
> going to get any smarter anytime soon.

Ah, ok, nevermind :(

Comment 77

[Georg Fritzsche [:gfritzsche]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #75)
> with respect to 
> > +const PasswordsHealthReport = {
> >Nit: This only holds one function, i don't think we need to stick it in an object.
> >Maybe make it just a top-level utility function?
> 
> I tried that, and FHR stopped finding my provider and the push probes
> stopped working. I don't pretend to understand why.

That sounds like we should understand why, not work around it - were there any errors showing up in the terminal or the browser console?

Comment 78

[Allison Naaktgeboren :ally]

> > I tried that, and FHR stopped finding my provider and the push probes
> > stopped working. I don't pretend to understand why.
> 
> That sounds like we should understand why, not work around it - were there
> any errors showing up in the terminal or the browser console?

In general I agree, however I'm up against the merge deadline. :/ 

I saw nothing in either, but the entire probe was missing from the fhr payload. I think I ran into a related bug during development where I needed to create an instance of the provider to work. It may be related to that.

Comment 79

[Georg Fritzsche [:gfritzsche]]

You would still be good to land e.g. tomorrow with checking that, this doesn't sound like it should be a complicated issue?
Or am i missing something intricate here?

Comment 80

[Allison Naaktgeboren :ally]

(In reply to Georg Fritzsche [:gfritzsche] from comment #79)
> You would still be good to land e.g. tomorrow with checking that, this
> doesn't sound like it should be a complicated issue?
> Or am i missing something intricate here?

I'm confused by "tomorrow with checking that". What do you mean by 'that'?

I'm saying I did check out/tried to apply your requested nit and it did not work, so I am letting you know why that requested change is not present in the new patch.

Comment 81

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8582821 [details] [diff] [review]
oldFHRprobesPart2

Review of attachment 8582821 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me with the issues below adressed. Ping me if needed.

::: services/healthreport/docs/dataformat.rst
@@ +1983,5 @@
> +      "numSavedPasswords": 32,
> +      "enabled": 1,
> +      "numNewSavedPasswords": 5,
> +      "numSuccessfulFills": 11,
> +      "totalLoginsEncountered": 23,

numTotalLoginsEncountered

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +58,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");
>  
> +const PasswordsHealthReport = {
> +  recordDailyCounter: function(aField) {

I'm still convinced that this should be trivial to make top-level (instead of nesting it as the only function in an object as a work-around).
You are probably just running into that this function has the same name as 
PasswordsMetricsProvider.recordDailyCounter.
Please try again.

@@ +69,5 @@
> +    if (!reporter) {
> +      return;
> +    }
> +      reporter.onInit().then(() => reporter.getProvider("org.mozilla.passwordmgr")
> +                                           .recordDailyCounter(aField));

Fix indentation.

@@ +70,5 @@
> +      return;
> +    }
> +      reporter.onInit().then(() => reporter.getProvider("org.mozilla.passwordmgr")
> +                                           .recordDailyCounter(aField));
> +

Redundant empty line.

@@ +112,5 @@
> +      let fieldID = this.storage.fieldIDFromMeasurement(m.id, aField, Metrics.Storage.FIELD_DAILY_COUNTER);
> +      return this.enqueueStorageOperation(() => m.incrementDailyCounter(aField));
> +    }
> +
> +    // Otherwise, we first need to create the field.

I think we had a misunderstanding here.
Why did you drop the this.enqueueStorageOperation() part here?
This should be (with proper linebreaks/indentation):
return this.enqueueStorageOperation(() => this.storage.registerField(m.id, aField, Metrics.Storage.FIELD_DAILY_COUNTER).then(() => m.incrementDailyCounter(aField));

Comment 82

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8582821 [details] [diff] [review]
oldFHRprobesPart2

See above.

Comment 83

[Allison Naaktgeboren :ally]

 https://hg.mozilla.org/integration/fx-team/rev/70a0676ca7c6

Comment 84

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/mozilla-central/rev/70a0676ca7c6

Comment 85

[Allison Naaktgeboren :ally]

Comment on attachment 8582821 [details] [diff] [review]
oldFHRprobesPart2

Approval Request Comment 
[Feature/regressing bug #]: Password Manager Project
[User impact if declined]: we will not know what sort of logins users encounter in the wild, how often, and whether or not any of the other changes we've made have any impact
[Describe test coverage new/current, TreeHerder]: current
[Risks and why]: see above
[String/UUID change made/needed]: no

Comment 86

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8582821 [details] [diff] [review]
oldFHRprobesPart2

Taking this for aurora. Sounds low risk, and we would like to have this data.

Comment 87

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8582821 [details] [diff] [review]
oldFHRprobesPart2

Having info from beta will probably help a lot. taking it. should be in beta 2

Comment 88

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for uplift.

Comment 89

[Ryan VanderMeulen [:RyanVM]]

Oh hah, this landed before the uplift. No wonder why I was hitting conflicts on Aurora!

Comment 90

[Ryan VanderMeulen [:RyanVM]]

But this still has pretty significant conflicts on beta that'll need rebasing around or uplift requests in other bugs.

Comment 91

[Allison Naaktgeboren :ally]

Attachment: oldFHRprobespart2_Beta

Since the rebase was nontrival in the Content.jsm file, I'd like :dolske to look it over before uplift just to be extra safe.

I'd pick on :MattN, but he's apparently in a barn atm. :)

Comment 92

[Justin Dolske [:Dolske]]

Comment on attachment 8589346 [details] [diff] [review]
oldFHRprobespart2_Beta

Review of attachment 8589346 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, although I didn't look at the FHR-specific bits.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +871,5 @@
>            accessKey: rememberButtonAccessKey,
>            popup:     null,
>            callback: function(aNotifyObj, aButton) {
>              pwmgr.addLogin(aLogin);
> +            Services.obs.notifyObservers(null, 'LoginStats:NewSavedPassword', null);

You can remove this addition (although it's harmless to keep it). This block is only used for the save-password notification *bar*, which is no longer used in Firefox.

Comment 93

[Allison Naaktgeboren :ally]

beta version of the patch is good

Comment 94

[Allison Naaktgeboren :ally]

apparently I don't have permission to land on beta. Hg is bouncing me for permissions

Comment 95

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

You should have all the permissions necessary if you can push to trunk trees like fx-team. You'll need to amend the commit message to include "a=[person that approved it for beta]" to pass the approval-required hook.

Comment 96

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ac9862939f3e