Threading code in jabber makes no sense

[arivanov]

I am trying to figure out the threading portion of jabber to allow for better logging using jabberd(2) and bandersnatch. When reading the relevant portions of the pifgin jabber implementation I came along the following code snippet:

Version 2.2.0:

message.c around line 129

if(jbr->thread_id)

g_free(jbr->thread_id);

jbr->thread_id = g_strdup(jbr->thread_id);

If I understand it correctly the thread_id gets freed if non NULL and after that the freed string gets dup-ed.

I apologise, but this does not quite make sense to me. If I understand correctly this will always result the jbr->tread_id being equal NULL.

[deryni]

Correct, that code does in fact make no sense and I have had a change to it locally for some weeks now but as I don't talk to anyone on XMPP that uses a client that handles threading in any real way I have been unable to test it and therefore unwilling to commit it.

The change is to make the code read:

if (jm->thread_id) {

g_free(jbr->thread_id); jbr->thread_id = g_strdup(jm->thread_id);

}

but like I said I haven't tested that at all beyond the fact that it compiles and I am not really at all familiar with the thread code in pidgin. I do believe that pidgin is also not following the best practices for threads as described in XEP-0201 in some ways but haven't had time to look into it more than that.

Feel free to test my change and report back and/or to work on fixing up the thread code and submit patches.

[arivanov]

I have added code which more or less makes Pidgin compliant to the current draft amendments to the XMPP RFCs. It looks correct on tcpdump. I do not have another thread aware client to properly verify it and bandersnatch which is the primary reason for me to do this has the thread logging b0rken.

Principle of operation - if there is no Thread ID set in the message transmit routine one globally unique is generated using the routines in libossp. If an incoming message has a Thread ID it is used for the conversation.

In order to compile and work it requires adding libossp-uuid to the linked library list all the way from the jabber dir through protocols, libpurple, etc to the top makefile. I have hacked the makefiles quick and ugly so I am not going to post that. Someone who understands their structure better than me should add it where it belongs.

The code to generate the UUID itself is straight out of the ossp-uuid manpages. It may be a good idea to move it elsewhere.

*** pidgin-2.2.0/libpurple/protocols/jabber/message.c	2007-09-14 05:45:40.000000000 +0100
--- pidgin-2.2.0-thread/libpurple/protocols/jabber/message.c	2007-09-17 20:56:23.000000000 +0100
***************
*** 31,36 ****
--- 31,52 ----
  #include "message.h"
  #include "xmlnode.h"
  #include "pep.h"
+ #include <ossp/uuid.h>
+ 
+ 
+ char *uuid_v1(void)
+         {
+             uuid_t *uuid;
+             char *str;
+ 
+             uuid_create(&uuid);
+             uuid_make(uuid, UUID_MAKE_V1);
+             str = NULL;
+             uuid_export(uuid, UUID_FMT_STR, (void **)&str, NULL);
+             uuid_destroy(uuid);
+             return str;
+         }
+ 
  
  void jabber_message_free(JabberMessage *jm)
  {
***************
*** 126,134 ****
  				jbr->capabilities |= JABBER_CAP_COMPOSING;
  			}
  
! 			if(jbr->thread_id)
! 				g_free(jbr->thread_id);
! 			jbr->thread_id = g_strdup(jbr->thread_id);
  		}
  		
  		if (jm->js->googletalk && jm->xhtml == NULL) {
--- 142,150 ----
  				jbr->capabilities |= JABBER_CAP_COMPOSING;
  			}
  
! 			if (jm->thread_id) {
! 			      jbr->thread_id = g_strdup(jm->thread_id);
! 			} 
  		}
  		
  		if (jm->js->googletalk && jm->xhtml == NULL) {
***************
*** 620,627 ****
  	jm->chat_state = JM_STATE_ACTIVE;
  
  	if(jbr) {
! 		if(jbr->thread_id)
! 			jm->thread_id = jbr->thread_id;
  
  		if(jbr->chat_states != JABBER_CHAT_STATES_UNSUPPORTED) {
  			jm->typing_style |= JM_TS_JEP_0085;
--- 636,644 ----
  	jm->chat_state = JM_STATE_ACTIVE;
  
  	if(jbr) {
! 		if(!jbr->thread_id)
! 		     jbr->thread_id = uuid_v1();
! 		jm->thread_id = g_strdup(jbr->thread_id);
  
  		if(jbr->chat_states != JABBER_CHAT_STATES_UNSUPPORTED) {
  			jm->typing_style |= JM_TS_JEP_0085;

[nwalp]

Your patch will leak the thread ID with each message.

Also, libpurple should not be creating thread IDs according to my understanding of the spec, which I clarified with stpeter some time ago (if the spec has changed, I could be wrong). According to my understanding, clients that don't actually support threaded conversations should not set thread IDs on their own, just mirror them back when another client sends them.

[arivanov]

I believe the patch does exactly what the first sentence in the Thread section of the most recent draft-saintandre-rfc3921bis. So it looks like this is what St Andre (I assumed you meant him by stPeter) currently thinks:

5.2.3. Thread

The primary use of the XMPP <thread/> element is to uniquely identify a conversation thread or "chat session" between two entities instantiated by <message/> stanzas of type 'chat'.

[snip]

The value of the <thread/> element MUST be a universally unique identifier (UUID) as described in [UUID].

[snip]

You are right that the meaning of thread was slightly different before.

At least in my tests the patch does exactly what the draft says - any new "chat session" is marked with a new UUID and all messages within the conversation are marked with it. The moment both parties leave the conversation the thread is considered finished and no further messages are marked with the same UUID.

I probably need to look how this looks for all use cases especially when one of the people "leaves" the conversation by closing the window while the other one stays. There are some obvious ambiguities on setting/resetting ids for conversations started simultaneously by two people or restarted by a person while another one has closed it.

We should probably also comment on the draft before it becomes yet another slightly underbaked RFC to ensure that these cases are covered.

[bernmeister]

Is there an updated patch for this issue?