From 490e41d6ca5b61415d8adc1ca70cc32578e82c4c Mon Sep 17 00:00:00 2001 From: Thilo Molitor Date: Tue, 12 Nov 2024 03:18:23 +0100 Subject: [PATCH] Fix double-disconnect assertion when creating unusable account When creating a new account that can not be connected to, a timeout error is triggered after 30 seconds. If the app was put into background, a disconnect and receiveQueue freeze could happen before the timeout handler tries to disconnect again. Prviosuly this triggered an assertion, now we try to handle that case more gracefully while not introducing new race conditions. --- Monal/Classes/xmpp.m | 133 +++++++++++++++++++++++-------------------- 1 file changed, 72 insertions(+), 61 deletions(-) diff --git a/Monal/Classes/xmpp.m b/Monal/Classes/xmpp.m index fa6e985ce..c3a3863f2 100644 --- a/Monal/Classes/xmpp.m +++ b/Monal/Classes/xmpp.m @@ -829,19 +829,21 @@ -(void) unfreeze //this operation has highest priority to make sure it will be executed first once unfrozen NSBlockOperation* unfreezeOperation = [NSBlockOperation blockOperationWithBlock:^{ //this has to be the very first thing even before unfreezing the parse or send queues - if(self.accountState < kStateReconnecting) - { - DDLogInfo(@"Reloading UNfrozen account %@", self.accountNo); - //(re)read persisted state (could be changed by appex) - [self readState]; + @synchronized(self->_stateLockObject) { + if(self.accountState < kStateReconnecting) + { + DDLogInfo(@"Reloading UNfrozen account %@", self.accountNo); + //(re)read persisted state (could be changed by appex) + [self readState]; + } + else + DDLogInfo(@"Not reloading UNfrozen account %@, already connected", self.accountNo); + + //this must be inside the dispatch async, because it will dispatch *SYNC* to the receive queue and potentially block or even deadlock the system + [self unfreezeParseQueue]; + + [self unfreezeSendQueue]; } - else - DDLogInfo(@"Not reloading UNfrozen account %@, already connected", self.accountNo); - - //this must be inside the dispatch async, because it will dispatch *SYNC* to the receive queue and potentially block or even deadlock the system - [self unfreezeParseQueue]; - - [self unfreezeSendQueue]; }]; unfreezeOperation.queuePriority = NSOperationQueuePriorityVeryHigh; //make sure this will become the first operation executed once unfrozen [self->_receiveQueue addOperations: @[unfreezeOperation] waitUntilFinished:NO]; @@ -962,11 +964,67 @@ -(void) disconnectWithStreamError:(MLXMLNode* _Nullable) streamError -(void) disconnectWithStreamError:(MLXMLNode* _Nullable) streamError andExplicitLogout:(BOOL) explicitLogout { DDLogInfo(@"disconnect called..."); + //commonly used by shortcut outside of receive queue and called from inside the receive queue, too + monal_void_block_t doExplicitLogout = ^{ + @synchronized(self->_stateLockObject) { + DDLogVerbose(@"explicitLogout == YES --> clearing state"); + + //preserve unAckedStanzas even on explicitLogout and resend them on next connect + //if we don't do this, messages could get lost when logging out directly after sending them + //and: sending messages twice is less intrusive than silently loosing them + NSMutableArray* stanzas = self.unAckedStanzas; + + //reset smacks state to sane values (this can be done even if smacks is not supported) + [self initSM3]; + self.unAckedStanzas = stanzas; + + //inform all old iq handlers of invalidation and clear _iqHandlers dictionary afterwards + @synchronized(self->_iqHandlers) { + for(NSString* iqid in [self->_iqHandlers allKeys]) + { + DDLogWarn(@"Invalidating iq handler for iq id '%@'", iqid); + if(self->_iqHandlers[iqid][@"handler"] != nil) + $invalidate(self->_iqHandlers[iqid][@"handler"], $ID(account, self), $ID(reason, @"disconnect")); + else if(self->_iqHandlers[iqid][@"errorHandler"]) + ((monal_iq_handler_t)self->_iqHandlers[iqid][@"errorHandler"])(nil); + } + self->_iqHandlers = [NSMutableDictionary new]; + } + + //invalidate pubsub queue (*after* iq handlers that also might invalidate a result handler of the queued operation) + [self.pubsub invalidateQueue]; + + //clear pipeline cache + self->_pipeliningState = kPipelinedNothing; + self->_cachedStreamFeaturesBeforeAuth = nil; + self->_cachedStreamFeaturesAfterAuth = nil; + + //clear all reconnection handlers + @synchronized(self->_reconnectionHandlers) { + [self->_reconnectionHandlers removeAllObjects]; + } + + //persist these changes + [self persistState]; + } + + [[DataLayer sharedInstance] resetContactsForAccount:self.accountNo]; + + //trigger view updates to make sure enabled/disabled account state propagates to all ui elements + [[MLNotificationQueue currentQueue] postNotificationName:kMonalRefresh object:nil userInfo:nil]; + }; //short-circuit common case without dispatching to receive queue //this allows calling a noop disconnect while the receive queue is frozen - if(self->_accountState an unfreeze can not happen half way through this explicit logout and therefore can't corrupt any state + //--> an unfreeze is needed to dispatch to the receive queue which is used by our connect method + if(self->_accountState_stateLockObject) { - DDLogVerbose(@"explicitLogout == YES --> clearing state"); - - //preserve unAckedStanzas even on explicitLogout and resend them on next connect - //if we don't do this, messages could get lost when logging out directly after sending them - //and: sending messages twice is less intrusive than silently loosing them - NSMutableArray* stanzas = self.unAckedStanzas; - - //reset smacks state to sane values (this can be done even if smacks is not supported) - [self initSM3]; - self.unAckedStanzas = stanzas; - - //inform all old iq handlers of invalidation and clear _iqHandlers dictionary afterwards - @synchronized(self->_iqHandlers) { - for(NSString* iqid in [self->_iqHandlers allKeys]) - { - DDLogWarn(@"Invalidating iq handler for iq id '%@'", iqid); - if(self->_iqHandlers[iqid][@"handler"] != nil) - $invalidate(self->_iqHandlers[iqid][@"handler"], $ID(account, self), $ID(reason, @"disconnect")); - else if(self->_iqHandlers[iqid][@"errorHandler"]) - ((monal_iq_handler_t)self->_iqHandlers[iqid][@"errorHandler"])(nil); - } - self->_iqHandlers = [NSMutableDictionary new]; - } - - //invalidate pubsub queue (*after* iq handlers that also might invalidate a result handler of the queued operation) - [self.pubsub invalidateQueue]; - - //clear pipeline cache - self->_pipeliningState = kPipelinedNothing; - self->_cachedStreamFeaturesBeforeAuth = nil; - self->_cachedStreamFeaturesAfterAuth = nil; - - //clear all reconnection handlers - @synchronized(self->_reconnectionHandlers) { - [self->_reconnectionHandlers removeAllObjects]; - } - - //persist these changes - [self persistState]; - } - - [[DataLayer sharedInstance] resetContactsForAccount:self.accountNo]; - - //trigger view updates to make sure enabled/disabled account state propagates to all ui elements - [[MLNotificationQueue currentQueue] postNotificationName:kMonalRefresh object:nil userInfo:nil]; - } + doExplicitLogout(); return; } DDLogInfo(@"disconnecting");