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

Fix origin migrator for SSO logins #10920

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Sep 19, 2019

For some reason this was trying to close the same window twice
when the app was reloaded after an SSO login. Possibly also a
problem on electron < 6 - presumably a race condition.

Merging to release branch

For some reason this was trying to close the same window twice
when the app was reloaded after an SSO login. Possibly also a
problem on electron < 6 - presumably a race condition.
@dbkr dbkr requested a review from a team September 19, 2019 16:21
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Hmm, why would the events come back multiple times...? Seems suspicious right?

@dbkr
Copy link
Member Author

dbkr commented Sep 19, 2019

Well, the events coming back is legit since with SSO we are coming back to the page and therefore loading up the app again, causing another origin migration attempt. In this case the old listeners will still be there.

@jryans
Copy link
Collaborator

jryans commented Sep 19, 2019

Hmm, I see... What about the case of getting origin_migration_complete one time followed by origin_migration_nodata the next? Is that possible?

Effectively I am wondering if we need to remove all listeners whenever any one of them fires.

@dbkr
Copy link
Member Author

dbkr commented Sep 19, 2019

Yeah, we technically do, although in practice nobody should actually need the migrator now so there's practically zero chance of actually hitting that case. That said, I guess if we're fixing it we should fix it properly.

@dbkr dbkr requested a review from jryans September 19, 2019 16:41
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! 😁

electron_app/src/originMigrator.js Outdated Show resolved Hide resolved
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
@dbkr dbkr merged commit 0e406c5 into release-v1.3.6 Sep 19, 2019
@t3chguy t3chguy deleted the dbkr/fix_originmigrator_electron6_2 branch May 12, 2022 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants