1361974 - Add API to check usage of a given storage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

In SessionStore, we're currently "estimating" storage size by first collecting it and then summing up the string length of the DOMSessionStorage entries. That's bad and we should add a proper API to check usage before we collect anything.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1361974-Add-API-to-check-usage-of-a-given-storag.patch

I hope this isn't too stupid. At first I added it to the StorageManager, but then one could basically pass the sessionStorage to the localStorage manager and still get correct results, which seemed confusing. So instead I just added it to nsIDOMWindowUtils.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8864471 [details] [diff] [review]
0001-Bug-1361974-Add-API-to-check-usage-of-a-given-storag.patch

Review of attachment 8864471 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, why not.

::: dom/storage/StorageCache.cpp
@@ +542,5 @@
>  
> +uint64_t
> +StorageCache::GetOriginQuotaUsage(const Storage* aStorage) const
> +{
> +  return mData[GetDataSetIndex(aStorage)].mOriginQuotaUsage;

mOriginQuotaUsage is signed

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> > +uint64_t
> > +StorageCache::GetOriginQuotaUsage(const Storage* aStorage) const
> > +{
> > +  return mData[GetDataSetIndex(aStorage)].mOriginQuotaUsage;
> 
> mOriginQuotaUsage is signed

Indeed, fixed. Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd0b66f2ab5
Add nsIDOMWindowUtils API to check usage of a given storage r=mayhemer

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd0b66f2ab5

Comment 6

[Chris H-C :chutten]

This bug may have contributed to a sudden change in the Telemetry probe FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS[1] which seems to have occurred in Nightly 20170506[2][3].

It seems as though we're recording many many more storage size estimates.

Is this an improvement? A regression?

Is this intentional? Is this expected?

Is this probe measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1646/alerts/?from=2017-05-06&to=2017-05-06
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155bde3726&tochange=37a5b7f6f101df2eb292b1b6baaf1540c9920e20
[3]: https://mzl.la/2rnszt9

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Chris H-C :chutten from comment #6)
> This bug may have contributed to a sudden change in the Telemetry probe
> FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS[1] which seems to have
> occurred in Nightly 20170506[2][3].
> 
> It seems as though we're recording many many more storage size estimates.

Yeah... Probably dozens while just doing a Google search. Is the amount of data/pings we collect problematic?

> Is this an improvement? A regression?

It's kind of an improvement, we get a lot more data points now :)

> Is this intentional? Is this expected?

Definitely expected but I didn't make that change to get more data points.

> Is this probe measuring something useful?

Great question. It does tell us that people store a lot of data in DOMSessionStorage, but we knew that before already. I wouldn't mind if the probe went away, there's no real value in seeing these data points change over time. Its value was to realize we have to do something about that. WDYT?

Comment 8

[Chris H-C :chutten]

Well, if it's still useful we can keep it. The number of samples it stores doesn't have an adverse effect on storage or bandwidth (which is why we store these things as sparse histograms :) ).

While we're thinking about it, could we bring the definition up-to-date? It needs an alert_emails field containing email addresses of people who know what it means when this histogram changes suddenly (not sure why it isn't alerting session-restore-telemetry-alerts@ like the other session restore probes). It also needs a bug_numbers field containing bugs that affect its behaviour or provide more information (like this one!)

If we're unsure about its usefulness, we can give it five versions to prove it (or prove otherwise) and set an expires_in_version of 61.

Sound good to you?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

SGTM. I'll attach a patch to bug 1362058 that changed the probe.