807842 - Data provider for profile metadata

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Comment 1

[Gregory Szorc [:gps]]

bsmedberg and I were discussing a more proper way of doing this that isn't based off of querying the root bookmarks node in Places...

Comment 2

[Richard Newman [:rnewman]]

Switching this to timestamp (rounded to day) rather than age, because it's exactly the same info but invariant over time.

Comment 3

[Richard Newman [:rnewman]]

Yeah, I was thinking of extending nsIToolkitProfile. Would very much welcome suggestions for the best way to get the data…

Comment 4

[Richard Newman [:rnewman]]

Turns out that profiles are crufty things. Mine is from 2004 (!). The oldest files, in terms of creation date:

 Jun 18 2008 places.sqlite
 Jun 18 2008 cookies.sqlite
 Jun 18 2008 permissions.sqlite
 Jun 18 2008 minidumps
 Jun 18 2008 bookmarks.bak
 Jun 18 2008 bookmarks.html
 Apr 15 2008 dh-media-lists.rdf
 Mar 31 2008 operator
 Mar 28 2008 webappsstore.sqlite
 Mar  1 2008 formhistory.dat
 Mar  1 2008 cert8.db
 Mar  1 2008 history.dat
 Mar  1 2008 key3.db
 Mar  1 2008 urlclassifier2.sqlite
 Mar  1 2008 extensions
 Mar  1 2008 kf.txt
 Mar  1 2008 bookmarkbackups
 Jan  2 2008 signons2.txt
 Jul 31 2007 compatibility.ini
 Feb 13 2007 hints-fieldname.properties
 Feb 13 2007 hints-label.properties
 Jan 25 2007 searchplugins
 Oct 24 2006 search.sqlite
 Apr 12 2006 bookmarks-6.html
 Apr  9 2006 bookmarks-5.html
 Apr  5 2006 bookmarks-4.html
 Oct 10 2005 bookmarks-3.html
 Sep 27 2005 bookmarks-2.html
 Sep 27 2005 Cache.Trash
 Sep  5 2005 chrome
 Sep  5 2005 components.ini
 Sep  5 2005 defaults.ini
 Jun 21 2005 bookmarks-1.html
 Mar 23 2005 search.rdf
 Apr  6 2004 secmod.db
 Jan 27 2004 cookperm.txt


Note that we don't even get to places.sqlite until 2008. Sure enough, the dateAdded for my places root node:

  Wed Jun 18 17:50:08 PDT 2008

So we can't use that. The appropriate algorithm here seems to be to check for, in order:

  secmod.db
  defaults.ini
  search.sqlite
  searchplugins
  cert8.db
  places.sqlite

and use the creation date of the earliest found item.

Sound unreasonable to anyone?

Comment 5

[Richard Newman [:rnewman]]

OS.File is returning an incorrect creation date on my machine. Filed.

Comment 6

[Gregory Szorc [:gps]]

Not all filesystems record created time. Not all do it the way you think.

We should probably have the profile write out a file at profile creation time which records basic metadata, such as the current time. This doesn't solve our problem of identifying the age of existing profiles, unfortunately.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #6)
> Not all filesystems record created time. Not all do it the way you think.

HFS+ does. NTFS does. I think this is a viable approach (if applied carefully), considering the years of evolution in profile directories.

We're just getting the wrong value back on my OS/Fx/FS combination. It disagrees with `ls`, so I'm hopeful that it can be fixed.

> We should probably have the profile write out a file at profile creation
> time which records basic metadata, such as the current time. This doesn't
> solve our problem of identifying the age of existing profiles, unfortunately.

Exactly. I would *like* for us to do both -- save this file-walking for old profiles, even caching the value in the profile directory once we've figured it out once. But we still need *a* way of doing it, lest we appear to have acquired all of our users in 2008 (or 2012, if we're really lazy).

Comment 8

[Daniel Einspanjer [:dre] [:deinspanjer]]

One thing I think we should consider is that we are very unlikely to have any sort of system for calculating the profile creation that is completely reliable.  If the machine clock is set to a date in the past when one of these files or the root node of the places DB is created, then it will have that incorrect date.  We have created a few rough guidelines for how we will deal with potentially skewed dates on the server side, but I think that it would be fine to build a system that works fairly well in the normal case, and accept the possibility that there will be outliers with very inaccurate creation times.

Even in rnewman's case, For analytic purposes, there is no significant difference between a profile age of 2008 vs 2004.  It would only become an issue if a significant number of profiles reported that they were created last week when they were actually created a year ago.

Comment 9

[Benjamin Smedberg]

Why aren't you taking the ctime of the Firefox profile directory itself, instead of one of those files inside it?

Also please don't do anything with nsIToolkitProfile, it's cumbersome and we normally try not to expose that interface during normal runtime. Just expose some API via a .jsm which operates on the profile directory.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Why aren't you taking the ctime of the Firefox profile directory itself,
> instead of one of those files inside it?

Because on my machine it reports the folder as having been created March 1 2008, which is late.

I imagine that folder contents will *generally* be more reliable, but this is all hacky anyway…


> Also please don't do anything with nsIToolkitProfile, it's cumbersome and we
> normally try not to expose that interface during normal runtime. Just expose
> some API via a .jsm which operates on the profile directory.

I'm using OS.File to do the actual work; I only use nsIToolkitProfileService (and .selectedProfile, .rootDir.path) to get to a path that I can give to OS.File.

Is there a better way to get that?

I would be very happy for OS.File to offer an accessor for the current profile directory!


(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #8)
> One thing I think we should consider is that we are very unlikely to have
> any sort of system for calculating the profile creation that is completely
> reliable.  If the machine clock is set to a date in the past when one of
> these files or the root node of the places DB is created, then it will have
> that incorrect date. 

There's more than that — files have been introduced and deprecated over time, and there's backup/restore to consider. That's why I was planning to scan the contents of the profile directory for candidates.

> We have created a few rough guidelines for how we will
> deal with potentially skewed dates on the server side, but I think that it
> would be fine to build a system that works fairly well in the normal case,
> and accept the possibility that there will be outliers with very inaccurate
> creation times.

If you're happy with an age bucket which is "more than 3 years", say, and you're happy documenting that we'll have no reliable mechanism for analyzing ages older than that (some might report as older, but there will be false negatives like my profile), then that could save us some complexity.

As with previous discussions, it's important to capture this somewhere, both for posterity and to avoid confusing analysts!


> Even in rnewman's case, For analytic purposes, there is no significant
> difference between a profile age of 2008 vs 2004.  It would only become an
> issue if a significant number of profiles reported that they were created
> last week when they were actually created a year ago.

That would be the case if we used the Places time, because several repair mechanisms (profile reset, Places' own fixers, manual steps) involve replacing places.sqlite. I'd be much more confident in using a filesystem timestamp on supported platforms.

Comment 11

[Benjamin Smedberg]

You should absolutely *not* use nsIToolkitProfile to get the profile directory! You should be using OS.Constants.Path.profileDir.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> You should absolutely *not* use nsIToolkitProfile to get the profile
> directory! You should be using OS.Constants.Path.profileDir.

Good to know.

I will update MDN, which neither warns about not using nsIToolkitProfileService, nor mentions Path.profileDir in OS.Constants at all!

(Seriously, how is anyone supposed to know that this stuff exists?)

Comment 13

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #12)

> I will update MDN, which neither warns about not using
> nsIToolkitProfileService, nor mentions Path.profileDir in OS.Constants at
> all!

Done: nsIToolkitProfileService marked with a warning box and alternatives; Path.profileDir added to OS.Constants.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to Richard Newman [:rnewman] from comment #12)
> I will update MDN, which neither warns about not using
> nsIToolkitProfileService, nor mentions Path.profileDir in OS.Constants at
> all!
> 
> (Seriously, how is anyone supposed to know that this stuff exists?)

Hey, don't hesitate to add the link. I am a one man team working on OS.File and plenty of other stuff. So far, I have not succeeded at being everywhere at once :)

Comment 15

[Richard Newman [:rnewman]]

Aligning bug title to Bug 809644, making more general to decrease granularity.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Draft patch. v1

Of note:

* We will probably roll this into the monolithic provider file introduced in Bug 809644. I see no need to do this right now; it just adds coupling between parallel work streams. Needs to happen before landing.

* There's a test for OS.File included. I think that's prudent for the very short term.

* This will be greatly improved by Bug 808263, which is what I'm going to address now.

* WIP, so only f? for now.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 679467 [details] [diff] [review]
Draft patch. v1

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

::: services/healthreport/profile.jsm
@@ +11,5 @@
> +const {utils: Cu, classes: Cc, interfaces: Ci} = Components;
> +
> +Cu.import("resource://gre/modules/services/metrics/dataprovider.jsm");
> +Cu.import("resource://gre/modules/commonjs/promise/core.js");
> +Cu.import("resource://services-common/log4moz.js");

core.js nor log4moz.js are needed AFAICT.

@@ +19,5 @@
> +// Get access to OS.Constants.
> +Cc["@mozilla.org/net/osfileconstantsservice;1"].
> +  getService(Components.interfaces.nsIOSFileConstantsService).
> +  init();
> +    

Nit: ws

I would also love to see this in a test-only module provided by OS.File itself.

@@ +20,5 @@
> +Cc["@mozilla.org/net/osfileconstantsservice;1"].
> +  getService(Components.interfaces.nsIOSFileConstantsService).
> +  init();
> +    
> +const DEFAULT_PROFILE_MEASUREMENT_NAME = "org.mozilla.profile";

We should establish a naming convention...

@@ +95,5 @@
> +
> +    function onSuccess() {
> +      iterator.close();
> +      log.debug("Oldest timestamp is " + oldest);
> +      return Math.floor(oldest / 1000000);        // Millseconds -> days.

1000000?

@@ +112,5 @@
> +    let name = DEFAULT_PROFILE_MEASUREMENT_NAME;
> +
> +    result.expectMeasurement(name);
> +    result.addMeasurement(new ProfileMetadataMeasurement(name));
> +    result.setValue(name, "profileCreation", this.getProfileCreationDays());

Aren't you doing async stuff in getProfileCreationDays()? You should chain off of the return from getProfileCreationDays and call result.finish() from within that. I fail to see how this code is even working as is.

@@ +115,5 @@
> +    result.addMeasurement(new ProfileMetadataMeasurement(name));
> +    result.setValue(name, "profileCreation", this.getProfileCreationDays());
> +
> +    result.finish();
> +    return result;

Need to use the new API. Code looks good though.

::: services/healthreport/tests/xpcshell/test_profile.js
@@ +28,5 @@
> +    run_next_test();
> +  }, function onFail() {
> +    iterator.close();
> +    do_throw("Iterating over current directory failed.");
> +    run_next_test();

Do you need run_next_test() after do_throw()? I thought do_throw, well, threw.

@@ +50,5 @@
> +};
> +
> +add_test(function test_profile_files() {
> +  let provider = new ProfileMetadataProvider();
> +  // This doesn't work in xpcshell -- no profile directory.

Really? We do create a profile directory in the xpcshell runner. I wonder what isn't getting the message.

@@ +61,5 @@
> +  do_check_true(!!failed);
> +
> +  // But we can iterate over an explicit directory.
> +  function onSuccess(answer) {
> +    let now = Date.now() / 1000000;     // Milliseconds -> days.

1000000 again?!

@@ +76,5 @@
> +});
> +
> +add_test(function test_collect_constant() {
> +  let result = new MockProfileMetadataProvider().collectConstantMeasurements();
> +  

Nit: ws

Comment 18

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Here's something that's a lot less WIPey. I split the date provider into a standalone class, and tested that individually. If the creation time isn't there (possible for both legacy and new profiles, even after Bug 808263), it figures it out and writes it.

The provider follows the new API.

I have *not* yet tested this with a real profile; xpcshell only.

Urgh, promises are unpleasant to indent.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 686021 [details] [diff] [review]
Proposed patch. v1

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

Looking pretty good. The race condition for stat() is why this doesn't get r+. I'd also like answers on code ownership (which might be lurking in the comments here and elsewhere).

I also held off reviewing test_profile.js: I think once you discover toolkit/content/Task.jsm, you'll want to rewrite large parts of it :)

If you want me to look at that file, just put it up for review again.

::: services/healthreport/Makefile.in
@@ +11,5 @@
>  
>  modules := \
>    healthreporter.jsm \
>    policy.jsm \
> +	profile.jsm \

Nit: 2 spaces, not a tab. (Your editor probably does tabs automatically for makefiles.)

::: services/healthreport/profile.jsm
@@ +10,5 @@
> +];
> +
> +const {utils: Cu, classes: Cc, interfaces: Ci} = Components;
> +
> +const DEFAULT_PROFILE_MEASUREMENT_NAME = "org.mozilla.profile";

I reckon we should standardize our naming convention.

@@ +23,5 @@
> +
> +// Get access to OS.Constants.
> +Cc["@mozilla.org/net/osfileconstantsservice;1"].
> +  getService(Components.interfaces.nsIOSFileConstantsService).
> +  init();

Huh? I thought you could just import osfile.jsm and OS.Constants would magically be available.

Yoric: am I wrong?

@@ +28,5 @@
> +
> +// Profile creation time access.
> +// This is separate from the provider to simplify testing and enable extraction
> +// to a shared location in the future.
> +function ProfileCreationTimeAccessor(profile) {

Not sure why this is in health report code. Ideally it would live in profile code land.

@@ +45,5 @@
> +   *
> +   * If we have to calculate, we write out the file; if we have
> +   * to touch the file, we persist in-memory.
> +   */
> +  get created() {

I'm not sure how I feel about a getter returning a promise. Although, if you go the "all values are promises" route, I suppose it makes sense.

Nit: document return type in docblock

@@ +54,5 @@
> +    function onSuccess(times) {
> +      if (times && times.created) {
> +        return this._created = times.created;
> +      }
> +      return this._created = this.computeAndPersistTimes(times);

This threw me for a loop initially because the first return returns a number and the 2nd a promise. Oh, the magic of promises!

@@ +88,5 @@
> +    let path = OS.Path.join(this.profilePath, file);
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(JSON.stringify(contents));
> +    return OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"});
> +  },

Move to services/common/utils.js or FileUtils.jsm?

@@ +103,5 @@
> +      contents.created = oldest;
> +      return this.writeTimes(contents, path)
> +                 .then(function onSuccess() {
> +                   return oldest;
> +                 });

I don't like how the code for managing this file is scattered across modules. I'd love to see a common interface to ensure things don't get out of sync.

@@ +146,5 @@
> +    let promise = iterator.forEach(onEntry);
> +
> +    function onSuccess() {
> +      iterator.close();
> +      return oldest;

There is no gating on OS.File.stat above. Therefore, couldn't the iterator finish before a stat() has completed, returning possibly bad data in the process?

Comment 20

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #19)

> I'd also like answers on code ownership (which might be lurking in the
> comments here and elsewhere).

Mossop expressed in Bug 808263 that he wanted FHR to create the times file if it didn't exist. I'd be happy for that logic to eventually live somewhere else, closer to nsToolkitService, but (a) that's C++, (b) I think people want it to die and not get any larger, (c) I don't know of anywhere better.

I opted to just make the accessor standalone and separable, and commented to that effect. I'm happy to pull that out and put it somewhere else at a later date.


> Nit: 2 spaces, not a tab. (Your editor probably does tabs automatically for
> makefiles.)

Happens every time. Sometimes even when I've already fixed it once.


> > +const DEFAULT_PROFILE_MEASUREMENT_NAME = "org.mozilla.profile";
> 
> I reckon we should standardize our naming convention.

I would like to standardize on this :D


> Huh? I thought you could just import osfile.jsm and OS.Constants would
> magically be available.

Apparently that's now the case, or importing the profile in the test changed something. I'm removing those lines and adding a test check.


> > +// Profile creation time access.
> > +// This is separate from the provider to simplify testing and enable extraction
> > +// to a shared location in the future.
> > +function ProfileCreationTimeAccessor(profile) {
> 
> Not sure why this is in health report code. Ideally it would live in profile
> code land.

"Ideally" is why. If you can name a JS component that is a good home for it, I'd be very happy to move it. Or if you think it's meaningful enough to live in toolkit on its own, then I can do that too.


> > +    let path = OS.Path.join(this.profilePath, file);
> > +    let encoder = new TextEncoder();
> > +    let array = encoder.encode(JSON.stringify(contents));
> > +    return OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"});
> > +  },
> 
> Move to services/common/utils.js or FileUtils.jsm?

Lifted readJSON and writeJSON to CommonUtils. readTimes and writeTimes are now wrappers around those.

Bear in mind that this now imports OS.File into CommonUtils.

I didn't want to 'pollute' FileUtils with OS.File and modern JS ;)


> I don't like how the code for managing this file is scattered across
> modules. I'd love to see a common interface to ensure things don't get out
> of sync.

If by "scattered" you mean "exists in one C++ file and one JavaScript file", then sure.

This is the *only* JS interface that reads or writes times.json; ProfileCreationTimeAccessor exists to abstract that file access.

I understand your concern, but I think it would be overkill to extend nsIToolkitProfileService to intermediate a simple file read/write.


> There is no gating on OS.File.stat above. Therefore, couldn't the iterator
> finish before a stat() has completed, returning possibly bad data in the
> process?

Yoric's example does not:

https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.DirectoryIterator_for_the_main_thread

but I've added a return for the stat promise, which I believe will wait for completion before processing the next file.

Comment 21

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Reflects review comments, uses Task.spawn.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 686467 [details] [diff] [review]
Proposed patch. v2

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

::: services/common/utils.js
@@ +354,5 @@
> +  writeJSON: function(contents, path) {
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(JSON.stringify(contents));
> +    return OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"});
> +  },

Where's the direct test coverage?

Please file a follow-up to implement jsonLoad and jsonSave in terms of this and to deprecated jsonLoad and jsonSave.

::: services/healthreport/profile.jsm
@@ +26,5 @@
> +/*
> +Cc["@mozilla.org/net/osfileconstantsservice;1"].
> +  getService(Components.interfaces.nsIOSFileConstantsService).
> +  init();
> +*/

Nuke it?

@@ +43,5 @@
> +   * There are three ways we can get our creation time:
> +   *
> +   * 1. From our own saved value (to avoid redundant work)
> +   * 2. From the on-disk JSON file
> +   * 3. By calculating it from the filesystem.

Nit: Period consistency.

@@ +73,5 @@
> +  },
> +
> +  getPath: function (file) {
> +    return OS.Path.join(this.profilePath, file);
> +  },

I think this function's existence might be a side-effect of your time with Java. I know DRY, but sheesh ;)

@@ +96,5 @@
> +   * Merge existing contents with a 'created' field, writing them
> +   * to the specified file. Promise, naturally.
> +   */
> +  computeAndPersistTimes: function (existingContents, file="times.json") {
> +    let path = OS.Path.join(this.profilePath, file);

Kill or use this.getPath().

@@ +99,5 @@
> +  computeAndPersistTimes: function (existingContents, file="times.json") {
> +    let path = OS.Path.join(this.profilePath, file);
> +
> +    function onOldest(oldest) {
> +      let contents = existingContents || {};

Use default argument value in function?

@@ +195,5 @@
> +}
> +ProfileMetadataProvider.prototype = {
> +  __proto__: MetricsProvider.prototype,
> +
> +  getProfileCreationDays: function getProfileCreationDays() {

Nit: Clear function names in this prototype

::: services/healthreport/tests/xpcshell/test_profile.js
@@ +51,5 @@
> +
> +  // Ensure that we get constants, too.
> +  do_check_neq(OS.Constants.Path.profileDir, null);
> +
> +  let iterator = new OS.File.DirectoryIterator(".");

Should this not be OS.Constants.Path.profileDir?

Comment 23

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #22)

> Where's the direct test coverage?

Added to test_utils_json.
 
> Please file a follow-up to implement jsonLoad and jsonSave in terms of this
> and to deprecated jsonLoad and jsonSave.

Elected not to touch this for sanity reasons.

> > +    function onOldest(oldest) {
> > +      let contents = existingContents || {};
> 
> Use default argument value in function?

As we discussed, not equivalent semantics.

> > +  let iterator = new OS.File.DirectoryIterator(".");
> 
> Should this not be OS.Constants.Path.profileDir?

As we discussed, doesn't make a difference, and works if there's no profile dir.

New patch coming momentarily.

Comment 24

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Comment 25

[Richard Newman [:rnewman]]

Note: can't do final testing in Nightly until Bug 820106 is addressed.

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 689884 [details] [diff] [review]
Proposed patch. v3

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

You will also need some registration magic in the .manifest file for this to get hooked up inside the application.

::: services/common/tests/unit/test_utils_json.js
@@ +133,5 @@
> +  function doRead() {
> +    CommonUtils.readJSON(path)
> +               .then(checkJSON, do_throw);
> +  }
> +    

Nit: WS

::: services/healthreport/tests/xpcshell/test_profile.js
@@ +94,5 @@
> +             .then(function onSuccess(json) {
> +               print("Read: " + JSON.stringify(json));
> +               do_check_eq(12345, json.created);
> +               run_next_test();
> +             });

let json = yield acc.readTimes("test.json");
print("Read: " + JSON.stringify(json));
...

@@ +123,5 @@
> +               print("Read: " + JSON.stringify(json));
> +               do_check_eq("123", json.abc);
> +               do_check_eq("abc", json.easy);
> +               do_check_eq(expected, json.created);
> +             });

Ditto.

Comment 27

[Richard Newman [:rnewman]]

I have this with manifest change lined up ready to land as soon as Yoric figures out what's going on with profileDir.

Comment 28

[Richard Newman [:rnewman]]

Fixed and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49e7e261603

Comment 29

[Ed Morley [:emorley]]

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