1640291 - Choose the most recent attachment in the downloader as a fallback

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Rob Wu [:robwu]]

When the attachment downloader of the RemoteSettings client is unable to download the latest version of the attachment, it will optionally fall back to a previous version, either from the cache (last download from the network), or from the packaged dump.

The current implementation prioritizes the cached attachment over the packaged dump. We should compare the records' last_updated fields and use the most recent of the two instead, so that clients who at some point had downloaded a copy of the attachment are able to use the latest packaged dump if somehow the remote settings server is unreachable.

And while we are at it, let do an optimization: let the client read the packaged dump before the cached dump, because those packaged attachments are expected to be up to date (otherwise there wouldn't really be a point in packaging them with Firefox).

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1640291 - Add test coverage for corrupted cache

In practice, the cache of the attachment downloader can become corrupt
and unusable when IndexedDB breaks. The implementation correctly handled
this case, but there were no tests that verified that it did.

This patch adds test coverage for the scenario of a broken cache,
to ensure that the implementation continues to behave in a sane way.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1640291 - Read RemoteSettings dump before cache, and minimize unnecessary reads

This commit does the following:

• Feature / Optimization: Check the dump before the cache, instead of
  the reverse. The dump is expected to match the requested attachment in
  the common case, and checking it first helps with ensuring that the
  expected (packaged dump) is used when available.

• Optimization: Defer reading the cached attachment until it's needed.

• Refactor / Feature: Treat a missing .meta.json file as a sign that
  the attachment dump does not exist, rather than an error.
  Previously, if an attachment cannot be downloaded from the network,
  that error would be replaced with a generic DownloadError (from the
  missing .meta.json file). This is mostly relevant for telemetry.

• Refactor / Maintainability: Create helper to manage lazy access to the
  record and attachment, to ensure that the record and attachment is
  only read on demand, and at most once.

• Refactor / Readability: Move the common return value generation logic
  to the helper as getResult, to avoid the verbose duplication of the
  logic. Now the return value fits in one line instead of 5-6 lines.

• Fix test: Rename filename-of-dump.meta.json and fix test expectation
  to ensure that the test checks the absence of the file content,
  rather than the absence of the meta data file.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1640291 - Account for last_modified in attachment downloader

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0833992af392
Add test coverage for corrupted cache r=leplatrem
https://hg.mozilla.org/integration/autoland/rev/32e4360d472a
Read RemoteSettings dump before cache, and minimize unnecessary reads r=leplatrem
https://hg.mozilla.org/integration/autoland/rev/96c36737f728
Account for last_modified in attachment downloader r=leplatrem

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/0833992af392
https://hg.mozilla.org/mozilla-central/rev/32e4360d472a
https://hg.mozilla.org/mozilla-central/rev/96c36737f728