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

store notification metadata in individual keys to avoid races #1002

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

eritbh
Copy link
Member

@eritbh eritbh commented Oct 10, 2024

Whenever notifier generates a bunch of notifications all at once, the background page has to create a bunch of metadata objects for them all at the same time. Unfortunately since all the metadata objects are held under the same key, and storage calls are asynchronous, we don't have a great way to ensure that the object containing all the keys hasn't had anything else added to it between when we read it and when we write it back with a new key. This results in metadata objects getting overwritten and not existing for some portion of the notifications that are created. If a metadata object doesn't exist for a notification, we assume it's already been cleared, so this results in a bunch of notifications from the group becoming un-dismissable until the page is reloaded.

To fix this, each notification now gets its own top-level key in extension storage for its own metadata. This avoids unsafe modification of a shared storage area. I also moved this data into the session storage area, since we don't care about persisting notification state across browser restarts and having the browser automatically clean up after us in case we miss something is kinda nice.

@eritbh eritbh merged commit cdbde48 into master Oct 10, 2024
3 checks passed
@eritbh eritbh deleted the fix/notification-clearing-race branch October 10, 2024 19:30
@eritbh eritbh added this to the v7 milestone Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant