1354500 - Remove the proprietary persistent indexedDB permission

Status: RESOLVED FIXED

(Firefox :: Site Permissions, task, P3)

Description

[Fischer [:Fischer]]

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

[Anne (:annevk)]

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.

Comment 2

[Anne (:annevk)]

Jan, is there still anything blocking this from your perspective? It'd be good to get this done.

Comment 3

[Jan Varga [:janv]]

Shawn, I remember you added a telemetry for this, what did you find out ?

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

[Anne (:annevk)]

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

[Jan Varga [:janv]]

Yes, we should add a warning at this point. A blog post would be even better to help developers with the transition.

Comment 7

[Johann Hofmann [:johannh]]

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?

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

[Anne (:annevk)]

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

[Johann Hofmann [:johannh]]

(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

[Anne (:annevk)]

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

[Jan Varga [:janv]]

(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

[Johann Hofmann [:johannh]]

(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

[Anne (:annevk)]

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.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 16

[Fischer [:Fischer]]

(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

[Johann Hofmann [:johannh]]

(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 :)

Comment 18

[Johann Hofmann [:johannh]]

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

[Anne (:annevk)]

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

[Andrew Overholt [: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.

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: [WIP] Bug 1354500 - Warn about the proprietary persistent storage type

This is just a WIP. I will test a bit more tomorrow.

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: [WIP] Bug 1354500 - Warn about the proprietary persistent storage type

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 24

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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).

Comment 25

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

If AboutHome uses open api with persistent storage type, maybe that explains high usage in telemetry.

Comment 26

[Jan Varga [:janv]]

(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

[Jan Varga [:janv]]

(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.

Comment 28

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

[Johann Hofmann [:johannh]]

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

[Johann Hofmann [:johannh]]

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

[Andrei Oprea [:andreio]]

- 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

Comment 32

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fd993d647e3dc82e7a76ede7a72abea06748868

Comment 33

[Johann Hofmann [:johannh]]

Great! I'll spin off a new bug to ignore about: schemes.

Comment 34

[Johann Hofmann [:johannh]]

Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/3b700_oeAzo

Comment 35

[Kohei Yoshino]

The site compat note posted for Firefox 61, Bug 1451486.

Comment 36

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

[Johann Hofmann [:johannh]]

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

[Simon Giesecke [:sg] [he/him]]

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

[Johann Hofmann [:johannh]]

There's a bunch of stuff that's still used to support it, for the sake of WebExtensions mostly:

• The old prompt front-end code
• The native prompt code
• The add-ons code that supports this

Maybe there's more. It might also be polite to send a final reminder to the dev-addons list.

Thanks!

Comment 40

[Kagami [:saschanaz]]

Attachment: Bug 1354500 - Part 1: Remove options parameter from nsDOMWindowUtils::GetFileReferences r=#dom-storage-reviewers

Comment 41

[Kagami [:saschanaz]]

Attachment: Bug 1354500 - Part 2: Remove IDBDatabase#storage r=#dom-storage-reviewers

Depends on D151416

Comment 42

[Kagami [:saschanaz]]

Attachment: Bug 1354500 - Part 3: Remove indexedDB/PermissionRequestBase r=#dom-storage-reviewers

Depends on D151417

Comment 43

[Kagami [:saschanaz]]

Attachment: Bug 1354500 - Part 4: Remove indexedDB permission uses from scripts r=#dom-storage-reviewers

Depends on D151418

Comment 44

[Pulsebot]

Pushed by [e-mail]:
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

[Iulian Moraru]

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

[Paul Zühlcke [:pbz]]

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

Comment 47

[Kagami [:saschanaz]]

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

[Paul Zühlcke [: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

Comment 49

[Hamish Willee]

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.