Closed Bug 1649700 Opened 1 month ago Closed 1 month ago

Fallback to dump data if `.get()` fails to read from IndexedDB

Categories

(Firefox :: Remote Settings Client, enhancement)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 80
Tracking Status
firefox-esr78 79+ verified
firefox79 --- verified
firefox80 --- verified

People

(Reporter: leplatrem, Assigned: leplatrem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

In Bug 1649393 we noticed that profiles will a corrupted Chrome IndexedDB would fail at reading records. In order to avoid returning an empty list in that case, we could load the packaged JSON dump in memory and return that as a fallback.

Pushed by mleplatre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efc807aec606
Fallback to dump data if fails to read from IndexedDB r=Standard8
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Flags: qe-verify+

Comment on attachment 9160605 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r?standard8

Beta/Release Uplift Approval Request

  • User impact if declined: If indexedDB fails to work for some reason, remote settings will not be able to return any data. This can impact the modern configuration of the search service, it can also prevent allow/block lists from being filled.

This patch makes it so that remote settings will at least provide the shipped dump where there is one - so that worst case we'll fallback to the version Firefox was shipped with.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: This is related to bug 1649393. Unfortunately the only way to reproduce this manually is with a special build - I will back out bug 1649393 and supply that for QA to use in their verifications. I have already been talking to Adrian about this.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Provides a fallback case that is only triggered when IndexedDB is already broken
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: We have agreed to turn search modernisation back on in 78.1esr, so this would be necessary for that.
  • User impact if declined:
  • Fix Landed on Version: 80.0
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
Attachment #9160605 - Flags: approval-mozilla-esr78?
Attachment #9160605 - Flags: approval-mozilla-beta?

Comment on attachment 9160605 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r?standard8

Approved for 79.0b6.

Attachment #9160605 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Reminder for Mark that we need the try-build for this verification.

Flags: needinfo?(standard8)

Adrian, have you had any luck verifying this?

Flags: needinfo?(adrian.florinescu)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Adrian, have you had any luck verifying this?

Probably, should've listed this update earlier to reduce the confusion.
I did have a bit off trouble verifying directly this bug (more on the time required side), but since it is meant to be a fallback in case bug 1649393 fix is not working, I am/was more reluctant about potential search-modernization/remote-settings regressions that would be pretty tough to catch in normal conditions, although when discussing it with Mark, it was catalogued as rather unlikely. Adding to that, by good timing, we also have the Post-Modernization clean-up testing run for beta79(eta: today 7/17), which includes an extensive Search sanity and smoke testing for 79, hence my thoughts were to wait marking this as verified until we finish running the suite in order to be certain nothing surprizing comes to get reported.

[Environment:]

Windows 10, Ubuntu 20, Mac 10.13.6
Firefox versions used: try-builds from comment 8, since the fix cannot be punctually verified with bug 1649393 landed as well. (fix = fallback in case bug 1649393 fails)

Using the try-builds that :standard8 was kind enough to supply in comment 8, we've verified the fix by using a broken index-db profile as follows:

  1. make sure that search is broken with search-modernization enabled on a release78 build using the broken profile
  2. disable all network connections
  3. make sure that search is still broken with search-modernization enabled on a release78 build
  4. start-up the broken profile with the try-build while network is disabled and confirm that search is initialized accordingly and the dump is used.

Additionally to the above simple scenario, the [79]Post-Modernization clean-up test run has just been completed with no outstanding issues, which ensures that at least on the search-modernization side, there are no obvious mishaps.

[Note:]
  1. We've also tried to check if we can find additional ways to break either Remote Settings/search-config, but no luck, at least with the current amout of time invested into this.
  2. My only concern remaining related to this fix is that in my view, there might be still exceptional use-cases with broken profiles (downgrades and other pre-dedicated profiles corruptions) which would affects Remote-Settings/Search-Modernization and maybe other Remote-Settings implementations and which would not be covered by default QA suites, since downgrading profiles is technically unsupported and out-of-scope for QA.
Flags: needinfo?(adrian.florinescu)

[Tracking Requested - why for this release]: To my knowledge, there is intent to have Search Modernization enabled on 78ESR at some point, therefore setting up the tracking flag in order to decide if this fix should be uplifted to ESR as well.

Note: Not updating flags for 80 atm, since QA is going to run PostModernization suite for Fx80 as well, so leaving the NI for myself standing.

Flags: needinfo?(adrian.florinescu)

Comment on attachment 9160605 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r?standard8

Sounds like the results with this patch have been good so far. Approved for 78.1esr.

Attachment #9160605 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Comment on attachment 9160605 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r?standard8

Actually, this has conflicts with ESR78. Please attach a rebased patch ASAP (this blocks the RC build).

Flags: needinfo?(mathieu)
Attachment #9160605 - Flags: approval-mozilla-esr78+
Attachment #9164986 - Attachment is obsolete: true
Flags: needinfo?(mathieu)

Comment on attachment 9165054 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r=standard8

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Beta/release uplift was requested and approved. This patch is a rebase for ESR78.

It relies on 2 other minor patches regarding loading of packaged data and previewing Remote Settings changes:

o Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r=standard8
|
o Bug 1644153 - Fix loading of JSON dump from main-preview r=standard8
|
o Bug 1640136 - Load main dump when using preview r=standard8
|

  • User impact if declined: See beta/release uplift request
  • Fix Landed on Version: 79
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
Attachment #9165054 - Flags: approval-mozilla-esr78?

Comment on attachment 9165054 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r=standard8

Approved for 78.1esr.

Attachment #9165054 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

:standard8, could you pretty please spin a trybuild for esr78.1 with 1649700 and without 1649393?

Flags: needinfo?(standard8)

With comment 21 try-builds, confirmed that using the comment 11 scenarios are working as intended. Additionally, the same scenarios were ran using Nightly 80 (24-07-2020).
The above tests were performed using Ubuntu 20 and Windows 10.

Status: RESOLVED → VERIFIED
Flags: needinfo?(adrian.florinescu)
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.