Fix multithreaded _completion access #6068
Conversation
There was a problem hiding this comment.
Seems reasonable to me. @maksymmalyhin would you take a look?
There was a problem hiding this comment.
FIRAuthURLPresenter has a complex state that is mutated along the course of the operation. Making it thread-safe with simple locks may be a challenge. I think we may cover most of the case by the following approach:
- Perform all actions and mutations on a serial dispatch queues. Then we have a better chance to avoid inconsistent states and race conditions.
- (Optional) Consider additional synchronization on the caller side, e.g. use a separate instance of the presenter for each request and make sure there is a single disposable instance at the time.
Feel free to ask questions here or offline as for the details.
| @@ -75,12 +95,15 @@ - (void)presentURL:(NSURL *)URL | |||
| completion:(FIRAuthURLPresentationCompletion)completion { | |||
| if (_isPresenting) { | |||
There was a problem hiding this comment.
If it is possible that presentURL:UIDelegate :callbackMatcher:completion: method may be called at the same time from another thread? If yes, it looks like we have another race condition here (and other places where _isPresenting is used).
There was a problem hiding this comment.
Yes. Looks like this class was originally written to serialize accesses to _isPresenting and _completion on the global auth work queue, which is a serial queue. I removed the locks and moved all accesses to this queue.
There was a problem hiding this comment.
For some reason I cannot see the synchronization on the global auth worker queue in the PR. Is it outside the scope or haven't pushed it yet?
There was a problem hiding this comment.
It's out of scope of the PR. The serialization is pre-existing in the form of dispatches from the SFSafariViewControllerDelegate methods. The bug arose (I think) from calling the wrong completion closure off of the main queue, when it should have been called on the main queue.
There was a problem hiding this comment.
Makes sense, thank you for the explanation! I think it may be useful to add this explanation to the PR description for the history.
| FIRAuthURLPresentationCompletion completion = _completion; | ||
| _completion = nil; | ||
| FIRAuthURLPresentationCompletion completion = [_completion copy]; | ||
| _completion = NULL; |
There was a problem hiding this comment.
I think nil should be used in objc, no?
There was a problem hiding this comment.
nil and NULL will behave the same here, but NULL is more appropriate because _completion as a reference to a block which is technically a C pointer. (this article provides a summary for the difference)
Probably fixes #5979. Without a repro, who knows.
Fixes an issue where if an attempt to present a web URL via SFSafariViewController happened while there was already a URL being presented the error handler callback would be invoked on the wrong thread.
Also added some copies when moving blocks around.