Remove the proprietary persistent indexedDB permission

ASSIGNED
Assigned to

Status

()

P1
normal
ASSIGNED
2 years ago
4 months ago

People

(Reporter: Fischer, Assigned: johannh)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(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

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Blocks: 1254428

Comment 1

2 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.
Keywords: dev-doc-needed
See Also: → bug 1405742
(Reporter)

Updated

11 months ago
See Also: → bug 1348223
(Reporter)

Updated

11 months ago
Depends on: 1334411

Comment 2

10 months ago
Jan, is there still anything blocking this from your perspective? It'd be good to get this done.
Flags: needinfo?(jvarga)

Comment 3

10 months ago
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)

Comment 5

10 months 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

10 months ago
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)
(Assignee)

Comment 7

10 months 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

10 months 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.
(Assignee)

Comment 10

10 months 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

10 months 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.
Flags: needinfo?(shuang)

Comment 12

10 months 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.
(Assignee)

Comment 13

10 months 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

10 months 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?
Flags: needinfo?(shuang) → needinfo?(fliu)
(Reporter)

Comment 16

10 months 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.
Flags: needinfo?(fliu)
(Assignee)

Comment 17

10 months 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 :)
(Assignee)

Updated

10 months ago
Whiteboard: [storage-v2][triage]
(Assignee)

Comment 18

10 months 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

9 months 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.
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)
Assignee: nobody → shuang
Created attachment 8937677 [details] [diff] [review]
[WIP] Bug 1354500 - Warn about the proprietary persistent storage type

This is just a WIP. I will test a bit more tomorrow.
Attachment #8937677 - Attachment is obsolete: true
Created attachment 8937937 [details] [diff] [review]
[WIP] Bug 1354500 - Warn about the proprietary persistent storage type
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

9 months 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

9 months 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()|.
(Assignee)

Comment 29

9 months 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. :)
(Assignee)

Comment 30

9 months 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!
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)
(Assignee)

Comment 33

9 months ago
Great! I'll spin off a new bug to ignore about: schemes.
(Assignee)

Updated

9 months ago
Depends on: 1428320
(Assignee)

Updated

9 months ago
Blocks: 1421690
Priority: -- → P1
(Assignee)

Updated

9 months ago
Whiteboard: [storage-v2][triage] → [storage-v2]
Assignee: shuang → nobody
(Assignee)

Updated

8 months ago
Priority: P1 → P2

Updated

7 months ago
Flags: needinfo?(khudson)
(Assignee)

Updated

7 months ago
Depends on: 1442560
(Assignee)

Updated

7 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

7 months ago
Depends on: 1445318
(Assignee)

Updated

7 months ago
Depends on: 1445326
(Assignee)

Updated

6 months ago
Depends on: 1448491
(Assignee)

Updated

6 months ago
User Story: (updated)
(Assignee)

Updated

6 months ago
Depends on: 1409054
(Assignee)

Updated

6 months ago
Depends on: 1451486
(Assignee)

Updated

6 months ago
Depends on: 1451794
(Assignee)

Updated

6 months ago
Blocks: 1453587

Updated

5 months ago
Keywords: site-compat
The site compat note posted for Firefox 61, Bug 1451486.
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.