1701414 - Occasional loss of feeds.json data

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Wladimir Palant]

So far I’ve lost feeds.json data three times after bug 1534163 landed. I see a “folder” feeds.json.corrupted appear in Thunderbird, it’s a zero-sized file. All RSS subscriptions are gone and have to be restored manually. This never happened with feeds.rdf.

The way I see this, the issue is JSONFile using OS.File.writeAtomic() to save the data. The flush option isn’t being passed here, but it is required to prevent data loss should Thunderbird crash.

Comment 1

[alta88]

Incredibly, this just happened today to a test account. One of the ancient feed renovations added an rdf flush() everywhere but this wasn't carried over to JSONFile. It seems that

 let ds = new JSONFile({ path: feedsFile.path });
+ds._options.flush = true;

and

 let ds = new JSONFile({ path: itemsFile.path });
+ds._options.flush = true;

in FeedUtils.jsm should do it. OS.File is being replaced slowly but they haven't updated JSONFile yet. It seems IOUtils.write() takes the same options as OS.File.writeAtomic() though.

Comment 2

[Magnus Melin [:mkmelin] (away until Aug 10)]

tmpPath is passed though, which is supposed to make it very safe. With tmpPath you will NOT get an incomplete file for process crash, but can get one for an OS crash. backupTo could give some extra safety.
https://searchfox.org/mozilla-central/rev/fa48ebee58d59fa846919c3e2c3122b08db57c9c/toolkit/modules/JSONFile.jsm#464

Comment 3

[Ben Campbell]

Attachment: Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

Also adds the .tmp and .corrupt name variants to the hacky whitelist of
files-to-not-treat-as-mboxes.

Comment 4

[Ben Campbell]

I think backupTo is the way to go. It seems to have improved a lot since the last time I looked at it (from memory I seem to recall it didn't play nice with tmpPath).

I think flush is too heavyweight - it extends way beyond the process to stall the whole OS filesystem (and can still have issues with abrupt power-off).

JSONFile already uses tmpFile behind the scenes, so I added the .tmp (and the .corrupt) variant to the icky not-an-mbox lists (which should fix the bogus-folder issue in Wladimir's original comment).

As an aside: looks like JSONFile was already converted from OS.File to IOUtils a couple of times, but reverted each time. I can't imagine it'll be long before the conversion sticks. Shouldn't affect anything here though.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7aa4bcaad0b5
Use backup file for RSS feeds.json/feeditems.json. r=mkmelin

Comment 6

[Rob Lemley [:rjl]]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1534163
User impact if declined: possible data loss on crash
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
Low risk patch as this is creating backup files of feeds.json.

Comment 7

[Wayne Mery (:wsmwk)]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

[Triage Comment]
Approved for beta

Comment 8

[Rob Lemley [:rjl]]

Thunderbird 89.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/675eff7e1463

Comment 9

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

[Approval Request Comment]
User impact if declined: may lose feed data
Testing completed (on c-c, etc.): c-c and beta
Risk to taking this patch (and alternatives if risky): low

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 11

[Rob Lemley [:rjl]]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

This patch does not apply on comm-esr78. It looks like the SPECIAL_FILES code is quite different so it's probably better to leave this wontfix for 78.

Specifically, the SPECIAL_FILES constant is not present in converterWorker.js. It looks like that was originally created in bug 1694942 to fix a mailstore conversion bug.

Comment 12

[Magnus Melin [:mkmelin] (away until Aug 10)]

Ah, let's wontfix for 78 then.

Comment 13

[alta88]

Please apply this to 78ESR. All you have to do is skip the hunk in converterWorker.js.

Comment 14

[Rob Lemley [:rjl]]

Comment on attachment 9217618 [details]
Bug 1701414 - Use backup file for RSS feeds.json/feeditems.json. r?mkmelin

[Triage Comment]
Previously accepted for esr78, will make another attempt based on comment 13.

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.12.0:
https://hg.mozilla.org/releases/comm-esr78/rev/4e50244620d5