1138322 - TelemetryEnvironment.jsm should not use |ProfileTimesAccessor|

Status: RESOLVED FIXED

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

Description

[Alessio Placitelli [:Dexter]]

test_TelemetryEnvironment.js fails for Thunderbird builds (see bug 1137412) because |ProfileTimesAccessor| [1] is not available.

TelemetryEnvironment.jsm should not use that object or, better, ProfileTimesAccessor should be available when FHR is not built in: ProfileTimesAccessor falls back to profile age manual calculation if no "times.json" file is available.

[1] https://hg.mozilla.org/mozilla-central/annotate/eea6188b9b05/services/healthreport/profile.jsm

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1138322_telemetry_not_profileaccessor.patch

Try is closed atm, works locally though.

Comment 2

[Alessio Placitelli [:Dexter]]

|ProfileTimesAccessor| also computes the creation date of the profile if no "times.json" is available. It traverses the profile directory and looks for the oldest file available there [1]. I don't know how frequently that happens, but I think that behaviour should be preserved. Maybe it's better getting |ProfileTimesAccessor| out of FHR or creating a similar object in Telemetry?

Also, as Georg will not be available before March 15th: maybe :vladan is the best person to review this.

[1] - https://hg.mozilla.org/mozilla-central/annotate/44bcd21e59fe/services/healthreport/profile.jsm#l118

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1138322_move_ProfileTimesAccessor.patch

Yeah, might be better to just make it an independent component.

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

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 8573460 [details] [diff] [review]
bug1138322_move_ProfileTimesAccessor.patch

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

::: services/healthreport/profile.jsm
@@ +22,5 @@
>  
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/osfile.jsm")
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");

doesn't FHR need to import ProfileTimesAccessor.jsm now as well?

::: toolkit/modules/ProfileTimesAccessor.jsm
@@ +18,5 @@
> + * Profile access to times.json (eg, creation/reset time).
> + * This is separate from the provider to simplify testing and enable extraction
> + * to a shared location in the future.
> + */
> +this.ProfileTimesAccessor = function(profile, log) {

if we're gonna make this into a JSM, let's give it a more descriptive name.. how about ProfileAge?

Comment 5

[Vladan Djeric (:vladan)]

Btw I think the profile age info should be cached, either in TelemetryEnvironment or ProfileAge.jsm (preferably the latter). That's work for another bug though

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Magnus Melin from comment #3)
> Created attachment 8573460 [details] [diff] [review]
> bug1138322_move_ProfileTimesAccessor.patch
> 
> Yeah, might be better to just make it an independent component.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2d62a276830

Nice work! There are some tests failing because FHR is not importing ProfileTimesAccessor.jsm as :vladan mentioned.

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: bug1138322_move_ProfileTimesAccessor.patch

Make it ProfileAge.jsm and add the inclusion I missed earlier.

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

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

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