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 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 vector-im/riot-web Sep 19, 2019
Copy link
Member

jryans left a comment

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

@dbkr

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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 Sep 19, 2019
@jryans
jryans approved these changes Sep 19, 2019
Copy link
Member

jryans left a comment

Thanks, looks good! 😁

electron_app/src/originMigrator.js Outdated Show resolved Hide resolved
Typo
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
@dbkr dbkr merged commit 0e406c5 into release-v1.3.6 Sep 19, 2019
5 checks passed
5 checks passed
buildkite/riot-web/pr Build #948 passed (3 minutes, 38 seconds)
Details
buildkite/riot-web/pr/eslint-lint Passed (41 seconds)
Details
buildkite/riot-web/pr/i18n Passed (1 minute, 43 seconds)
Details
buildkite/riot-web/pr/karma-tests Passed (2 minutes, 47 seconds)
Details
buildkite/riot-web/pr/pipeline Passed (9 seconds)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.