Fix sending of oh_hais on bad sessions #213
Conversation
Fix a bunch of bugs in the code which tried to send an oh_hai message when we got a message with an unknown megolm session.
There was a problem hiding this comment.
two questions; otherwise LGTM. i'm surprised that @NegativeMjark never tested it (and irritated that I didn't review it better)
| // (Alternatively, it might be that we are just looking at | ||
| // scrollback... at least we rate-limit the m.new_device events :/) | ||
| // | ||
| // XXX: this is a band-aid which masks symptoms of other bugs. It would |
There was a problem hiding this comment.
Is the bandaid not useful in papering over scenarios where we upgrade the protocol (e.g. the session id change from 1.2 to 1.3)?
There was a problem hiding this comment.
uh, mayyybee? I'm afraid I'm not entirely clear on the difference between 1.2 and 1.3; and in any case I don't really think this is a good way to solve that problem.
| @@ -897,34 +906,40 @@ Crypto.prototype.decryptEvent = function(event) { | |||
| * @param {string} roomId The ID of the room we want to remind them about. | |||
| */ | |||
| Crypto.prototype._sendPingToDevice = function(userId, deviceId, roomId) { | |||
| if (deviceId === undefined) { | |||
| deviceId = "*"; | |||
There was a problem hiding this comment.
why have we lost this? it feels like it's needed if deviceId is optional.
There was a problem hiding this comment.
deviceId is always included now, so it just felt like cruft to me. I suppose it might as well stay in.
Fix a bunch of bugs in the code which tried to send an oh_hai message when we
got a message with an unknown megolm session.