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

Inconsistency: Persistence of alarms in browser.alarms API #406

Open
oliverdunk opened this issue Jun 8, 2023 · 10 comments
Open

Inconsistency: Persistence of alarms in browser.alarms API #406

oliverdunk opened this issue Jun 8, 2023 · 10 comments
Labels
inconsistency Inconsistent behavior across browsers supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari topic: alarms

Comments

@oliverdunk
Copy link
Member

The persistence of alarms created using the browser.alarms API is different across browsers.

MDN states the following, which appears to be true in Firefox and Safari:

Alarms do not persist across browser sessions.

In Chrome, alarms do generally persist. Due to an open bug (https://crbug.com/1285798) they can be lost if the extension is reloaded and then there is a restart before any further changes are made to the registered alarms.

My instinct is that persisting alarms across all browsers would make sense:

  • This would lead to consistent behaviour without being a breaking change for extensions in Chrome that rely on it, or extensions in other browsers which are likely recreated on each restart anyway.
  • As browsers move to non-persistent background contexts, this would allow alarms to continue being registered without developers needing to listen to runtime.onInstalled and runtime.onStartup to re-create them (not to mention the missing onEnabled event).
@bbruneau
Copy link

bbruneau commented Jun 8, 2023

At 1Password, we use alarms as part of a system of watching for user idleness and ensuring that we lock the user's session to safeguard their secrets. Persistence is very desirable to us, and in fact the promise of persistence across session and service worker lifetimes is quite important to us. That said, if this were an opt-in feature, this would still work for us (we would, of course, opt in!)

Would an opt-in option require a new permissions setting, and would such a setting require a user-facing warning? This would likely be detrimental to us.

@zombie
Copy link
Collaborator

zombie commented Jun 8, 2023

It's rather unfortunate that browsers have been shipping with different behavior for a while. If we try to align now, whichever option we choose, it will likely cause incompatibilities with existing extensions in the browser which previously didn't have the proclaimed "correct" behavior.

After a brief discussion in the meeting, I believe we have consensus that allowing extensions to explicitly choose persistence (or non-persistence) would enable consistent behavior across browsers, so implementing an additional persistent option for alarms.create() by all browsers, while keeping the default undefined (if unspecified) is a good way forward.

@Rob--W
Copy link
Member

Rob--W commented Jun 8, 2023

I would propose to use the persistAcrossSessions name, consistent with the same name in the scripting API.

@Rob--W Rob--W added follow-up: chrome Needs a response from a Chrome representative supportive: safari Supportive from Safari supportive: firefox Supportive from Firefox labels Jun 8, 2023
@Rob--W
Copy link
Member

Rob--W commented Jun 8, 2023

In the meeting everyone expressed support. @oliverdunk I'm putting follow-up: chrome Needs a response from a Chrome representative here so you can check in with Chrome engineering before slapping the supportive: chrome Supportive from Chrome label on this issue.

@hanguokai
Copy link
Member

so implementing an additional persistent option for alarms.create() by all browsers, while keeping the default undefined (if unspecified) is a good way forward.

Persistent alarms by default are important and reasonable for developers. If adding a new parameter, it should be true by default for Chrome for compatibility. Thus, the default value of this parameter is different in different browsers.

@oliverdunk
Copy link
Member Author

oliverdunk commented Jun 9, 2023

We are supportive of controlling persistence in Chrome using a flag named persistAcrossSessions. This would default to true in Chrome to match our existing behaviour.

One thing I did want to clarify after some more discussions is that alarms in Chrome should not persist across extension updates, including reloading unpacked extensions (which is similar to an update behind the scenes). This is not always the case today due to https://crbug.com/1285798 but we plan to fix that as part of this.

Edit: I want to take another look at what we do in other APIs and go back to the team before making a final stance on if we think these should persist across extension updates.

@oliverdunk oliverdunk added supportive: chrome Supportive from Chrome and removed follow-up: chrome Needs a response from a Chrome representative labels Jun 9, 2023
@hanguokai
Copy link
Member

Persistence across extension updates is still reasonable, otherwise developers must restore alarms in runtime.onInstalled. Extension updates are typically to provide new features or fix bugs, rather than breaking existing behavior. Restoring alarms also means saving them in storage.

@Rob--W
Copy link
Member

Rob--W commented Jun 12, 2023

We are supportive of controlling persistence in Chrome using a flag named persistAcrossSessions. This would default to true in Chrome to match our existing behaviour.

One thing I did want to clarify after some more discussions is that alarms in Chrome should not persist across extension updates, including reloading unpacked extensions (which is similar to an update behind the scenes). This is not always the case today due to https://crbug.com/1285798 but we plan to fix that as part of this.

The semantics of persistAcrossSessions (in the scripting API) is:

  • "persistent across sessions", i.e. browser restarts and browser updates
  • cleared upon extension updates
  • reloading an extension: cleared if treated as an extension update (along with triggering runtime.onInstalled), kept otherwise.

I believe that similar semantics would make sense.

Persistence across extension updates is still reasonable, otherwise developers must restore alarms in runtime.onInstalled. Extension updates are typically to provide new features or fix bugs, rather than breaking existing behavior. Restoring alarms also means saving them in storage.

While I acknowledge that there are use cases where it's desirable to persist alarms across extension updates, I'll also note that doing so carries the risk of the extension unknowingly encountering alarms from a previous version. If persistence of alarms across extension updates is to be supported, then that should happen on an opt-in basis only, e.g. through a manifest key.

@bershanskiy
Copy link
Member

I looked into how this could be implemented in Chromium and came up with the following possible edge cases:

  • it is not entirely clear how to define the "browsing session". It can span multiple browser restarts if profile is set to "Continue where you left off" on chrome://settings/onStartup
  • when should the non-cross-session-persistent alarms be removed from storage? During session shutdown or during session start (in AlarmManager::ReadFromStorage() called by AlarmManager::OnExtensionLoaded()? Probably, both places to ensure minimal amount of retained data after browser shutdown and correct behavior during restart after browser crash.
  • does sub-module update count as an extension update?

@oliverdunk
Copy link
Member Author

@bershanskiy Super cool! Happy to chat about this more in the Chromium issue, but my initial thinking was that rather than trying to remove it from storage, we would simply not write it. That solves the first half of this. To make sure reloads also clear the alarm we would likely need to clear the AlarmManager on certain events. I'm not certain what the behaviour of other APIs is with "Continue where you left off" but I assume it's ignored in which case it probably makes sense to do the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inconsistency Inconsistent behavior across browsers supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari topic: alarms
Projects
None yet
Development

No branches or pull requests

7 participants