STR:

1. Intoolkit/components/search/tests/xpcshell/xpcshell.ini set these prefs near the top of the file:

prefs =
  dom.indexedDB.dataThreshold=0
  dom.indexedDB.preprocessing=true

2. Run ./mach xpcshell-test toolkit/components/search/tests/xpcshell/test_distributions.js

Expected Results: Test passes
Actual Results: Test fails - remote settings fails to return the local dump.

 0:01.69 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "IndexedDB Preprocessing for indexes not yet implemented!: ActorsParent.cpp:26068"]"
 0:01.69 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:562"]"
 0:01.69 pid:9794 Full command: ['/Users/mark/dev/gecko/objdir-ff/dist/Nightly.app/Contents/MacOS/xpcshell', '-g', '/Users/mark/dev/gecko/objdir-ff/dist/Nightly.app/Contents/Resources', '-a', '/Users/mark/dev/gecko/objdir-ff/dist/Nightly.app/Contents/Resources/browser', '-r', '/Users/mark/dev/gecko/objdir-ff/dist/Nightly.app/Contents/Resources/components/httpd.manifest', '-m', '-e', 'const _HEAD_JS_PATH = "/Users/mark/dev/gecko/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/var/folders/6x/x01fpl8s7m11qpvrhbw0svf80000gp/T/firefox/xpcshellprofile/mozinfo.json";', '-e', 'const _PREFS_FILE = "/Users/mark/dev/gecko/objdir-ff/temp/xpc-other-c511tm2f/user.js";', '-e', 'const _TESTING_MODULES_DIR = "/Users/mark/dev/gecko/objdir-ff/_tests/modules/";', '-f', '/Users/mark/dev/gecko/testing/xpcshell/head.js', '-p', '/Users/mark/dev/gecko/objdir-ff/temp/xpc-plugins-dn9nquxh', '-e', 'const _HEAD_FILES = ["/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/search/tests/xpcshell/head_search.js", "/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/search/tests/xpcshell/head_opensearch.js", "/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/search/tests/xpcshell/head_modernconfig.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', 'const _TEST_FILE = ["/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_distributions.js"];', '-e', 'const _TEST_NAME = "xpcshell.ini:toolkit/components/search/tests/xpcshell/test_distributions.js";', '-e', '_execute_test(); quit(0);']
pid:9794 console.error: SearchEngineSelector: (new UnknownError("IndexedDB: main/search-config list() IndexedDB:  execute() The operation failed for reasons unrelated to the database itself and not covered by any other error code.", "resource://services-settings/IDBHelpers.jsm", 24))
 0:01.69 pid:9794 console.error: SearchEngineSelector: "Received empty search configuration!"
 0:01.76 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "IndexedDB Preprocessing for indexes not yet implemented!: ActorsParent.cpp:26068"]"
 0:01.76 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:562"]"
 0:01.76 pid:9794 console.error: SearchEngineSelector: (new UnknownError("IndexedDB: main/search-config list() IndexedDB:  execute() The operation failed for reasons unrelated to the database itself and not covered by any other error code.", "resource://services-settings/IDBHelpers.jsm", 24))
 0:01.76 pid:9794 console.error: SearchEngineSelector: "Received empty search configuration!"
 0:01.76 ERROR Unexpected exception NS_ERROR_UNEXPECTED: Failed to get engine data from Remote Settings

The failure route is this:

• We attempt to read the (empty in this case) indexedDB database to get the last modified timestamp
• That passes, because the database is empty, we proceed and try to write the dump into the database
• That also succeeds, so we try and load the records from the database.

That last step fails and throws the errors.

From my local testing, I can reproduce this on my development profile if I set the preferences, then start it, trigger a remote settings update, then restart Firefox.

[Tracking Requested - why for this release]: As we're turning on Search modernisation in 79/78.1esr we will be using remote settings. If the remote setting service fails to provide the necessary data for the search service, then various parts of the Firefox UI will be broken.

I picked this up from bug 1652505 and bug 1637715.

Jan/Simon, do you know how common cases like this are likely to be? Specifically, the type of case where we may be able to read from indexedDB to begin with, but then it fails after a write?

Tracking+, but we need an answer ASAP because it's RC week and it sounds like this may require us to disable search modernization again and respin RCs depending on what the severity is.

I have a small patch that adjusts the workaround implemented in bug 1649700 to cover the whole of the RemoteSettingsClient.get() function - basically if any of that fails, then we'll use the fallback.

This should be safe as it will only affect the failure case, and the fallback code has already been tested as part of bug 1649700. The patch is working for the case above, and I'm just writing a test.

This extends the current try/catch to cover writing of the dump to the database and reading it back again. It does not cover the verify signature section, as it is expected that it will need to be known if the signature fails.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a509b6bc77ada0a9a495326a9066acf66f265cb

Comment on attachment 9164844 [details]
Bug 1653988 - Extend the loading of dump fallback for Remote Settings to cover more indexedDB failure areas. r?leplatrem!

Beta/Release Uplift Approval Request

• User impact if declined: This extends the patch added in bug 1649700 to add protection against different possible indexedDB failures. The first patch only caught one case, this patch should catch any case where we fully read/write and would therefore fail.

Unfortunately we don't have any real stats from the SearchService yet for how often this might occur. On nightly over the last 4 days prior (the only the stats we have), we have seen 2 instances out of 300k of this. That could have been broken profiles, or it could have been people testing. Of course, we don't know what beta/release profiles will look like yet.

The IndexedDB team may have some better stats or ideas of the likely possibility of failures here (pending NI).

• 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: See comment 0 and comment 1.
• List of other uplifts needed: Bug 1649700
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is low risk as the patch for bug 1649700 has already been tested, and this extends the try/catch case to cover more possible failures that were weren't originally expecting, the fallback code is unchanged.

Worst case, we'd fallback to returning the locally stored dump, or an empty list if there isn't a dump, rather than throwing an error.

• String changes made/needed: None

I'll have an ESR patch up in a bit.

Comment on attachment 9164844 [details]
Bug 1653988 - Extend the loading of dump fallback for Remote Settings to cover more indexedDB failure areas. r?leplatrem!

79 is on release, moving the request accordingly.

Mark, we've already built 79.0 RC1. Is this something that you think should drive a respin or just ride-along if we need one?

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

Mark, we've already built 79.0 RC1. Is this something that you think should drive a respin or just ride-along if we need one?

At the moment, I have no data to say how much of an issue this might be. Once RC is out for a day or two on the beta channel, we might be able to get some data.

I'm a little torn, as if users do hit this, it is quite a significant issue, but I think at this time, I'm ok for this to be a ride-along as long as we can also check the data in a day or two & continue to monitor it.

(In reply to Mark Banner (:standard8) from comment #2)

[Tracking Requested - why for this release]: As we're turning on Search modernisation in 79/78.1esr we will be using remote settings. If the remote setting service fails to provide the necessary data for the search service, then various parts of the Firefox UI will be broken.

I picked this up from bug 1652505 and bug 1637715.

Jan/Simon, do you know how common cases like this are likely to be?

As far as this happens only when setting dom.indexedDB.preprocessing=true, this is highly unlikely to happen, since this is an experimental pref, and since the feature is not fully implemented, it would break the user experience anyway in most contexts.

AFAIU, this does not happen when only setting dom.indexedDB.dataThreshold=0, right? At least there is test coverage by several tests with this setting of the pref. We do not run all tests with this pref set to 0 at the moment, though.

Specifically, the type of case where we may be able to read from indexedDB to begin with, but then it fails after a write?

This is obviously hard to say in general, but I would deem it rather unlikely.

https://hg.mozilla.org/mozilla-central/rev/504fea031a58

(In reply to Simon Giesecke [:sg] [he/him] from comment #12)

(In reply to Mark Banner (:standard8) from comment #2)

Jan/Simon, do you know how common cases like this are likely to be?

As far as this happens only when setting dom.indexedDB.preprocessing=true, this is highly unlikely to happen, since this is an experimental pref, and since the feature is not fully implemented, it would break the user experience anyway in most contexts.

AFAIU, this does not happen when only setting dom.indexedDB.dataThreshold=0, right? At least there is test coverage by several tests with this setting of the pref. We do not run all tests with this pref set to 0 at the moment, though.

Specifically, the type of case where we may be able to read from indexedDB to begin with, but then it fails after a write?

This is obviously hard to say in general, but I would deem it rather unlikely.

Thank you for the clarifications Simon. That would indeed seem like this is less likely to be a possible occurrence.

As I discussed with Mathieu earlier today - we want to ensure that get() doesn't fail at all, unless expected, so I think having the patch in place is still a good idea.

Ryan, are you happy to proceed with us simply checking the stats later this week and possibly not landing until next release? This is certainly now feeling more like just an extra safety-net.

I would be fine with letting this ship next cycle.

Reproduced the issue with a Nightly build(80.0a1 2020-07-12) after the initially landed db-index fail fixes from bug 1649393 and bug 1649700. Confirmed updating to the latest Nightly (80.0a1 2020-07-24) that the problem is fixed. Also confirmed that ESR 78.1 still fails to provide fallback to dump with this issue's broken profile.

The above tests were performed using Ubuntu 20 and Windows 10.

Removing the qe+ until a decision regarding the esr uplift is taken. Please re-add the qe+ flag in case of uplifting to esr.

Comment on attachment 9165119 [details] [diff] [review]
ESR Patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Attempts to improve the situation around potential cases where IndexedDB failures cause the search service to fail to start up properly which blocks the use of the search functionality and full use of the address bar.
• User impact if declined: See above.
• Fix Landed on Version: 80.0a1
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Has been on nightly & beta with no issues raised.
• String or UUID changes made by this patch: None

Comment on attachment 9165119 [details] [diff] [review]
ESR Patch

Approved for 78.2esr.

https://hg.mozilla.org/releases/mozilla-esr78/rev/78ab738e6da9

Reproduced on 78.1.0esr 20200722151235, then proceeded to verify the fix on 78.2.0esr 20200807194714.

• Note:
  This bug has been verified using the search-config Remote Settings implementation point of view, with the note that search-config has a dump fallback mecanism, which in this simulated case of failure is used. A truly complete verification of this bug would actually mean verifying all the implementations of Remote Settings list here which would be quite the effort since I lack the context and the knowledge of how to actually verify all the listed features functionality in case of db-index or/and Remote Settings sync failure and after asking advice from :leplatrem, we arrived at the conclusion that following-up with bug 1658574 and bug 1658597 to figure out the next steps would be the best approach.

Comment on attachment 9164844 [details]
Bug 1653988 - Extend the loading of dump fallback for Remote Settings to cover more indexedDB failure areas. r?leplatrem!

80 ships next week, there are no plans for a 79 dot release