Looking at the barely existing usage data in https://georgf.github.io/usecounters/index.html#kind=page&group=DEPRECATED&channel=beta&version=60 I think we can be bold and try to unship this starting in Nightly 61 (instead of originally suggested 62). I would like to prime this by preffing the storage attribute off in Nightly ASAP so that we're able to get reports of any (unexpected) data loss happening in the wild on Nightly and Beta and are able to flip it back on if necessary so that Release 61 users don't lose data. We can remove the redundant code and the pref in 62.

I maintain an RSS reader WebExtension called Brief that currently stores the RSS entries in a "persistent" database and I really thought I have some time to implement the migration to a "default" one. Deadlines jumping closer are no fun :-(

Hey Denis, thanks for the heads up. I think it's reasonable to keep this around a little longer for WebExtensions (assuming we don't prompt for them). However, I do think that moving ahead on Nightly and moving ahead for web content are both good.

(In reply to Denis Lisov from comment #1) > I maintain an RSS reader WebExtension called Brief that currently stores the > RSS entries in a "persistent" database and I really thought I have some time > to implement the migration to a "default" one. Deadlines jumping closer are > no fun :-( Thank you for the comment! I did not consider that WebExtensions get the (deprecated) indexedDB permission set to allow when they have the "unlimitedStorage" (WebExtension-)permission. I'll have to file a bug about that... https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/toolkit/components/extensions/Extension.jsm#1559,1565 Generally our usage numbers also cover WebExtensions usage, so I'm sorry that we're forcing you to do a migration, but there was bound to be some usage somewhere. That shouldn't stop us from unshipping. I'm happy to defer that to 62, though, and not consider WebExtensions when checking the pref...

(In reply to Johann Hofmann [:johannh] from comment #3) > (In reply to Denis Lisov from comment #1) > > I maintain an RSS reader WebExtension called Brief that currently stores the > > RSS entries in a "persistent" database and I really thought I have some time > > to implement the migration to a "default" one. Deadlines jumping closer are > > no fun :-( > > Thank you for the comment! I did not consider that WebExtensions get the > (deprecated) indexedDB permission set to allow when they have the > "unlimitedStorage" (WebExtension-)permission. I'll have to file a bug about > that... Filed bug 1451794

Ah, dammit, getting that pref in a worker won't work, obviously. Is there any pre-baked solution for getting prefs off-mainthread?

(In reply to Johann Hofmann [:johannh] from comment #11) > Ah, dammit, getting that pref in a worker won't work, obviously. Is there > any pre-baked solution for getting prefs off-mainthread? Yes. DOMPrefs: https://searchfox.org/mozilla-central/source/dom/base/DOMPrefs.h https://searchfox.org/mozilla-central/source/dom/base/DOMPrefsInternal.h

Ah, great, thanks, I'll cancel review for now!

Comment on attachment 8966743 [details] Bug 1451486 - Part 2 - Add a test for setting the dom.indexedDB.storageOption.enabled pref. https://reviewboard.mozilla.org/r/235438/#review242080 Thanks for the great test! ::: dom/indexedDB/test/unit/test_storageOption_pref.js:74 (Diff revision 3) > + event = yield undefined; > + > + is(event.target.result, data.key, "Got correct key"); > + > + // Turn the storage option on, content databases should be able to get > + // "persistent" now. nit: Maybe add an additional explanatory comment here of: Because persistent storage is separate from default storage, we will not find the database we just created and will receive an upgradeneeded event.

Comment on attachment 8966742 [details] Bug 1451486 - Part 1 - Ignore the storage attribute on indexedDB.open() by default. https://reviewboard.mozilla.org/r/235436/#review242062 I'm going to r? :baku to sign off on the DOMPrefs changes. I may get it wrong or it may take a few tries; please make sure he signs off somewhere before landing. ::: dom/indexedDB/IDBFactory.cpp:699 (Diff revision 3) > + if (NS_IsMainThread()) { > + nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(principalInfo); > + if (principal) { You can just check aPrincipal here. In this on-mainthread case, we would have derived the principalInfo from aPrincipal above, so the roundtrip isn't necessary.

Comment on attachment 8966742 [details] Bug 1451486 - Part 1 - Ignore the storage attribute on indexedDB.open() by default. https://reviewboard.mozilla.org/r/235436/#review242136

(In reply to Andrew Sutherland [:asuth] from comment #18) > Comment on attachment 8966742 [details] > Bug 1451486 - Part 1 - Ignore the storage attribute on indexedDB.open() by > default. > > https://reviewboard.mozilla.org/r/235436/#review242062 > > I'm going to r? :baku to sign off on the DOMPrefs changes. I may get it > wrong or it may take a few tries; please make sure he signs off somewhere > before landing. > > ::: dom/indexedDB/IDBFactory.cpp:699 > (Diff revision 3) > > + if (NS_IsMainThread()) { > > + nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(principalInfo); > > + if (principal) { > > You can just check aPrincipal here. In this on-mainthread case, we would > have derived the principalInfo from aPrincipal above, so the roundtrip isn't > necessary. Are you sure? There's a case (on and off mainthread) where we do not have aPrincipal but only the principalInfo from mPrincipalInfo and in my testing it seems like that's hit by WebExtensions... Thanks for the reviews :)

(In reply to Johann Hofmann [:johannh] from comment #20) > Are you sure? There's a case (on and off mainthread) where we do not have > aPrincipal but only the principalInfo from mPrincipalInfo and in my testing > it seems like that's hit by WebExtensions... Yes, you're absolutely right. I was just going from memory, but was remembering things about IDBFactory::Create*, not IDBFactory::Open*, and I had it 100% backwards. How about adding a comment here like "aPrincipal is passed inconsistently, so even when we are already on the main thread, we may have been passed a null aPrincipal."

(In reply to Andrew Sutherland [:asuth] from comment #24) > (In reply to Johann Hofmann [:johannh] from comment #20) > > Are you sure? There's a case (on and off mainthread) where we do not have > > aPrincipal but only the principalInfo from mPrincipalInfo and in my testing > > it seems like that's hit by WebExtensions... > > Yes, you're absolutely right. I was just going from memory, but was > remembering things about IDBFactory::Create*, not IDBFactory::Open*, and I > had it 100% backwards. How about adding a comment here like "aPrincipal is > passed inconsistently, so even when we are already on the main thread, we > may have been passed a null aPrincipal." Good idea, thanks!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2e635314b75f Part 1 - Ignore the storage attribute on indexedDB.open() by default. r=asuth,baku https://hg.mozilla.org/integration/autoland/rev/eea1bfe5404d Part 2 - Add a test for setting the dom.indexedDB.storageOption.enabled pref. r=asuth

Backed out for xpcshell failures on test_storageOption_pref.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eea1bfe5404db3705ef7d32c7a6f19a533531be7&filter-searchStr=xpcshell&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=173712398 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173712398&repo=autoland&lineNumber=1550 [task 2018-04-14T20:42:01.251Z] 20:42:01 INFO - TEST-START | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js [task 2018-04-14T20:42:03.865Z] 20:42:03 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | xpcshell return code: 0 [task 2018-04-14T20:42:03.866Z] 20:42:03 INFO - TEST-INFO took 2614ms [task 2018-04-14T20:42:03.867Z] 20:42:03 INFO - >>>>>>> [task 2018-04-14T20:42:03.867Z] 20:42:03 INFO - xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | xpcw: cd /sdcard/tests/xpc/dom/indexedDB/test/unit [task 2018-04-14T20:42:03.868Z] 20:42:03 INFO - xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/sdcard/tests/xpc/dom/indexedDB/test/unit/xpcshell-head-parent-process.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_storageOption_pref.js"]; -e const _TEST_NAME = "xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js" -e _execute_test(); quit(0); [task 2018-04-14T20:42:03.868Z] 20:42:03 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2018-04-14T20:42:03.868Z] 20:42:03 INFO - (xpcshell/head.js) | test pending (2) [task 2018-04-14T20:42:03.868Z] 20:42:03 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) [task 2018-04-14T20:42:03.868Z] 20:42:03 INFO - running event loop [task 2018-04-14T20:42:03.869Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 43 - "upgradeneeded" == "upgradeneeded" [task 2018-04-14T20:42:03.869Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 52 - "success" == "success" [task 2018-04-14T20:42:03.870Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 54 - "Splendid Test" == "Splendid Test" [task 2018-04-14T20:42:03.870Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 55 - 1 == 1 [task 2018-04-14T20:42:03.871Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 56 - "default" == "default" [task 2018-04-14T20:42:03.871Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 65 - "undefined" == "undefined" [task 2018-04-14T20:42:03.871Z] 20:42:03 INFO - TEST-PASS | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | is - [is : 20] JS frame :: test_storageOption_pref.js :: testSteps :: line 71 - 1 == 1 [task 2018-04-14T20:42:03.872Z] 20:42:03 INFO - xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | indexedDB error: InvalidStateError [task 2018-04-14T20:42:03.872Z] 20:42:03 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | errorHandler - [errorHandler : 93] false == true [task 2018-04-14T20:42:03.873Z] 20:42:03 INFO - /sdcard/tests/xpc/dom/indexedDB/test/unit/xpcshell-head-parent-process.js:errorHandler:93 [task 2018-04-14T20:42:03.873Z] 20:42:03 INFO - /sdcard/tests/xpc/head.js:_do_main:211 [task 2018-04-14T20:42:03.874Z] 20:42:03 INFO - /sdcard/tests/xpc/head.js:_execute_test:517 [task 2018-04-14T20:42:03.874Z] 20:42:03 INFO - -e:null:1 [task 2018-04-14T20:42:03.874Z] 20:42:03 INFO - exiting test [task 2018-04-14T20:42:03.875Z] 20:42:03 INFO - xpcshell.ini:dom/indexedDB/test/unit/test_storageOption_pref.js | JavaScript error: /sdcard/tests/xpc/head.js, line 723: NS_ERROR_ABORT: [task 2018-04-14T20:42:03.876Z] 20:42:03 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ABORT: " {file: "/sdcard/tests/xpc/head.js" line: 723}]" [task 2018-04-14T20:42:03.876Z] 20:42:03 INFO - <<<<<<< Backout link: https://hg.mozilla.org/integration/autoland/rev/5067d8ae88c30bd7ba80f47e81f7f4d5240a3bbf

So, the try runs above are showing that this problem seems quite specific to getting the persistent indexedDB permission on Android. I can't seem to run the tests locally, but interestingly I get the same results when just trying to open a persistent DB on Fennec Stable and Nightly. No permission prompt is shown and there's an InternalStateError thrown (I'm not quite sure how to debug native code on Android from that point). I would have suggested opening a bug for fixing persistent indexedDB on Android in general if this wasn't the bug to pref it off... At this point I'd suggest disabling the test on Android. Andrew, what do you think?

I was using https://joo.crater.uberspace.de/to-do-notifications/ to test.

We investigated this on IRC and found we explicitly do: #ifdef IDB_MOBILE return NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR; #else at https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/dom/indexedDB/ActorsParent.cpp#21083 This means the test can't possibly pass on Android. This all appears to have been intentional per https://bugzilla.mozilla.org/show_bug.cgi?id=1119462#c11 when an earlier generation of the logic was introduced. I think needinfo-ing :janv is the right course if anyone wants to dig into such things further. Given that we're trying to clean this all up and have a better persistence mechanism now (https://developer.mozilla.org/en-US/docs/Web/API/StorageManager), it seems most appropriate to disable the test and just continue to clean this all out. r=asuth to disable the test and thanks for this cleanup (and future cleanups :)!

Thank you for finding that, looking forward to more cleanups!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f7e0fca79166 Part 1 - Ignore the storage attribute on indexedDB.open() by default. r=asuth,baku https://hg.mozilla.org/integration/autoland/rev/fe6ac84d5ee4 Part 2 - Add a test for setting the dom.indexedDB.storageOption.enabled pref. r=asuth

https://hg.mozilla.org/mozilla-central/rev/f7e0fca79166 https://hg.mozilla.org/mozilla-central/rev/fe6ac84d5ee4

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/storage-option-for-indexeddb-open-has-been-removed/