mozilla / self-repair-server

This project is now EOL, replaced by Normandy Recipe Server.
6 stars 11 forks source link

deprecate localStorage for client data / persistent state. #212

Closed gregglind closed 8 years ago

gregglind commented 8 years ago

Issue: localStorage

Ideas:

  1. test if there is persistent data storage available. React accordingly.
  2. use cookies on first startup.
  3. switch to an entirely 'smart server-driven' architecture, where all storage is in cloud.
  4. expose some get/set pref API to Tour, which is 'less erasable'.

Each of these introduces new problems and trade-offs.

gijsk commented 8 years ago

Note that you could technically use localStorage itself to check if localStorage works. Let's say you create a new property "heartbeat-localstorage-check", and you do the following:

var haveChecked = localStorage.getItem("heartbeat-localstorage-check");
if (!haveChecked || haveChecked < 3) {
  localStorage.setItem("heartbeat-localstorage-check", ++haveChecked);
  return;
}

// if we get here, we know that this is the third session since we started saving to
// localStorage, and it persisted across sessions (assuming I didn't make an off-by-one
// error). (note that certain things like crash recovery and other immediate restarts
// like version upgrades (which preserve all state across a shutdown, irrespective
// of prefs) could make it seem to work 1 session and still break the next)

This is essentially what I meant by (2), though I erroneously said "cookie" rather than "localStorage". The concept is the same, though.

gregglind commented 8 years ago

Thanks for the clarification!

Under this system, we have to see you multiple times before we do any work that involves anything around persistence. That would totally allow 'nth time' to work right.

This is obviously much much Hardened.

gregglind commented 8 years ago

Re: localStorage

I am leaning toward a Privacy API that reflects (into Tour) or via some other "Permission".

Also possible: if you turn on privacy prefs (or turn off telemetry), then Heartbeat / Self-Repair just doesnt work.

MattGrimes commented 8 years ago

I like the thinking here, but I think we should be cautious. If you turn on privacy prefs I think we should disable SOME of the hb/self repair features. If you go into about:config and turn it off then it's all gone.

My thinking is that you might not want to hear about keyboard shortcuts or have us tell you about addons (totally understandable), but if you update and lose your bookmarks you'd still be a sad panda knowing that we COULD have retrieved them for you. I think this is something we can discuss further down the road, but just my 2 cents.

gijsk commented 8 years ago

Re: localStorage

  • it only says the have EVER stored localStorage
  • doesn't reflect current privacy prefs.

If they start clearing localStorage, that will remove the value you have, and you would never reach the bit after the return;, right? That would effectively achieve "heartbeat doesn't work" in the sense that you wouldn't prompt them for stuff, ever. Am I misunderstanding what you mean?

gregglind commented 8 years ago

Plan to remedy:

  1. Add new function bool personinfo.isLocalstoragePersistent(store=localStorage). This would use the algorithm above.
  2. Update all recipes that rely on localStorage to only run if localStorage is reliable.
    1. Heartbeat First Impressions
    2. repair search and updates (not yet built)
    3. messaging (if we ever do this again)
  3. Add tests for affected recipes.
  4. Set 'attempt to use localStorage' SOMEWHERE once per session
  5. Add debug flags around this for the local version of this to main.js
  6. Update README.md to reflect sampling changes
  7. Update package.json to 0.12.0. This is a 'major change' due to sampling changes.

Drawbacks

  1. The sample is now subtly different, and excludes:
    1. 'first-time' users
    2. privacy-strict users will be excluded from some samples.
  2. This all relies on the self-repair page js being called ONCE PER DAY. If that assumption changes, this breaks again.

Wish list

  1. I would rather see person info know directly that the user has privacy clearing settings directly. I don't like inferring it.
gregglind commented 8 years ago

Amended:

gregglind commented 8 years ago

@gijsk , please take a look over this. It solves some of the issues.