Fallback to dump data if `.get()` fails to read from IndexedDB
Categories
(Firefox :: Remote Settings Client, enhancement)
Tracking
()
People
(Reporter: leplatrem, Assigned: leplatrem)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr78+
|
Details |
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.
Assignee | ||
Comment 1•4 years ago
|
||
Updated•4 years ago
|
Comment 3•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Comment 4•4 years ago
|
||
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:
Comment 5•4 years ago
|
||
Comment on attachment 9160605 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r?standard8
Approved for 79.0b6.
Comment 6•4 years ago
|
||
bugherder uplift |
Updated•4 years ago
|
Comment 7•4 years ago
|
||
Reminder for Mark that we need the try-build for this verification.
Comment 8•4 years ago
|
||
I did a try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4313a433fb746c10b02bc0fec04d4c1468bfe492
Linux64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/HrkbEoKWSO-MIRNSzl9JLQ/runs/0/artifacts/public/build/target.tar.bz2
Mac: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/DjwdyX3nTFmlt17ISeOaNQ/runs/0/artifacts/public/build/target.dmg
Windows: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/KAYWojdgRwmcRsU9r3tKTQ/runs/0/artifacts/public/build/target.zip
Comment 9•4 years ago
|
||
Adrian, have you had any luck verifying this?
Comment 10•4 years ago
|
||
(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.
Comment 11•4 years ago
|
||
[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:
- make sure that search is broken with search-modernization enabled on a release78 build using the broken profile
- disable all network connections
- make sure that search is still broken with search-modernization enabled on a release78 build
- 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:]
- 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.
- 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.
Comment 12•4 years ago
|
||
[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.
Updated•4 years ago
|
Comment 13•4 years ago
|
||
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.
Comment 14•4 years ago
|
||
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).
Assignee | ||
Comment 15•4 years ago
|
||
Assignee | ||
Comment 16•4 years ago
|
||
Depends on D79016
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 17•4 years ago
|
||
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:
Comment 18•4 years ago
|
||
Comment on attachment 9165054 [details]
Bug 1649700 - Fallback to dump data if fails to read from IndexedDB r=standard8
Approved for 78.1esr.
Comment 19•4 years ago
|
||
bugherder uplift |
Comment 20•4 years ago
|
||
:standard8, could you pretty please spin a trybuild for esr78.1 with 1649700 and without 1649393?
Comment 21•4 years ago
|
||
Based on ESR 78:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8029f4e33bd48f58e07bf8ce83b2ba1dd505ed
- Linux 64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/PNhyUg5tRTyoViQOxP5ueA/runs/0/artifacts/public/build/target.tar.bz2
- Mac: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/M4mQjyW_QQulZ7UKKV8Ojg/runs/0/artifacts/public/build/target.dmg
- Windows: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/JONQyjJ4SxSRsF_0EzXkQA/runs/0/artifacts/public/build/target.zip
Comment 22•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•4 years ago
|
Description
•