Fischer [:Fischer] Reporter
	Description • 7 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

	Anne (:annevk)
	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.

	Anne (:annevk)
	Comment 2 • 7 years ago

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

	Jan Varga [:janv]
	Comment 3 • 7 years ago

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 4 • 7 years ago

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

	Anne (:annevk)
	Comment 5 • 7 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.

	Jan Varga [:janv]
	Comment 6 • 7 years ago

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

	Johann Hofmann [:johannh]
	Comment 7 • 7 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?

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 8 • 7 years ago

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

	Anne (:annevk)
	Comment 9 • 7 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.

	Johann Hofmann [:johannh]
	Comment 10 • 7 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.

	Anne (:annevk)
	Comment 11 • 7 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.

	Jan Varga [:janv]
	Comment 12 • 7 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.

	Johann Hofmann [:johannh]
	Comment 13 • 7 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.

	Anne (:annevk)
	Comment 14 • 7 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.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 15 • 7 years ago

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

	Fischer [:Fischer] Reporter
	Comment 16 • 7 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.

	Johann Hofmann [:johannh]
	Comment 17 • 7 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 :)

	Johann Hofmann [:johannh]
	Comment 18 • 7 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.

	Anne (:annevk)
	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.

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 21 • 6 years ago

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 23 • 6 years ago

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.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 24 • 6 years ago

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 25 • 6 years ago

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

	Jan Varga [:janv]
	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 :(

	Jan Varga [:janv]
	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.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 28 • 6 years ago

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

	Johann Hofmann [:johannh]
	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. :)

	Johann Hofmann [:johannh]
	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!

	Andrei Oprea [:andreio]
	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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 32 • 6 years ago

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

	Johann Hofmann [:johannh]
	Comment 33 • 6 years ago

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

	Johann Hofmann [:johannh]
	Comment 34 • 6 years ago

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

	Kohei Yoshino [:kohei]
	Comment 35 • 6 years ago

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

	BugBot [:suhaib / :marco/ :calixte]
	Comment 36 • 6 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

	Pulsebot
	Comment 44 • 2 years 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