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.

Sign up for GitHub

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

Jump to bottom

Remove identityState when deleting containers #1141

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Remove identityState when deleting containers #1141

merged 1 commit into from
Jul 15, 2022

Conversation

stoically
Copy link
Member

@stoically stoically commented Feb 28, 2018 •
edited
Loading

Fixes #1140

@stoically stoically mentioned this pull request Feb 28, 2018
Closed
@stoically stoically force-pushed the delete-identitystate branch 14 times, most recently from 9220d56 to 96e221e Compare March 2, 2018 05:32
@stoically
Copy link
Member Author

Some users of Temporary Containers are up to 100k+ opened (and deleted) containers, and hence with 100k entries in MACs storage.js. Would be nice if we could merge this to avoid it in the future.

groovecoder
groovecoder previously requested changes Sep 20, 2019
View reviewed changes
Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like #1131 is merged, but this is still red on Travis? I also tried to rebase this branch on latest master and got the same test failures. So I think this needs some updating before we can merge it.

@stoically
Copy link
Member Author

stoically commented Sep 21, 2019 •
edited
Loading

Fixed. The mock for browser.contextualIdentities.remove was missing.

#1131 was merged into the testing-7.0.0 branch which never got merged into master. #1464 now includes those test-only changes, like introducing sinon-chrome to automatically mock all APIs.

@casual-will
Copy link

I am one of the Temporary Containers users. Could this be related to #1675 ?

@stoically
Copy link
Member Author

stoically commented Feb 21, 2022 •
edited
Loading

@liambluebox It's not. It generates other left-over data.

@casual-will
Copy link

@groovecoder any chance this could be looked at?

Nomes77 added a commit to Nomes77/multi-account-containers that referenced this pull request May 18, 2022
@Nomes77
https://github.com/mozilla/multi-account-containers/pull/1141
5495166
@stoically stoically mentioned this pull request May 27, 2022
Merged
@stoically stoically force-pushed the delete-identitystate branch 2 times, most recently from 77f7a78 to 08e9ad7 Compare June 22, 2022 23:45
dannycolin
dannycolin approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@dannycolin dannycolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. LGTM.

@dannycolin dannycolin removed the request for review from groovecoder July 12, 2022 17:37
@dannycolin dannycolin dismissed groovecoder’s stale review July 12, 2022 17:38

Changes have been made and reviewed by me

@stoically stoically merged commit 2f32c91 into mozilla:main Jul 15, 2022
@stoically stoically deleted the delete-identitystate branch July 15, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannycolin dannycolin dannycolin approved these changes

@groovecoder groovecoder groovecoder left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IdentityState is not removed from storage when deleting containers
4 participants
@stoically @casual-will @groovecoder @dannycolin