1286785 - Addon Repository never recovers from a corrupt addons.json

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Via bug 1285866:

* If addons.json is missing at startup, we happily create a new .json file without re-populating it - it ends up being partially repopulated as various things happen (eg, explicit requests to add an addon), but the world doesn't seem to end.

* Is addons.json is *corrupt* at startup, we act as though the cache is disabled and never use it.

Thus bug is to suggest that we treat the second case the same as the first - just re-create the DB and act as though it was missing. There doesn't seem to be any benefit in forever starting up, failing to read the same invalid JSON and leaving the feature/optimization/whatever-it-is disabled.

Additional context: in bug 1275139, there's a subtle change to how Sync behaves if the repository is disabled - and it appears as "disabled" due to a corrupt file, whereas Sync really would prefer it actually meant "extensions.getAddons.cache.enabled=False"

Felipe/Unfocused, this was originally done as part of bug 853389 as part of moving away from sqlite. I *guess* it was done this way due to the SQL code doing roughly the same thing, but there could well be something I'm missing. Can anyone think of why this is a bad idea?

Comment 1

[Andy McKay]

Not the same, but related bug 1272446.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I don't recall any good reason this was done this way, so I believe you're right: it was just translated from the previous SQL handling. Sounds like a good improvement to make. I'd say go for it!

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andy McKay [:andym] from comment #1)
> Not the same, but related bug 1272446.

Thanks. I think bug 1272446 is more related to bug 1285866 than this; I added a note to bug 1272446.

I'm making this bug block 1285866 due to a very subtle change in semantics this issue causes - if there is a corrupt DB, the fact that Sync will consider the cache explicitly disabled means there is an edge-case where Sync would install and addon where it wouldn't previously.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1286785 - treat a corrupt addons.json as though it was missing.

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

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8771807 [details]
Bug 1286785 - treat a corrupt addons.json as though it was missing.

https://reviewboard.mozilla.org/r/64806/#review62096

lgtm - I love patches that remove more code than they add, *and* make things better :)

Comment 6

[Robert Helmer [:rhelmer]]

I took a quick look over the testfails in the try run and pretty sure they are all known intermittent.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Thanks Robert.

Reviewboard seems to have decided I don't have permission to autoland :/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d1ef80b6820
treat a corrupt addons.json as though it was missing. r=rhelmer

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d1ef80b6820