Skip to content

Commit

Permalink
android notif: Fix bug clearing notifs with colliding message IDs
Browse files Browse the repository at this point in the history
This bug would be tricky to reproduce, but seems definitely there
from studying the code.

When we get a RemoveFcmMessage, it lists a number of Zulip message
IDs which were read, and for which we should clear notifications in
the UI.  But a Zulip message ID is unique only on a given server; a
message ID like 12345 will identify one message on one server, and
some other message on another server.  So it's entirely possible
(though it'd be a rather unlucky coincidence) for the user to have
active notifications at the same time for two conversations on two
different Zulip servers that happen to include messages with the
same Zulip message ID, and even for their respective latest messages
to have the same message ID.

In that case, when the user goes and reads one of those
conversations on one server so that we get a RemoveFcmMessage for
that message, the existing (recently-added) code here would clear
both conversations' notifications.

The bug could also trigger just as well if the notification for the
just-read message was already cleared -- for example, if the user
opened that notification and that's what led to reading the message.
In that case, we'd clear just one notification, but it'd be a wrong
one that shouldn't be cleared.

To fix the bug, we just need to limit this loop to operate on
notifications that are for the right Zulip identity.  (Limiting to
the right Zulip server would be enough to fix this bug; but limiting
to the right identity is both easier and cleaner, as the whole
RemoveFcmMessage is addressed to a particular Zulip identity.)
  • Loading branch information
gnprice committed Nov 16, 2021
1 parent 1c45840 commit cb7f6aa
Showing 1 changed file with 3 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ private fun removeNotification(context: Context, fcmMessage: RemoveFcmMessage) {
// So don't try to match those.
val notification = statusBarNotification.notification

// Don't act on notifications that are for other Zulip accounts/identities.
if (notification.group != groupKey) continue;

val lastMessageId = notification.extras.getInt("lastZulipMessageId")
if (fcmMessage.messageIds.contains(lastMessageId)) {
// The latest Zulip message in this conversation was read.
Expand Down

0 comments on commit cb7f6aa

Please sign in to comment.