Closed Bug 1016138 Opened 8 years ago Closed 8 years ago

Add telemetry probe for master password usage

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: markh, Assigned: markh)

Details

(Whiteboard: p=1 s=33.1 [qa!])

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1013448 +++

This will help us better estimate how many users have master password enabled.

The patch from bug 1013448 caused xperf test failures:

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\secmod.db' was accessed and we were not expecting it.  DiskReadCount: 6, DiskWriteCount: 0, DiskReadBytes: 16904, DiskWriteBytes: 0

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\cert8.db' was accessed and we were not expecting it.  DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 33288, DiskWriteBytes: 0

00:38:30     INFO -  TEST-UNEXPECTED-FAIL : xperf: File '{profile}\key3.db' was accessed and we were not expecting it.  DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 8712, DiskWriteBytes: 0

because we were querying the master-password state as part of delayedStartup, causing these reads.

(In reply to Vladan Djeric (:vladan) from bug 1013448 comment 21)
> I think you can fix this by moving the Telemetry work out of
> _delayedStartup. Either do it after sessionstore-windows-restored (i.e.
> listen for this event and then do the work in a setTimeout) or at browser
> exit ("profile-before-change" event).

I'm wondering if there is a way to avoid this additional IO completely - eg, move the probe closer to the security modules, and write the probe once we are already doing the IO for other reasons (eg, to read a password).

Dolske, any thoughts?
Flags: needinfo?(dolske)
Flags: firefox-backlog+
See also bug 853549 which looks like it may impact this
Bug 853549 isn't really related; the password manager code implementation is completely separate from the NSS/PSM code involved with these files.

I think the best fix is what Vladan suggested -- just move the check later. I'm pretty sure IO to these files is inevitable when NSS/PSM is initialized, that lots of things are going to be triggering that initialization fairly early on. I'm actually surprised it doesn't happen by delayedStartup().
Flags: needinfo?(dolske)
Whiteboard: p=8 → p=1 [qa+]
(In reply to Justin Dolske [:Dolske] from comment #2)
> I think the best fix is what Vladan suggested -- just move the check later.

Cool.

> (In reply to Vladan Djeric (:vladan) from bug 1013448 comment 21)
> > I think you can fix this by moving the Telemetry work out of
> > _delayedStartup. Either do it after sessionstore-windows-restored (i.e.
> > listen for this event and then do the work in a setTimeout) or at browser
> > exit ("profile-before-change" event).

This patch uses the existing use of SessionStore.promiseInitialized, then creates a 5 second timer before doing the telemetry dance.

Try at https://tbpl.mozilla.org/?tree=Try&rev=865bbf3459ca which includes the xperf test.
Assignee: nobody → mhammond
Attachment #8428941 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8437382 - Flags: review?(dolske)
Comment on attachment 8437382 [details] [diff] [review]
0001-Bug-1016138-Add-telemetry-probe-for-master-password-.patch

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

r+ for the timeout fix, r- for the probe type I just noticed.

::: browser/base/content/browser.js
@@ +1227,5 @@
>        SocialUI.init();
>        TabView.init();
> +
> +      // Telemetry for master-password - we do this after 5 seconds as it
> +      // will cause some IO.

"...as it can cause IO if NSS/PSM has not already initialized".

::: toolkit/components/telemetry/Histograms.json
@@ +6347,5 @@
>      "description": "Whether FxA Sync is configured to use a custom token server"
> +  },
> +  "MASTER_PASSWORD_ENABLED": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",

Err, I think this should be "flag" not "boolean". This value is getting set every time a window opens, so we don't want the value incremented each time.

[Now that I think about it, the probe could check in nsBrowserGlue so it's once-per-session, but even then this should be "flag" so the data isn't skewed by how many sessions a user had.]
Attachment #8437382 - Flags: review?(dolske) → review-
Fixed comment and probe type.  Note I still unconditionally call .add with a boolean, but this seems to work fine.
Attachment #8437382 - Attachment is obsolete: true
Attachment #8439707 - Flags: review?(dolske)
Attachment #8439707 - Flags: review?(dolske) → review+
https://hg.mozilla.org/mozilla-central/rev/cff3a3c904b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Added to Iteration 33.1
Whiteboard: p=1 [qa+] → p=1 s=33.1 [qa+]
QA Contact: andrei.vaida
I was able to confirm that the WEAVE_CONFIGURED_MASTER_PASSWORD probe is working as expected on Nightly 33.0a1 (2014-06-15), using:
 * Windows 7 64-bit [1],
 * Ubuntu 12.04 LTS 32-bit [2],
 * Mac OS X 10.9.2 [3].

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0
[2] Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0
Status: RESOLVED → VERIFIED
Whiteboard: p=1 s=33.1 [qa+] → p=1 s=33.1 [qa!]
This crashes on start when e10s is enabled(today's nightly). Please let me know if you would like me to file a new bug or re-open this one.
Flags: needinfo?(mhammond)
discussed on irc with markh & billm.
Attachment #8441024 - Flags: review?(mhammond)
Comment on attachment 8441024 [details] [diff] [review]
markhBandaid crash fix

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

Thanks!
Attachment #8441024 - Flags: review?(mhammond) → review+
Pushed the followup as https://hg.mozilla.org/integration/fx-team/rev/63dacdff674c

Re-opening and resetting [qa] flag so this can be reverified.

Sorry all for the noise...
Status: VERIFIED → REOPENED
Flags: needinfo?(mhammond)
Resolution: FIXED → ---
Whiteboard: p=1 s=33.1 [qa!] → p=1 s=33.1 [qa+]
Ack! If something isn't being backed out, you should never reopen a bug. New bugs are cheap!
https://hg.mozilla.org/mozilla-central/rev/63dacdff674c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
I re-verified this issue on the 2014-06-15 and 2014-06-16 Nightly builds with the "browser.tabs.remote.autostart" and "browser.tabs.remote" prefs set to true (e10s enabled) under:
 * Windows 7 64-bit, 
 * Ubuntu 14.04 LTS 32-bit,
 * Mac OS X 10.9.2,
but I was unable to reproduce the crash on neither of those builds, nor on the latest Nightly 33 (2014-06-17). I also had the same attempt using a "New e10s Window" instead of pref changes, but with the same result.

(In reply to :Allison Naaktgeboren from comment #10)
> This crashes on start when e10s is enabled(today's nightly). Please let me
> know if you would like me to file a new bug or re-open this one.
Allison, could you please provide STR on this matter and perhaps some information about your testing environment?
Flags: needinfo?(ally)
Windows 8, local debug build of fx-team, "browser.tabs.remote.autostart" and "browser.tabs.remote" prefs set to true.

STR was
0) profile with e10s enabled
1) mach build 
-- (You'll need a build of fx-team|m-c before my followup patch landed. I'd pull the m-c version listed in Comment 7)
2) mach run
3) crash
4) windows prompt to attach debugger
5) pull stack
Flags: needinfo?(ally)
Thank you for the detailed instructions Allison, but unfortunately I'm still unable to reproduce the crash for some reason. I've used two different Windows 8 machines (32-bit and 64-bit) with both the fx-team and m-c builds that do not contain your patch.

Could you please confirm this fix on your end? I already re-ran my tests on Nightly 33 (2014-06-18) and didn't find any issues.
Flags: needinfo?(ally)
Fix was confirmed on my end. (Since I work on e10s full time, it was quite the showstopper)
Flags: needinfo?(ally)
Hi Andrei, since Ally confirmed the fix on her end is there anything else left before verifying this bug?
Flags: needinfo?(andrei.vaida)
(In reply to Marco Mucci [:MarcoM] from comment #20)
> Hi Andrei, since Ally confirmed the fix on her end is there anything else
> left before verifying this bug?

This issue is now verified fixed. Thanks again Allison for your help!
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
Whiteboard: p=1 s=33.1 [qa+] → p=1 s=33.1 [qa!]
You need to log in before you can comment on or make changes to this bug.