1236588 - Remove the storage DB for HealthReport from user profiles

Status: RESOLVED FIXED

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

Description

[Alessio Placitelli [:Dexter]]

With FHR being removed, we should make sure sure the Health Report database file ("healthreport.sqlite") in the user's profile gets removed as well.

We should add DB removal code for a few releases in the startup code path:

- Is there a known/common code path for this kind of things?
- If not, should we add that to Telemetry initialization code?

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1236588.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8705203 [details] [diff] [review]
bug1236588.patch

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

OS.File should be blocking shutdown on pending operations. Also, none of Telemetry needs to wait on this cleanup, so i don't think we need to track the tasks at all here.

Lets also comment clearly on this being a temporary measure that we should drop in the future.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1236588.patch

Thanks. You're right, OS.File blocks on writes during shutdown.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8706848 [details] [diff] [review]
bug1236588.patch

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

Looks good with the below fixed.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +418,5 @@
> +   * Remove FHR database files. This is temporary and will be dropped in
> +   * the future.
> +   * @return {Promise} Resolved when the database files are deleted.
> +   */
> +  dropFHRDatabase: function() {

removeFHRDatabase() or delete...() or cleanup...(), drop...() is a little vague.

@@ +1739,5 @@
> +
> +    // FHR could have used either the default DB file name or a custom one
> +    // through this preference.
> +    const FHR_DB_FILENAME =
> +      Preferences.get("datareporting.healthreport.dbName", "healthreport.sqlite");

This sounds like potential for left-over files.
Please try removing both healthreport.sqlite and the DB files with the custom name (if any).

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +480,5 @@
> +
> +  // Trigger the cleanup and check that the files were removed.
> +  yield TelemetryStorage.dropFHRDatabase();
> +  for (let dbFilePath of CUSTOM_DB_PATHS) {
> +    Assert.ok(!(yield OS.File.exists(dbFilePath)), dbFilePath + " must not be on the disk anymore.");

Nit: Put the path on the end of the message for readability.

@@ +503,5 @@
> +
> +  // Trigger the cleanup and check that the files were removed.
> +  yield TelemetryStorage.dropFHRDatabase();
> +  for (let dbFilePath of DEFAULT_DB_PATHS) {
> +    Assert.ok(!(yield OS.File.exists(dbFilePath)), dbFilePath + " must not be on the disk anymore.");

Ditto.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1236588.patch

Comment 6

[Alessio Placitelli [:Dexter]]

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

Comment 7

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/eb992ed0700f1b223a62c2dd1f39d7d79ff1c90d
Bug 1236588 - Remove the storage DB for HealthReport from user profiles. r=gfritzsche

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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