Remove the proprietary persistent indexedDB permission
Categories
(Firefox :: Site Permissions, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox104 | --- | fixed |
People
(Reporter: Fischer, Assigned: saschanaz)
References
(Depends on 1 open bug, Blocks 5 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [storage-v2])
User Story
Attachments
(5 files, 1 obsolete file)
Because - we are having the standard persistent storage[1] which also handling persistent indexedDB data and - like the comment in Bug 1309123, it is confusing that having "Store Data in Persistent Storage" and "Maintain Offline Storage" permission at the same time, we should remove the proprietary persistent indexedDB permission. [1] https://storage.spec.whatwg.org [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1309123#c73
Comment 1•7 years ago
|
||
I think the main thing to figure out here is how often this feature is used. Jan knew of at least one site, but it would be good to have actual telemetry. Indicating in the developer console it's deprecated would also be good. Having both of those puts us in a much better place to remove this.
Updated•7 years ago
|
Comment 2•6 years ago
|
||
Jan, is there still anything blocking this from your perspective? It'd be good to get this done.
Comment 3•6 years ago
|
||
Shawn, I remember you added a telemetry for this, what did you find out ?
Sorry for the late reply. It looks like we still can see temporary/persistent type api usage from telemetry data. And persistent type usage is higher than temporary type, for release 57, daily count is still increasing. Temporary type count: Nightly 57-Nightly 59 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-10-16&keys=!__none__!__none__!__none__&max_channel_version=nightly%252F59&measure=SCALARS_IDB.TYPE.TEMPORARY_COUNT&min_channel_version=nightly%252F57&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-09-25&trim=1&use_submission_date=0 beta 57 - beta 57 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-10-16&keys=!__none__!__none__!__none__&max_channel_version=beta%252F57&measure=SCALARS_IDB.TYPE.TEMPORARY_COUNT&min_channel_version=beta%252F57&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-09-25&trim=1&use_submission_date=1 Persistent type count: nightly 57 - nightly 59 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-10-16&keys=!__none__!__none__!__none__&max_channel_version=nightly%252F59&measure=SCALARS_IDB.TYPE.PERSISTENT_COUNT&min_channel_version=nightly%252F57&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-09-25&trim=1&use_submission_date=0 beta 57 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-10-16&keys=!__none__!__none__!__none__&max_channel_version=beta%252F57&measure=SCALARS_IDB.TYPE.PERSISTENT_COUNT&min_channel_version=beta%252F57&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-09-25&trim=1&use_submission_date=1 release 57 https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-10-16&keys=!__none__!__none__!__none__&max_channel_version=beta%252F57&measure=SCALARS_IDB.TYPE.PERSISTENT_COUNT&min_channel_version=beta%252F57&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-09-25&trim=1&use_submission_date=1
Comment 5•6 years ago
|
||
I'm not really sure what to make of the data. Is that per user session? So both are used once per session or some such? Can we add a warning? indexedDB.open("x", {storage: 'persistent', version: 1}) not showing anything in the console seems wrong at this point. The warning should say something like "The indexedDB.open() storage dictionary member is deprecated. Please use https://storage.spec.whatwg.org/ instead." That has helped drive down usage in the past. Another thing we could do is blog about this change to alert web developers about the new API.
Comment 6•6 years ago
|
||
Yes, we should add a warning at this point. A blog post would be even better to help developers with the transition.
Comment 7•6 years ago
|
||
I don't really understand how this had a massive spike in end of October and I'd also like to know whether this is user session or each individual use. If it's the latter, maybe usage is already low enough for removal?
Each individual use. Whenever IDBFactory.open() is called with options (persistent or temporary storage type). https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/indexedDB/IDBFactory.cpp#458
Comment 9•6 years ago
|
||
So it's used once per day on average across all users? I think that means we can just remove it outright. A blog post to announce navigator.storage.persist() might still be nice if anyone wants to take that up.
Comment 10•6 years ago
|
||
(In reply to Anne (:annevk) from comment #9) > So it's used once per day on average across all users? I think that means we > can just remove it outright. A blog post to announce > navigator.storage.persist() might still be nice if anyone wants to take that > up. No, that's for storage: 'temporary'. storage: 'persistent' on beta seems to be called a million times per day at peak. It's really hard to put that number into context, though. (The spike is probably due to incremental release). What are some precedent threshold levels where we removed things in the past? Would it be viable to at least remove the indexedDB permission and check persistent-storage when storage: 'persistent' is requested? That and a warning would probably help with the transition in case we don't want to remove it right now.
Comment 11•6 years ago
|
||
Generally we only remove when usage is close to non-existent. Shawn, what do you think about adding a warning and migrating this API to use persistent storage instead (and prompt for that)? That should at least allow us to do some cleanup in a future release while we try to drive the numbers down.
Comment 12•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #10) > Would it be viable to at least remove the indexedDB permission and check > persistent-storage when storage: 'persistent' is requested? That and a > warning would probably help with the transition in case we don't want to > remove it right now. What do you mean by "remove the indexedDB permission and check persistent-storage when storage: 'persistent' is requested" ? The non-standard explicit persistent storage (requested by storage: 'persistent') is currently protected by a permission prompt. The prompt uses the indexedDB permission. I think we should just deprecate it and check the telemetry numbers regularly.
Comment 13•6 years ago
|
||
(In reply to Jan Varga [:janv] from comment #12) > (In reply to Johann Hofmann [:johannh] from comment #10) > > Would it be viable to at least remove the indexedDB permission and check > > persistent-storage when storage: 'persistent' is requested? That and a > > warning would probably help with the transition in case we don't want to > > remove it right now. > > What do you mean by "remove the indexedDB permission and check > persistent-storage when storage: 'persistent' is requested" ? > > The non-standard explicit persistent storage (requested by storage: > 'persistent') is currently protected by a permission prompt. > The prompt uses the indexedDB permission. Yes and I'd like to have it use the "persistent-storage" permission instead, because it's a UX issue to have two separate permissions.
Comment 14•6 years ago
|
||
Another thing we need to do, probably as a separate bug, is to remove internal usage of this feature. overholt pointed out: https://searchfox.org/mozilla-central/source/browser/extensions/shield-recipe-client/lib/AddonStudies.jsm#51 https://searchfox.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js#24 They probably don't block UI refactoring as they don't trigger UI, but block other cleanup.
(In reply to Anne (:annevk) from comment #11) > Generally we only remove when usage is close to non-existent. Shawn, what do > you think about adding a warning and migrating this API to use persistent > storage instead (and prompt for that)? That should at least allow us to do > some cleanup in a future release while we try to drive the numbers down. We can add warning message like we did for Synchronous XMLHttpRequest to deprecate idb persistent storage api. Fischer, do you have any concern if we replace idb persistent permission with persistent stroage permission?
Reporter | ||
Comment 16•6 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #15) > (In reply to Anne (:annevk) from comment #11) > > Generally we only remove when usage is close to non-existent. Shawn, what do > > you think about adding a warning and migrating this API to use persistent > > storage instead (and prompt for that)? That should at least allow us to do > > some cleanup in a future release while we try to drive the numbers down. > We can add warning message like we did for Synchronous XMLHttpRequest to > deprecate idb persistent storage api. > Fischer, do you have any concern if we replace idb persistent permission with persistent stroage permission? Hi Shawn, I think no. Here is what I imagined: 1. Website calls the legacy idb persistent permission api 2. In the storage manager it sends out the warning in the console to deprecate that 3. The storage manager switches that legacy permission request to our new storage permission. 4. Follow the storage permission path, the storage permission prompts for that request Not sure if the step 3 is possible? Just thinking that way is clean. Otherwise, another way might be like the JS side sees the legacy permission, then tells the storage manager about this, then the storage manager sends out the new storage permission request, then the new storage permission prompts. We can't just transfer a legacy permission request to the storage permission request in the JS side or only the permission manager knows that but the storage manager won't I think.
Comment 17•6 years ago
|
||
(In reply to Fischer [:Fischer] from comment #16) > Here is what I imagined: > 1. Website calls the legacy idb persistent permission api > 2. In the storage manager it sends out the warning in the console to > deprecate that > 3. The storage manager switches that legacy permission request to our new > storage permission. > 4. Follow the storage permission path, the storage permission prompts for > that request > > Not sure if the step 3 is possible? Just thinking that way is clean. Yes, I think if we don't want to remove the permission entirely for now we might want to spend the time to switch to using nsIContentPermissionRequest instead of https://searchfox.org/mozilla-central/source/dom/indexedDB/PermissionRequestBase.h. I could imagine looking into this but I'd need someone who can help me out with storage questions (though I'm also happy if someone else does it). > Otherwise, another way might be like the JS side sees the legacy permission, > then tells the storage manager about this, then the storage manager sends > out the new storage permission request, then the new storage permission > prompts. > > We can't just transfer a legacy permission request to the storage permission > request in the JS side or only the permission manager knows that but the > storage manager won't I think. These sound both really hacky to me and do we go through JS for existing permissions as well? I think not :)
Updated•6 years ago
|
Comment 18•6 years ago
|
||
After thinking about this for a bit, maybe we should clarify the damage that simply ignoring the "storage" parameter on IDBF.open() would have to websites that are currently passing storage: persistent. Wouldn't most websites simply continue working? Let's assume we don't switch permissions and instead adopt the following timeline: 1) Publicly deprecate storage: persistent 2) Put a developer warning in FF 59 3) Watch reactions from developers and telemetry 4) In the 60 or 61 cycle, start to ignore storage: persistent and always give out temporary storage. What problems could "affected" websites run into, e.g. what's the worst breakage that we could expect and how hard is it to fix those breakages? I'd be happy to talk about this in Austin.
Comment 19•6 years ago
|
||
Here's the proposed plan we came up with in Austin: * We put a warning in. And coordinate with DevRel on a blog post where we announce the new persistent storage API. Andrew, can you help coordinate that? I'm happy to help writing it, but perhaps Shawn and Tom want to take ownership given they did all the work? * We disable this capability when no database exists in either legacy temporary or legacy persistent storage with this name and instead create the new database in default storage. We do this at least one release after the warning. As part of this disabling, we can also remove the UI prompt as nobody should get it anymore at that point. * Four releases or so after that we migrate the databases from legacy temporary and legacy persistent to default storage. If there's duplicates (which we think is unlikely) we employ a renaming scheme. In case of migration from legacy persistent to default storage, we also set the new persistent storage permission.
Comment 20•6 years ago
|
||
(In reply to Anne (:annevk) from comment #19) > Here's the proposed plan we came up with in Austin: > > * We put a warning in. And coordinate with DevRel on a blog post where we > announce the new persistent storage API. Andrew, can you help coordinate > that? I'm happy to help writing it, but perhaps Shawn and Tom want to take > ownership given they did all the work? Yes, Shawn and Tom should work with the hacks.mozilla.org team about writing a post. I'll email to kick that off.
This is just a WIP. I will test a bit more tomorrow.
As Jan kindly advised in the Storage meeting, I should also check any internal code uses this persistent api, such as "about:home". Then make these code change to "default" storage type, otherwise we might break functions when migrating folders.
Another potential issue needs to check is that the old "persistent" storage type ignores checking quota limit on purpose. However, removing IDBOpenOption and replace with "navigator.storage.persist()", that means they will bound by global limit, 50% of free disk space. Maybe it's possible to hit global limit when opening new tab (loading AboutHome).
If AboutHome uses open api with persistent storage type, maybe that explains high usage in telemetry.
Comment 26•6 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #24) > Another potential issue needs to check is that the old "persistent" storage > type ignores checking quota limit on purpose. However, removing > IDBOpenOption and replace with "navigator.storage.persist()", that means > they will bound by global limit, 50% of free disk space. Maybe it's > possible to hit global limit when opening new tab (loading AboutHome). Yeah, maybe we should keep the explicit persistent storage in IDB, but just don't expose it to normal web. For example we have a method openForPrincipal() which is chrome only. On the other hand, about:home doesn't have chrome privileges :(
Comment 27•6 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #25) > If AboutHome uses open api with persistent storage type, maybe that explains > high usage in telemetry. Could be, but it doesn't explain a spike in usage (if there was any) because about:home has been using it for long time.
(In reply to Jan Varga [:janv] from comment #26) > Yeah, maybe we should keep the explicit persistent storage in IDB, but just > don't expose it to normal web. > For example we have a method openForPrincipal() which is chrome only. On the > other hand, about:home doesn't have chrome privileges :( Maybe we can check whether the page is using "about" protocol or verify URL scheme like we did in |QuotaManager::IsOriginInternal()|.
Comment 29•6 years ago
|
||
When I checked the telemetry I was already suspecting about:home to cause it but discarded that idea because we ignore chrome callers[0] and I assumed we loaded about:home with chrome privileges, but it turns out we don't[1]... :( Sorry, I could have saved us some time in Austin there. We should really adjust the telemetry to exclude internal callers (about: pages). [0] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/indexedDB/IDBFactory.cpp#454 [1] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/components/about/AboutRedirector.cpp#82 (In reply to Jan Varga [:janv] from comment #27) > (In reply to Shawn Huang [:shawnjohnjr] from comment #25) > > If AboutHome uses open api with persistent storage type, maybe that explains > > high usage in telemetry. > > Could be, but it doesn't explain a spike in usage (if there was any) because > about:home has been using it for long time. I think the "spike" was just me misinterpreting our release schedule. :)
Comment 30•6 years ago
|
||
Andrei, Tim, can one of you please provide us with the following information (or forward to whoever can): - What is AS using indexedDB for? - How much data are you storing in a worst case scenario? - Would it be absolutely necessary to migrate this data in case we change the storage type or can it be easily regenerated? Thank you very much!
Comment 31•6 years ago
|
||
- It uses indexedDB to store various client info: is it a default browser, profile age etc. and also the entire snippet payload served by the endpoint. - Somewhat surprising I can't inspect the DB in the developer tools, but I inspected a snippet response it's around 50KB so worst case it would be 50KB times the number of snippets available. - If we don't migrate the data the only thing we lose is the user blocklist so they will see a snippet campaign again (if that still exists). I don't think the work involved with the migration is worth it but I will defer to k88hudson who did most of the work Thanks
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fd993d647e3dc82e7a76ede7a72abea06748868
Comment 33•6 years ago
|
||
Great! I'll spin off a new bug to ignore about: schemes.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 34•6 years ago
|
||
Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/3b700_oeAzo
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 35•6 years ago
|
||
The site compat note posted for Firefox 61, Bug 1451486.
Comment 36•5 years ago
|
||
Moving to p3 because no activity for at least 24 weeks. See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Comment 37•4 years ago
|
||
Simon, I'm still assigned to this but I think driving this home is on your plate now, would you like to take this bug?
Comment 38•4 years ago
|
||
Johann, I probably can do that, but I am not sure about what needs to be done here. Much of the discussion on this bug is not directly related to the permission, but to the storage parameter to the IDBFactory.open
call. Can you point me to where the permission (called IndexedDB
IIUC?) is defined which ought to be removed as part of this?
Comment 39•4 years ago
|
||
There's a bunch of stuff that's still used to support it, for the sake of WebExtensions mostly:
Maybe there's more. It might also be polite to send a final reminder to the dev-addons list.
Thanks!
Updated•3 years ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 40•1 year ago
|
||
Assignee | ||
Comment 41•1 year ago
|
||
Depends on D151416
Assignee | ||
Comment 42•1 year ago
|
||
Depends on D151417
Assignee | ||
Comment 43•1 year ago
|
||
Depends on D151418
Assignee | ||
Updated•1 year ago
|
Comment 44•1 year ago
|
||
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/31425a693ca3 Part 1: Remove options parameter from nsDOMWindowUtils::GetFileReferences r=dom-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/d3b66dd1f509 Part 2: Remove IDBDatabase#storage r=dom-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/794e786eb394 Part 3: Remove indexedDB/PermissionRequestBase r=dom-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/04c2a21191a3 Part 4: Remove indexedDB permission uses from scripts r=dom-storage-reviewers,rpl,asuth
Comment 45•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31425a693ca3
https://hg.mozilla.org/mozilla-central/rev/d3b66dd1f509
https://hg.mozilla.org/mozilla-central/rev/794e786eb394
https://hg.mozilla.org/mozilla-central/rev/04c2a21191a3
Comment 46•1 year ago
•
|
||
Does this need a migration that removes the now unused permissions from profiles?
Assignee | ||
Comment 47•1 year ago
|
||
Probably yes. Although this has been disabled for a while, addons were still allowed to use this (with a deprecation warning).
Do we have some existing code for the migration?
Comment 48•1 year ago
|
||
It depends on where you want to do the migration. For Firefox you can do it in BrowserGlue here:
https://searchfox.org/mozilla-central/rev/5ea9694f10703efbd2f8c25c107c096309fb8fbb/browser/components/BrowserGlue.jsm#3492
For general Gecko migration you could do it in the permission manager, like here:
https://searchfox.org/mozilla-central/rev/5ea9694f10703efbd2f8c25c107c096309fb8fbb/extensions/permissions/PermissionManager.cpp#1375
Comment 49•1 year ago
|
||
FF104 docs work for this can be tracked in https://github.com/mdn/content/issues/19575#issuecomment-1214684755. This will mostly be addition of the removed option to browser compatibility and deletion of the information from the MDN docs themselves. Release note too.
Updated•1 year ago
|
Description
•