Closed Bug 1361974 Opened 3 years ago Closed 3 years ago

Add API to check usage of a given storage

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8864471 - Flags: review?(honzab.moz)
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
Attachment #8864471 - Flags: review?(honzab.moz) → review+
(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!
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd0b66f2ab5
Add nsIDOMWindowUtils API to check usage of a given storage r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/0cd0b66f2ab5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Flags: needinfo?(ttaubert)
(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?
Flags: needinfo?(ttaubert)
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?
Flags: needinfo?(ttaubert)
SGTM. I'll attach a patch to bug 1362058 that changed the probe.
Flags: needinfo?(ttaubert)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.