1366067 - Use JSONFile.jsm to write Sync metadata files

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler [:lina]]

We currently use JSONFile for the trackers, and we also persist outgoing commands, IDs to fetch, failed IDs, and add-ons reconciler state to disk. These don't register shutdown blockers, and `toFetch` and `failed` operate on a timer.

It's possible to lose data on shutdown, which might be OK for these cases, but let's make sure. If we want more durable storage, let's switch to JSONFile.jsm.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1366067 - Use `JSONFile.jsm` to persist the backlog and previously failed IDs.

Review commit: https://reviewboard.mozilla.org/r/196050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196050/

Comment 2

[Lina Butler [:lina]]

I didn't convert the clients engine or add-ons reconciler to use `JSONFile` here. That's good fodder for a follow-up, or we can leave them be. The clients engine might benefit from a shutdown blocker.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8924818 [details]
Bug 1366067 - Use `JSONFile.jsm` to persist the backlog and previously failed IDs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196050/diff/1-2/

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8924818 [details]
Bug 1366067 - Use `JSONFile.jsm` to persist the backlog and previously failed IDs.

https://reviewboard.mozilla.org/r/196050/#review201564

I'm not completely familiar with the JSONFile API, but this looks good to me!

::: services/sync/modules/engines.js:100
(Diff revision 1)
>    _dataPostProcessor(json) {
>      return typeof json == "object" && json || {};
>    },
>  
>    // Ensure the Weave storage directory exists before writing the file.
>    _beforeSave() {

Do we still need this function?

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8924818 [details]
Bug 1366067 - Use `JSONFile.jsm` to persist the backlog and previously failed IDs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196050/diff/2-3/

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8924818 [details]
Bug 1366067 - Use `JSONFile.jsm` to persist the backlog and previously failed IDs.

https://reviewboard.mozilla.org/r/196050/#review201564

> Do we still need this function?

I think we do; `test_engine` tests the tracker separately, and we create `weave/changes/` separately from `weave/toFetch/` and `weave/failed`.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66a726c06aa7
Use `JSONFile.jsm` to persist the backlog and previously failed IDs. r=tcsc

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/66a726c06aa7