Page MenuHomePhabricator

Consider switching from sessionStorage to localStorage for autosave
Open, Needs TriagePublic

Description

We used sessionStorage because we wanted to ensure the autosaved changes won't pile up forever, and a guarantee that it will be deleted when the "session" ends was very convenient (saves us code for cleaning up expired entries, and defining when they even should expire, etc.).

But it seems that sessionStorage data is deleted in several cases where autosave would be very convenient:

  • Mobile browsers unloading the page (T217783)
  • Machine shutting down unexpectedly, e.g. due to power loss (T214543)
  • (Unclear what happens in case of a single browser tab crashing vs entire browser crashing, might behave differently)

We should therefore consider switching from sessionStorage to localStorage.

Minimum test case

  1. Draft a comment using the Reply Tool
  2. Close the page you were drafting a comment on/within
  3. Re-open the page from "Step 2." in a new browser tab
  4. ✅ Verify the reply tool opens and the comment you started drafting in "1." is restored

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

The main issue with localStorage is not just that we would need to manage old entries, but that we could also run into limits more often (which would probably result in data loss)

  • Even with aggressive garbage collection some users may end up with 10s or 100s of documents in storage at any one time
  • ResourceLoader uses up a lot of the storage for its cache
  • On Firefox quota is shared across the whole domain, not subdomains, making it more likely to hit quota. RL caching was disabled on FF for this reason.

Mobile browsers unloading the page (T217783)

It's unclear from that bug if this is a general problem with iOS, or a rare edge case where they loaded a webview inside GChat.

If you re-open the same tab (ctrl+shift+t) it should keep your edit.

It actually doesn't keep it. But that's probably acceptable.

(If we wanted to change that, then I think it would require doing T218663 first)

Notes from conversation with @Esanders:

  • This would allow people to write a comment draft, close the browser tab, open a new browser tab with that same talk page and notice their in-progress comment is how they left it.
    • Note: this would not allow people "carry" drafted comments across devices or browsers.

Some users have reported that sessionStorage does not appear to persist in Firefox when the browser crashes.

Firefox also has terrible support for localStorage (https://bugzilla.mozilla.org/show_bug.cgi?id=1064466).

Resolved since FF92: https://bugzilla.mozilla.org/show_bug.cgi?id=1064466#c40

The storage quota/data-expiry issue is definitely solvable (T121646), however a trickier problem is how to handle conflicts that can occur when the storage is shared across multiple tabs:

  • And editor opens document X in tab A
  • They then open document X again in tab B
  • Some text is typed in tab A
  • Some different text is typed in tab B

Both tab A and tab B are trying to store auto-save information for document X which will cause big problems (especially with the append-only ve.init.ListStorage class).

I think one way to do this is to compare the length of the transaction list in storage with a counter in memory. If they ever differ then that means some other tab has modified the storage, and it needs to be re-written from scratch.

Change 807569 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] ListStorage: Handle conflicts when using localStorage

https://gerrit.wikimedia.org/r/807569

The above patch solves the local conflict issue described in T218663#7998655, but instead of comparing the transaction list length, we just generate a random ID for each tab, and write that to localStorage. Whenever we detect the random ID has changed, we know to restore all our data from memory.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/804591/ has been merged, which means we can set expiry times on local storage items. Along with the above patch I think all the technical barriers to doing this have been lifted.

ppelberg moved this task from Next Quarter to Upcoming on the Editing-team board.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/804591/ has been merged, which means we can set expiry times on local storage items. Along with the above patch I think all the technical barriers to doing this have been lifted.

Per today's team meeting, we decided to:

  1. Move forward with deploying the above (read: switching from sessionStorage to localStorage to power the Reply and New Topic Tools' autosave functionality)
  2. Set the expiry times for local storage items to 1 month
  3. Keep the browser's Leave site? dialog warning in place for now. Once we're confident this change to switch to localStorage is working as expected, we'll remove these dialogs in T315499

Change 829078 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Use localStorage for auto-save

https://gerrit.wikimedia.org/r/829078

Change 807569 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Replace ListStorage with ConflictableStorage

https://gerrit.wikimedia.org/r/807569

Change 829848 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (035756895)

https://gerrit.wikimedia.org/r/829848

Test wiki on Patch demo by ESanders (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/7285a906e4/w/

Change 829848 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (035756895)

https://gerrit.wikimedia.org/r/829848

Change 829078 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Use localStorage for auto-save

https://gerrit.wikimedia.org/r/829078

Done for DiscussionTools, not done yet for VisualEditor.

Re-using this task for switching VE to localStorage (as it was originally written), which has more issues with the quota. We probably should've created a sub task for DT, but this is ok.

We could also consider IndexedDB which has a much larger quota.

The new Edit Recovery feature in core is using indexedDB. Is there anything we can do for in that project that'd help you with VE's autosave? (I'm assuming it can't use the same stored data because the structure is different and VE needs much more info to be stored). We have T347864 which relates how to update the stored data when switching between editors.

... Is there anything we can do for in that project that'd help you with VE's autosave? (I'm assuming it can't use the same stored data because the structure is different and VE needs much more info to be stored) ...

I think factoring out the access to IndexedDB access into a generic storage with an API similar to SafeStorage would be the most helpful thing. The major difference will be that IndexedDB is async, but we can make SafeStorageAsync to make it properly interchangeable with the localStorage/sessionStorage versions.