Closed Bug 808263 Opened 8 years ago Closed 8 years ago

Write profile creation time to the profile on creation

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 2 obsolete files)

There are a bunch of ways one might choose to implement this — a transaction log of profile operations (creation, reset, crash…), a single file containing a Unix timestamp, whatever — but for Firefox Health Report we'd like an unambiguous way to know when a profile was first created.

For now we're going to be sniffing file creation times, poking around in databases, etc., but that's really janky (not least because none of those methods are reliable across platforms and profile ages).

This bug is to fix this issue the right way.

A solution that ships in Firefox 18 (or earlier) would be Most Splendid®.
Attached patch Proposed patch. v1 (obsolete) — Splinter Review
This patch extends the profile creation code in nsIToolkitProfileService to write out a "times.json" file immediately after creating the profile directory.

This is *perhaps* overkill -- we could instead rely on some JS code that we trust will always run at startup -- but it seems to work, so hey.

No tests, so only asking for feedback for now.

Bear in mind that my C++ is very rusty, NSPR is hideous, and I'm otherwise not confident in this being viewed as correct, so thorough feedback is welcome.

I'd also like to know where I should put a test for this.

Dietrich has the closest blame to these changes, so f? goes to him; please redirect if there's a better candidate!
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #680238 - Flags: feedback?(dietrich)
Attachment #680238 - Flags: feedback?(dietrich) → feedback?(dtownsend+bugmail)
Comment on attachment 680238 [details] [diff] [review]
Proposed patch. v1

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

Are you comfortable with this only working for profiles created pro-grammatically?
I'm not convinced json is a good format for this, it seems likely that if we were to use this file for other things in the future then it is going to have to be easily parseable by C++. json seems like it might be a little heavyweight.
Attachment #680238 - Flags: feedback?(dtownsend+bugmail) → feedback-
Thanks for the speedy response!

> Are you comfortable with this only working for profiles created
> pro-grammatically?

Is there another method of creating profiles?

This is all new to me.

(Ideally we have a JS hook which runs on profile first-run…)

> I'm not convinced json is a good format for this, it seems likely that if we
> were to use this file for other things in the future then it is going to
> have to be easily parseable by C++. json seems like it might be a little
> heavyweight.

I'm erring on the side of "easily parseable by JavaScript", to be honest, because the only consumer right now will be FHR, and that's time-constrained.

I don't care about the detailed format (I opted for a structured format rather than just a timestamp in a file, because it seems like we'll want to record things like Firefox Resets in the future), so long as it's well specified (because FHR will be writing a replacement for old profiles) and easy for JS to parse.

I would be very happy for someone more intimately acquainted with the profile code to crank this out and land it this week, if you'd rather do that. Otherwise, I'd appreciate rapid iteration for me to get it done.

Thanks!
Flags: needinfo?(dtownsend+bugmail)
(In reply to Richard Newman [:rnewman] from comment #3)
> Thanks for the speedy response!
> 
> > Are you comfortable with this only working for profiles created
> > pro-grammatically?
> 
> Is there another method of creating profiles?
> 
> This is all new to me.
> 
> (Ideally we have a JS hook which runs on profile first-run…)

mkdir foo
firefox -profile foo

Pretty sure this doesn't go through any of our profile creation code. It is also generally only used by QA, nightly testers, etc. but presumably we want FHR to work the same for them too so we probably need to support this case.

> > I'm not convinced json is a good format for this, it seems likely that if we
> > were to use this file for other things in the future then it is going to
> > have to be easily parseable by C++. json seems like it might be a little
> > heavyweight.
> 
> I'm erring on the side of "easily parseable by JavaScript", to be honest,
> because the only consumer right now will be FHR, and that's time-constrained.
> 
> I don't care about the detailed format (I opted for a structured format
> rather than just a timestamp in a file, because it seems like we'll want to
> record things like Firefox Resets in the future), so long as it's well
> specified (because FHR will be writing a replacement for old profiles) and
> easy for JS to parse.
> 
> I would be very happy for someone more intimately acquainted with the
> profile code to crank this out and land it this week, if you'd rather do
> that. Otherwise, I'd appreciate rapid iteration for me to get it done.

Mostly I worry that you've chosen a format that is trivially parseable/serialisable by JS but much harder to handle from C++. Not sure how easy it is to spin up the JS engine to do it, I imagine it's possible but probably pretty heavyweight.
It probably doesn't matter all that much given that we have no specific additional use cases for this in the future yet, it just may mean we're left with having to have some migration code.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #4)

> Pretty sure this doesn't go through any of our profile creation code. It is
> also generally only used by QA, nightly testers, etc. but presumably we want
> FHR to work the same for them too so we probably need to support this case.

I imagine that will just end up using the age-detection fallback code, but yes, I'd like it all to work the same way if it can.


> It probably doesn't matter all that much given that we have no specific
> additional use cases for this in the future yet, it just may mean we're left
> with having to have some migration code.

OK. That works for me (YAGNI).

My outstanding questions:

* How do I write a test for this?
* What needs to happen for this code to handle the existing-directory case?
* What else needs to change for this to get r+?
(In reply to Richard Newman [:rnewman] from comment #5)
> My outstanding questions:
> 
> * How do I write a test for this?

It's going to be difficult. You could just use a browser chrome test and check that the timestamp in the file isn't too old. Hacky but likely easy. Anything more is likely to involve an xpcshell test to launch Firefox, but even then you can't test the real world case of a new default profile :(

> * What needs to happen for this code to handle the existing-directory case?

The guaranteed way is to take a perf hit and check for the file on every startup. The alternate is to have the FHR code create it if it doesn't exist using its fallback checking code.

> * What else needs to change for this to get r+?

I'd want to see the -profile case handled and at least the simple test case described above.
I'm seeing an oddity when I run my mochitest for this. I'm extending toolkit/profile/test/test_create_profile.xul.

During CreateProfileInternal (which is invoked indirectly from the createProfile call in JS), the profile directory is in one place. When control returns to the mochitest JS, it's somewhere else.

 0:03.66 CreateTimesInternal: /Users/rnewman/Library/Caches/Firefox/Profiles/3e8cck15.myprofile
 0:03.66 6 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/profile/test/test_create_profile.xul | Path is /Users/rnewman/Library/Application Support/Firefox/Profiles/3e8cck15.myprofile/times.json

Note how it hops from ~/Library/Caches to ~/Library/Application Support.

The directory in Caches contains just my file. The directory in AS has been populated with prefs.js etc., presumably later down the call path from createProfile. My test naturally fails, because the file isn't in the second location.

All of this works fine when I run the browser by hand.

Can you shed some light on what I'm doing wrong? I am a mochitest noob.
Flags: needinfo?(dtownsend+bugmail)
I'm seeing an oddity when I run my mochitest for this. I'm extending toolkit/profile/test/test_create_profile.xul.

During CreateProfileInternal (which is invoked indirectly from the createProfile call in JS), the profile directory is in one place. When control returns to the mochitest JS, it's somewhere else.

 0:03.66 CreateTimesInternal: /Users/rnewman/Library/Caches/Firefox/Profiles/3e8cck15.myprofile
 0:03.66 6 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/profile/test/test_create_profile.xul | Path is /Users/rnewman/Library/Application Support/Firefox/Profiles/3e8cck15.myprofile/times.json

Note how it hops from ~/Library/Caches to ~/Library/Application Support.

The directory in Caches contains just my file. The directory in AS has been populated with prefs.js etc., presumably later down the call path from createProfile. My test naturally fails, because the file isn't in the second location.

All of this works fine when I run the browser by hand.

Can you shed some light on what I'm doing wrong? I am a mochitest noob.
The Caches should never be the profile directory. It is the *local* profile directory ("ProfLD" versus "ProfD"). On OSes such as windows which separate out the roaming user data from the local data, we store persistent data (such as prefs and places) in the profile directory, and cache information such as the network cache and the safe browing list in the local profile directory. Somehow these two are getting confused for you.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> The Caches should never be the profile directory. It is the *local* profile
> directory ("ProfLD" versus "ProfD"). On OSes such as windows which separate
> out the roaming user data from the local data, we store persistent data
> (such as prefs and places) in the profile directory, and cache information
> such as the network cache and the safe browing list in the local profile
> directory. Somehow these two are getting confused for you.

Aha, localDir vs rootDir. Got it. Thanks Benjamin!
Flags: needinfo?(dtownsend+bugmail)
With test. FHR will independently create this file if it doesn't exist; I'll implement that in Bug 807842. Comment added to indicate that this code path won't always be taken.

Tested locally; pushing to try now.
Attachment #680238 - Attachment is obsolete: true
Attachment #682556 - Flags: review?(dtownsend+bugmail)
Let's do this right.
Attachment #682556 - Attachment is obsolete: true
Attachment #682556 - Flags: review?(dtownsend+bugmail)
Attachment #682575 - Flags: review?(dtownsend+bugmail)
Try run for dab261de908e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dab261de908e
Results (out of 56 total builds):
    exception: 15
    success: 1
    failure: 40
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rnewman@mozilla.com-dab261de908e
*waits patiently for correct try push*
Apparently I have to post this one myself. Looks good.

https://tbpl.mozilla.org/?tree=Try&rev=08f80c1314fa
Review ping, Mossop?
Attachment #682575 - Flags: review?(dtownsend+bugmail) → review+
Thanks Dave!

https://hg.mozilla.org/integration/mozilla-inbound/rev/964fe8cb452b
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/964fe8cb452b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 682575 [details] [diff] [review]
Write times in nsToolkitProfileService.createProfile. v2

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

::: toolkit/profile/nsToolkitProfileService.cpp
@@ +8,5 @@
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <prlong.h>
> +#include <prprf.h>
> +#include <prtime.h>

This should have used "" instead of <>.
(In reply to :Ms2ger from comment #19)

> > +#include <prlong.h>
> > +#include <prprf.h>
> > +#include <prtime.h>
> 
> This should have used "" instead of <>.

Why?

wrt all of the distinctions I know of, <> is the correct choice here.

Is this something that needs fixing? What problem does it cause?

And why would this file be any different from the other files in the tree that use <> for PR headers?
Target Milestone: mozilla20 → ---
Try run for dab261de908e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dab261de908e
Results (out of 57 total builds):
    exception: 15
    success: 1
    failure: 41
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rnewman@mozilla.com-dab261de908e
Try run for 08f80c1314fa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=08f80c1314fa
Results (out of 309 total builds):
    success: 292
    warnings: 16
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rnewman@mozilla.com-08f80c1314fa
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.