Closed Bug 1354500 Opened 8 years ago Closed 2 years ago

Remove the proprietary persistent indexedDB permission

Categories

(Firefox :: Site Permissions, task, P3)

task

Tracking

()

RESOLVED FIXED
104 Branch
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

Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/3b700_oeAzo
Use counter: https://georgf.github.io/usecounters/index.html#kind=page&group=DEPRECATED&channel=beta&version=60

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
Blocks: 1254428
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.
See Also: → 1405742
See Also: → 1348223
Depends on: 1334411
Jan, is there still anything blocking this from your perspective? It'd be good to get this done.
Flags: needinfo?(jvarga)
Shawn, I remember you added a telemetry for this, what did you find out ?
Flags: needinfo?(shuang)
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
Flags: needinfo?(shuang)
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.
Yes, we should add a warning at this point. A blog post would be even better to help developers with the transition.
Flags: needinfo?(jvarga)
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
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.
(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.
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.
Flags: needinfo?(shuang)
(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.
(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.
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?
Flags: needinfo?(shuang) → needinfo?(fliu)
(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.
Flags: needinfo?(fliu)
(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 :)
Whiteboard: [storage-v2][triage]
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.
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.
Flags: needinfo?(overholt)
(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.
Flags: needinfo?(overholt)
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.
(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 :(
(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()|.
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. :)
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!
Flags: needinfo?(tspurway)
Flags: needinfo?(andrei.br92)
- 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
Flags: needinfo?(tspurway)
Flags: needinfo?(khudson)
Flags: needinfo?(andrei.br92)
Great! I'll spin off a new bug to ignore about: schemes.
Depends on: 1428320
Blocks: storage-v2
Priority: -- → P1
Whiteboard: [storage-v2][triage] → [storage-v2]
Priority: P1 → P2
Flags: needinfo?(khudson)
Depends on: 1442560
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1445318
Depends on: 1445326
Depends on: 1448491
User Story: (updated)
Depends on: 1409054
Depends on: 1451486
Depends on: 1451794
Keywords: site-compat
The site compat note posted for Firefox 61, Bug 1451486.
Keywords: site-compat
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
Priority: P1 → P3

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?

Assignee: jhofmann → nobody
Status: ASSIGNED → NEW
Type: enhancement → task
Flags: needinfo?(sgiesecke)

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?

Flags: needinfo?(sgiesecke) → needinfo?(jhofmann)

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!

Flags: needinfo?(jhofmann)
Blocks: 1254928
Severity: normal → N/A
Assignee: nobody → krosylight
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

Does this need a migration that removes the now unused permissions from profiles?

Flags: needinfo?(krosylight)

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?

Flags: needinfo?(krosylight) → needinfo?(pbz)

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

Flags: needinfo?(pbz)

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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: