Closed Bug 1286785 Opened 3 years ago Closed 3 years ago

Addon Repository never recovers from a corrupt addons.json

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
Flags: needinfo?(felipc)
Flags: needinfo?(bmcbride)
Not the same, but related bug 1272446.
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!
Flags: needinfo?(felipc)
(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.
Blocks: 1285866
Flags: needinfo?(bmcbride)
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 :)
Attachment #8771807 - Flags: review?(rhelmer) → review+
I took a quick look over the testfails in the try run and pretty sure they are all known intermittent.
Thanks Robert.

Reviewboard seems to have decided I don't have permission to autoland :/
Assignee: nobody → markh
Keywords: checkin-needed
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/2d1ef80b6820
treat a corrupt addons.json as though it was missing. r=rhelmer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d1ef80b6820
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.