-
Notifications
You must be signed in to change notification settings - Fork 365
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 crash when pressing create new account button ios 1019 #7483
Fix crash when pressing create new account button ios 1019 #7483
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPN/View controllers/OutOfTime/OutOfTimeViewController.swift
line 261 at r1 (raw file):
switch result { case let .success(response): MainActor.assumeIsolated {
Would it not make more sense to move the whole switch statement into a MainActor.assumeIsolated { ... }
block instead?
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 454 at r1 (raw file):
/// Even though that closure is not marked `@MainActor`, it is safe to assume isolation here /// because `LoginCoordinator.callDidFinishAfterDelay` will dispatch on the main queue. coordinator.didFinish = { [weak self] _ in
What would be the harm of not marking it @MainActor
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 454 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
What would be the harm of not marking it
@MainActor
here?
Err, sorry, bad phrasing.
What would be the harm of marking it @MainActor
here? Is it just because it isn't calling an asynchronous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 454 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
Err, sorry, bad phrasing.
What would be the harm of marking it@MainActor
here? Is it just because it isn't calling an asynchronous function?
There would be no harm.
I've looked again at this, and given that the LoginCoordinator
is already @MainActor
, its closures will run on the main actor anyway since they're not declared as non isolated.
I'll update this, thanks for catching it !
ios/MullvadVPN/View controllers/OutOfTime/OutOfTimeViewController.swift
line 261 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
Would it not make more sense to move the whole switch statement into a
MainActor.assumeIsolated { ... }
block instead?
Yes it would, I will update this.
ba9573b
to
acafedd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 7 files at r1, 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
acafedd
to
adc7130
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
This PR revisits main actor isolation in the application and adds comments where necessary.
It also fixes a crash where we wrongly assumed that a callback was ran on the main actor.
This change is