Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

we can run out of localstorage #3660

Closed
richvdh opened this issue Apr 19, 2017 · 7 comments
Closed

we can run out of localstorage #3660

richvdh opened this issue Apr 19, 2017 · 7 comments
Labels
A-E2EE P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@richvdh
Copy link
Member

richvdh commented Apr 19, 2017

because we try to store megolm keys forever, and then all hell breaks loose

@pik
Copy link

pik commented Apr 19, 2017

If using indexedDB I don't think we would run into storage limitations in any realistic scenario:

https://developer.chrome.com/apps/offline_storage#temporary
https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Browser_storage_limits_and_eviction_criteria

Though if such a thing were to occur (e.g. since maximum storage is % of Disk space restriction for most users this would be in the Gigabytes) I would suggest an option to export a file with all of the relevant keys (or maybe all session keys that are unused for X amount of time and the user can figure out where they want to store it).

Btw even if a client prunes old keys, they can later re-request Inbound Megolm Session keys from other members present in the group chat (as long as one user still has a session key) or is there no specification for something like that?

@richvdh
Copy link
Member Author

richvdh commented Apr 19, 2017

Btw even if a client prunes old keys, they can later re-request Inbound Megolm Session keys from other members present in the group chat (as long as one user still has a session key) or is there no specification for something like that?

This would be element-hq/element-meta#647

@richvdh
Copy link
Member Author

richvdh commented Apr 19, 2017

Having just run out of localstorage myself, I thought I would do some quick sums to see what is filling it up.

function keysStarting(p) { return Array.from(Array(localStorage.length).keys()).map((i) => localStorage.key(i)).filter((k) => k.startsWith(p)) }

keysStarting("session.e2e.inboundgroupsession").reduce((acc, k) => acc + (k.length + localStorage[k].length)*2, 0) 
# 1704470

keysStarting("session.e2e.sessions").reduce((acc, k) => acc + (k.length + localStorage[k].length)*2, 0)
# 1038904

keysStarting("session.e2e.devices").reduce((acc, k) => acc + (k.length + localStorage[k].length)*2, 0)
# 7724852

10 * 1024*1024 - (7724852 + 1038904 + 1704470)
# 17534

In summary:

  • Megolm keys: 1.7MB
  • Olm session data: 1.0MB
  • Device info: 7.7MB
  • Other stuff (by subtraction): 0.017MB

This is interesting, because there is no reason for us to have 7MB of device info there: for some reason we have the device info for everyone we've ever seen in any room, including unencrypted rooms. This may be related to #3588.

It also presents a workaround for the problem, which is to delete all the device info. This will of course mean that we forget any details about verified devices, but that may be preferable to an unusable session:

Array.from(Array(localStorage.length).keys()).map((i) => localStorage.key(i)).filter((k) => k.startsWith("session.e2e.devices")).forEach((k) => {delete localStorage[k]})

@pik
Copy link

pik commented Apr 20, 2017

I agree that there is no need to store Device Info in localStorage, indexedDB is a better place for this and as far as I am aware, matrix-js-sdk / riot-web are actually refreshing user-device lists on selecting the user-device pane anyways (although I suppose for message sending it doesn't refresh unless there is a new_device event, not sure if I am remembering correctly?)

On a side-note for dealing with very large e2e rooms, the strategy could be, share first with 'recently active', next with 'online', next with 'offline' - for this to work well the client should be able to send the actual m.megolm encrypted message as soon as key sharing has been done with 'recently active'. (The UI for the later should delay showing an 'unable to decrypt message' and instead give as few minutes to wait for the incoming key-share).

@ara4n ara4n added T-Defect P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Apr 22, 2017
@richvdh
Copy link
Member Author

richvdh commented Apr 25, 2017

I agree that there is no need to store Device Info in localStorage

I think you're missing the point. IndexedDB might be a better place, but my surprise is not that we are storing device info in localstorage, but 7.7MB of it. Basically: #3588 means that we are filling up localstorage with device lists for users we don't care about.

@krombel
Copy link
Contributor

krombel commented Sep 2, 2017

To increase the possibility that this issue gets found:

I had the following error in my dev console

Failed to execute 'setItem' on 'Storage': Setting the value of 'session.e2e.devices/<MXID>' exceeded the quota.

@richvdh
Copy link
Member Author

richvdh commented Feb 5, 2018

this should no longer be an issue (at least once riot-web is released), because @dbkr has moved things to indexeddb.

@richvdh richvdh closed this as completed Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

No branches or pull requests

4 participants