fix: do not fail to receive call accepted/ended messages referring to non-call Message-ID

In-Reply-To may refer to non-call message
as we do not control the sender.
It may also happen that call message
was received by older version and processed
as text, in which case correct In-Reply-To
appears to be referring to the text message.

Author: link2xt

deltachat-jsonrpc/src/api/types/calls.rs

@@ -1,4 +1,4 @@
-use anyhow::Result;
+use anyhow::{Context as _, Result};
 
 use deltachat::calls::{call_state, sdp_has_video, CallState};
 use deltachat::context::Context;
@@ -26,7 +26,9 @@ pub struct JsonrpcCallInfo {
 
 impl JsonrpcCallInfo {
     pub async fn from_msg_id(context: &Context, msg_id: MsgId) -> Result<JsonrpcCallInfo> {
-        let call_info = context.load_call_by_id(msg_id).await?;
+        let call_info = context.load_call_by_id(msg_id).await?.with_context(|| {
+            format!("Attempting to get call state of non-call message {msg_id}")
+        })?;
         let sdp_offer = call_info.place_call_info.clone();
         let has_video = sdp_has_video(&sdp_offer).unwrap_or_default();
         let state = JsonrpcCallState::from_msg_id(context, msg_id).await?;

src/calls.rs

@@ -213,7 +213,9 @@ impl Context {
         call_id: MsgId,
         accept_call_info: String,
     ) -> Result<()> {
-        let mut call: CallInfo = self.load_call_by_id(call_id).await?;
+        let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| {
+            format!("accept_incoming_call is called with {call_id} which does not refer to a call")
+        })?;
         ensure!(call.is_incoming());
         if call.is_accepted() || call.is_ended() {
             info!(self, "Call already accepted/ended");
@@ -248,7 +250,9 @@ impl Context {
 
     /// Cancel, decline or hangup an incoming or outgoing call.
     pub async fn end_call(&self, call_id: MsgId) -> Result<()> {
-        let mut call: CallInfo = self.load_call_by_id(call_id).await?;
+        let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| {
+            format!("end_call is called with {call_id} which does not refer to a call")
+        })?;
         if call.is_ended() {
             info!(self, "Call already ended");
             return Ok(());
@@ -291,7 +295,13 @@ impl Context {
         call_id: MsgId,
     ) -> Result<()> {
         sleep(Duration::from_secs(wait)).await;
-        let mut call = context.load_call_by_id(call_id).await?;
+        let Some(mut call) = context.load_call_by_id(call_id).await? else {
+            warn!(
+                context,
+                "emit_end_call_if_unaccepted is called with {call_id} which does not refer to a call."
+            );
+            return Ok(());
+        };
         if !call.is_accepted() && !call.is_ended() {
             if call.is_incoming() {
                 call.mark_as_canceled(&context).await?;
@@ -316,7 +326,10 @@ impl Context {
         from_id: ContactId,
     ) -> Result<()> {
         if mime_message.is_call() {
-            let call = self.load_call_by_id(call_id).await?;
+            let Some(call) = self.load_call_by_id(call_id).await? else {
+                warn!(self, "{call_id} does not refer to a call message");
+                return Ok(());
+            };
 
             if call.is_incoming() {
                 if call.is_stale() {
@@ -352,7 +365,11 @@ impl Context {
         } else {
             match mime_message.is_system_message {
                 SystemMessage::CallAccepted => {
-                    let mut call = self.load_call_by_id(call_id).await?;
+                    let Some(mut call) = self.load_call_by_id(call_id).await? else {
+                        warn!(self, "{call_id} does not refer to a call message");
+                        return Ok(());
+                    };
+
                     if call.is_ended() || call.is_accepted() {
                         info!(self, "CallAccepted received for accepted/ended call");
                         return Ok(());
@@ -377,7 +394,11 @@ impl Context {
                     }
                 }
                 SystemMessage::CallEnded => {
-                    let mut call = self.load_call_by_id(call_id).await?;
+                    let Some(mut call) = self.load_call_by_id(call_id).await? else {
+                        warn!(self, "{call_id} does not refer to a call message");
+                        return Ok(());
+                    };
+
                     if call.is_ended() {
                         // may happen eg. if a a message is missed
                         info!(self, "CallEnded received for ended call");
@@ -421,15 +442,26 @@ impl Context {
     }
 
     /// Loads information about the call given its ID.
-    pub async fn load_call_by_id(&self, call_id: MsgId) -> Result<CallInfo> {
+    ///
+    /// If the message referred to by ID is
+    /// not a call message, returns `None`.
+    pub async fn load_call_by_id(&self, call_id: MsgId) -> Result<Option<CallInfo>> {
         let call = Message::load_from_db(self, call_id).await?;
-        self.load_call_by_message(call)
+        Ok(self.load_call_by_message(call))
     }
 
-    fn load_call_by_message(&self, call: Message) -> Result<CallInfo> {
-        ensure!(call.viewtype == Viewtype::Call);
+    // Loads information about the call given the `Message`.
+    //
+    // If the `Message` is not a call message, returns `None`
+    fn load_call_by_message(&self, call: Message) -> Option<CallInfo> {
+        if call.viewtype != Viewtype::Call {
+            // This can happen e.g. if a "call accepted"
+            // or "call ended" message is received
+            // with `In-Reply-To` referring to non-call message.
+            return None;
+        }
 
-        Ok(CallInfo {
+        Some(CallInfo {
             place_call_info: call
                 .param
                 .get(Param::WebrtcRoom)
@@ -497,8 +529,13 @@ pub enum CallState {
 }
 
 /// Returns call state given the message ID.
+///
+/// Returns an error if the message is not a call message.
 pub async fn call_state(context: &Context, msg_id: MsgId) -> Result<CallState> {
-    let call = context.load_call_by_id(msg_id).await?;
+    let call = context
+        .load_call_by_id(msg_id)
+        .await?
+        .with_context(|| format!("{msg_id} is not a call message"))?;
     let state = if call.is_incoming() {
         if call.is_accepted() {
             if call.is_ended() {

src/calls/calls_tests.rs

@@ -1,6 +1,8 @@
 use super::*;
 use crate::chat::forward_msgs;
 use crate::config::Config;
+use crate::constants::DC_CHAT_ID_TRASH;
+use crate::receive_imf::receive_imf;
 use crate::test_utils::{TestContext, TestContextManager};
 
 struct CallSetup {
@@ -53,7 +55,10 @@ async fn setup_call() -> Result<CallSetup> {
     for (t, m) in [(&alice, &alice_call), (&alice2, &alice2_call)] {
         assert!(!m.is_info());
         assert_eq!(m.viewtype, Viewtype::Call);
-        let info = t.load_call_by_id(m.id).await?;
+        let info = t
+            .load_call_by_id(m.id)
+            .await?
+            .expect("m should be a call message");
         assert!(!info.is_incoming());
         assert!(!info.is_accepted());
         assert_eq!(info.place_call_info, PLACE_INFO);
@@ -71,7 +76,10 @@ async fn setup_call() -> Result<CallSetup> {
         t.evtracker
             .get_matching(|evt| matches!(evt, EventType::IncomingCall { .. }))
             .await;
-        let info = t.load_call_by_id(m.id).await?;
+        let info = t
+            .load_call_by_id(m.id)
+            .await?
+            .expect("IncomingCall event should refer to a call message");
         assert!(info.is_incoming());
         assert!(!info.is_accepted());
         assert_eq!(info.place_call_info, PLACE_INFO);
@@ -111,7 +119,10 @@ async fn accept_call() -> Result<CallSetup> {
         .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. }))
         .await;
     let sent2 = bob.pop_sent_msg().await;
-    let info = bob.load_call_by_id(bob_call.id).await?;
+    let info = bob
+        .load_call_by_id(bob_call.id)
+        .await?
+        .expect("bob_call should be a call message");
     assert!(info.is_accepted());
     assert_eq!(info.place_call_info, PLACE_INFO);
     assert_eq!(call_state(&bob, bob_call.id).await?, CallState::Active);
@@ -121,7 +132,10 @@ async fn accept_call() -> Result<CallSetup> {
     bob2.evtracker
         .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. }))
         .await;
-    let info = bob2.load_call_by_id(bob2_call.id).await?;
+    let info = bob2
+        .load_call_by_id(bob2_call.id)
+        .await?
+        .expect("bob2_call should be a call message");
     assert!(info.is_accepted());
     assert_eq!(call_state(&bob2, bob2_call.id).await?, CallState::Active);
 
@@ -140,7 +154,10 @@ async fn accept_call() -> Result<CallSetup> {
             accept_call_info: ACCEPT_INFO.to_string()
         }
     );
-    let info = alice.load_call_by_id(alice_call.id).await?;
+    let info = alice
+        .load_call_by_id(alice_call.id)
+        .await?
+        .expect("alice_call should be a call message");
     assert!(info.is_accepted());
     assert_eq!(info.place_call_info, PLACE_INFO);
     assert_eq!(call_state(&alice, alice_call.id).await?, CallState::Active);
@@ -460,14 +477,20 @@ async fn test_mark_calls() -> Result<()> {
         alice, alice_call, ..
     } = setup_call().await?;
 
-    let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?;
+    let mut call_info: CallInfo = alice
+        .load_call_by_id(alice_call.id)
+        .await?
+        .expect("alice_call should be a call message");
     assert!(!call_info.is_accepted());
     assert!(!call_info.is_ended());
     call_info.mark_as_accepted(&alice).await?;
     assert!(call_info.is_accepted());
     assert!(!call_info.is_ended());
 
-    let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?;
+    let mut call_info: CallInfo = alice
+        .load_call_by_id(alice_call.id)
+        .await?
+        .expect("alice_call should be a call message");
     assert!(call_info.is_accepted());
     assert!(!call_info.is_ended());
 
@@ -484,7 +507,10 @@ async fn test_update_call_text() -> Result<()> {
         alice, alice_call, ..
     } = setup_call().await?;
 
-    let call_info = alice.load_call_by_id(alice_call.id).await?;
+    let call_info = alice
+        .load_call_by_id(alice_call.id)
+        .await?
+        .expect("alice_call should be a call message");
     call_info.update_text(&alice, "foo bar").await?;
 
     let alice_call = Message::load_from_db(&alice, alice_call.id).await?;
@@ -529,3 +555,52 @@ async fn test_forward_call() -> Result<()> {
 
     Ok(())
 }
+
+/// Tests that "end call" message referring
+/// to a text message does not make receive_imf fail.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_end_text_call() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+
+    let received1 = receive_imf(
+        alice,
+        b"From: bob@example.net\n\
+        To: alice@example.org\n\
+        Message-ID: <first@example.net>\n\
+        Date: Sun, 22 Mar 2020 22:37:57 +0000\n\
+        Chat-Version: 1.0\n\
+        \n\
+        Hello\n",
+        false,
+    )
+    .await?
+    .unwrap();
+    assert_eq!(received1.msg_ids.len(), 1);
+    let msg = Message::load_from_db(alice, received1.msg_ids[0])
+        .await
+        .unwrap();
+    assert_eq!(msg.viewtype, Viewtype::Text);
+
+    // Receiving "Call ended" message that refers
+    // to the text message does not result in an error.
+    let received2 = receive_imf(
+        alice,
+        b"From: bob@example.net\n\
+        To: alice@example.org\n\
+        Message-ID: <second@example.net>\n\
+        Date: Sun, 22 Mar 2020 23:37:57 +0000\n\
+        In-Reply-To: <first@example.net>\n\
+        Chat-Version: 1.0\n\
+        Chat-Content: call-ended\n\
+        \n\
+        Call ended\n",
+        false,
+    )
+    .await?
+    .unwrap();
+    assert_eq!(received2.msg_ids.len(), 1);
+    assert_eq!(received2.chat_id, DC_CHAT_ID_TRASH);
+
+    Ok(())
+}