Closed Bug 1236588 Opened 4 years ago Closed 4 years ago

Remove the storage DB for HealthReport from user profiles

Categories

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

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 2 obsolete files)

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?
Blocks: 1209088
Points: --- → 2
Priority: -- → P2
Whiteboard: [measurement:client]
Assignee: nobody → alessio.placitelli
Priority: P2 → P1
Attached patch bug1236588.patch (obsolete) — Splinter Review
Attachment #8705203 - Flags: review?(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.
Attachment #8705203 - Flags: review?(gfritzsche)
Attached patch bug1236588.patch (obsolete) — Splinter Review
Thanks. You're right, OS.File blocks on writes during shutdown.
Attachment #8705203 - Attachment is obsolete: true
Attachment #8706848 - Flags: review?(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.
Attachment #8706848 - Flags: review?(gfritzsche) → review+
Attached patch bug1236588.patchSplinter Review
Attachment #8706848 - Attachment is obsolete: true
Attachment #8706976 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/eb992ed0700f1b223a62c2dd1f39d7d79ff1c90d
Bug 1236588 - Remove the storage DB for HealthReport from user profiles. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/eb992ed0700f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.