Fix: don’t send a verification cancel request if the session is closed #1716
Conversation
nimau
force-pushed
the
nimau/PSB-249_crash_cancel_key_verification
branch
from
abce798 to
1306872
Compare
Codecov ReportBase: 17.52% // Head: 37.38% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1716 +/- ##
============================================
+ Coverage 17.52% 37.38% +19.85%
============================================
Files 612 612
Lines 95983 96133 +150
Branches 40286 41556 +1270
============================================
+ Hits 16823 35939 +19116
+ Misses 78649 59146 -19503
- Partials 511 1048 +537
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -707,6 +707,15 @@ - (void)cancelVerificationRequest:(id<MXKeyVerificationRequest>)request | |||
| { | |||
| [self cancelTransaction:transaction code:cancelCode success:success failure:failure]; | |||
| } | |||
| // If the session is closed, don't try to send a request | |||
| else if (self.crypto.mxSession.state == MXSessionStateClosed) | |||
There was a problem hiding this comment.
Does this actually fix the problem? What if the first if branch is evaluated first, we never get to check session being closed. Maybe better to put this at the very start of the cancelVerificationRequest?
I would also be curious to know why cancelVerificationRequest is called in the first place when session is already closed, and perhaps the isClosed check really belongs in the caller of the cancelVerificationRequest. This is because if "cancel" can cause troubles, than so could the other methods in this manager, such as cancelTransaction or sendToOtherInRequest. Do you have reliable steps to reproduce this crash?
There was a problem hiding this comment.
Hi @Anderas, thanks for the feedback.
I moved the check at the very start of the cancelVerificationRequest
cancelVerificationRequest is called by MXKeyVerificationManager when the timeout (5 minutes by default) is fired.
So, if a user logs into the app, uses the recovery key to validate the session, then logs out or clears the cache (anything that invalidates the session), the app will try to send a verification cancel request even if the session is closed.
In DEBUG, this will crash because of the assertion in sendToOtherInRequest, but in practice, it may prevent some completion handler from being called.
There was a problem hiding this comment.
The thing is that when the session is closed (say via the close method), this will also close session.crypto, which will in turn close crypto.keyVerificationManager, even though by nillifying it. In other words when a session is closed no other objects should continue to function, and in the case of MXKeyVerificationManager it should cancel its timeout once closed. The reason this does not work at the moment is that even if crypto nillifies the manager, some other object keeps it in memory (not necessarily as a memory leak).
Whilst your solution does solve this particular problem, I fear this is merely one of many symptoms of the manager continuing its work after a session is closed. I would instead propose this solution:
- add new
closemethod in theMXLegacyKeyVerificationManagerheader file - implement the
closemethod by clearing any timeouts that we have scheduled - call the method just before
self->_keyVerificationManager = nilis called fromMXLegacyCrypto close
There was a problem hiding this comment.
@Anderas, Ok, thanks. The verification manager was retained by its timers. So I removed my previous "fix", change the way we create the timers to prevent them to retain the manager, and finally, properly invalidate the timer on dealloc. It should be better and safer now.
| @@ -101,7 +101,9 @@ - (BOOL)enableBackupWithKeyBackupVersion:(MXKeyBackupVersion *)keyBackupVersion | |||
| self.crypto.store.backupVersion = keyBackupVersion.version; | |||
| Class algorithmClass = AlgorithmClassesByName[keyBackupVersion.algorithm]; | |||
| // store the desired backup algorithm | |||
| MXWeakify(self); | |||
There was a problem hiding this comment.
Nice find on the retain cycles
This PR fixes: