[evolution-data-server] Bug 667725 - imapx_untagged: code should not be reached

[David Woodhouse <dwmw2 src gnome org>]

commit 17f3fa1b12faa89158458d976c110cc9f8733a56
Author: David Woodhouse <David Woodhouse intel com>
Date:   Mon May 21 16:45:56 2012 +0100

    Bug 667725 - imapx_untagged: code should not be reached
    
    This code is evil.
    
    When we scan a folder for new messages, we issue a 'FETCH 1:* (UID FLAGS)'
    or similar command.
    
    When we receive an untagged FETCH from the server telling us flags for a
    message, we make a decision about whether that information was solicited
    by such a command, or whether it was unsolicited.
    
    If it was unsolicited, we process it normally as an asynchronous flags
    update and all is well.
    
    If it was solicited, we add the UID to a list. When the FETCH (UID FLAGS)
    command *completes*, we'll sort that list and then fetch the full headers
    for each message.
    
    However, we weren't very good at telling when an update was solicited.
    Assuming that only solicited messages will have a UID is bogus.
    
    This was failing if an unsolicited update came in when the (UID FLAGS)
    fetch had completed, and we were already fetching the message headers.
    The "new" UID would be added to the end of the list, even if we were
    already fetching that message or if we already had it in cache. We'd
    issue a FETCH command for it, and the barf when the server complied,
    because when the UID list wasn't sorted we wouldn't find the offending
    uid when we looked for it.
    
    The simple "fix" for this is to keep a boolean flag 'scan_changes' which
    is TRUE only when that FETCH (UID FLAGS) command is running. If a flags
    change comes in at any other time, it is definitely unsolicited and
    should *not* be added to the uidset. This at least protects us from
    having UIDs added after we've sorted the list and started to do other
    things with it, which was causing the crash.
    
    In fact, this whole 'solicited' vs. 'unsolicited' thing is a design
    mistake. In imapx_untagged() we should never care about what we asked
    for and what we didn't. That's why the responses are *untagged*. The
    server tells us things about the state of the mailboxes, and we should
    process that information into our own local cache â it shouldn't
    *matter* what we asked for. But that's a more intrusive fix for another day.
    
    In addition, we were reliably *triggering* this behaviour in some cases
    because we had to issue a SELECT for the folder in question before
    issuing the FETCH (UID FLAGS) command. And on completion of the SELECT,
    if UIDNEXT had increased, we were automatically issuing a *new* FETCH
    (UID FLAGS) command starting from the last-known-uid in our cache. This
    was entirely gratiutous, so use the same scan_changes boolean flag to
    avoid it in that situation.

 camel/camel-imapx-server.c |   42 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 6 deletions(-)
---
diff --git a/camel/camel-imapx-server.c b/camel/camel-imapx-server.c
index d66e81c..9f65e49 100644
--- a/camel/camel-imapx-server.c
+++ b/camel/camel-imapx-server.c
@@ -105,6 +105,7 @@ struct _RefreshInfoData {
 	gint fetch_msg_limit;
 	CamelFetchType fetch_type;
 	gboolean update_unseen;
+	gboolean scan_changes;
 	struct _uidset_state uidset;
 	/* changes during refresh */
 	CamelFolderChangeInfo *changes;
@@ -1266,17 +1267,19 @@ imapx_untagged (CamelIMAPXServer *is,
 
 		if ((finfo->got & FETCH_FLAGS) && !(finfo->got & FETCH_HEADER)) {
 			CamelIMAPXJob *job = imapx_match_active_job (is, IMAPX_JOB_FETCH_NEW_MESSAGES | IMAPX_JOB_REFRESH_INFO | IMAPX_JOB_FETCH_MESSAGES, NULL);
+			RefreshInfoData *data = NULL;
+
+			if (job) {
+				data = camel_imapx_job_get_data (job);
+				g_return_val_if_fail (data != NULL, FALSE);
+			}
+
 			/* This is either a refresh_info job, check to see if it is and update
 			 * if so, otherwise it must've been an unsolicited response, so update
 			 * the summary to match */
-
-			if (job && (finfo->got & FETCH_UID)) {
-				RefreshInfoData *data;
+			if (data && (finfo->got & FETCH_UID) && data->scan_changes) {
 				struct _refresh_info r;
 
-				data = camel_imapx_job_get_data (job);
-				g_return_val_if_fail (data != NULL, FALSE);
-
 				r.uid = finfo->uid;
 				finfo->uid = NULL;
 				r.server_flags = finfo->flags;
@@ -2458,10 +2461,25 @@ imapx_command_select_done (CamelIMAPXServer *is,
 		ifolder->exists_on_server = is->exists;
 		ifolder->modseq_on_server = is->highestmodseq;
 		if (ifolder->uidnext_on_server < is->uidnext) {
+			/* We don't want to fetch new messages if the command we selected this
+			   folder for is *already* fetching all messages (i.e. scan_changes).
+			   Bug #667725. */
+			CamelIMAPXJob *job = imapx_is_job_in_queue (is, is->select_pending,
+								    IMAPX_JOB_REFRESH_INFO, NULL);
+			if (job) {
+				RefreshInfoData *data = camel_imapx_job_get_data (job);
+
+				if (data->scan_changes) {
+					c(is->tagprefix, "Will not fetch_new_messages when already in scan_changes\n");
+					goto no_fetch_new;
+				}
+			}
 			imapx_server_fetch_new_messages (is, is->select_pending, TRUE, TRUE, NULL, NULL);
 			/* We don't do this right now because we want the new messages to
 			 * update the unseen count. */
 			//ifolder->uidnext_on_server = is->uidnext;
+		no_fetch_new:
+			;
 		}
 		ifolder->uidvalidity_on_server = is->uidvalidity;
 		selected_folder = camel_folder_get_full_name (is->select_folder);
@@ -3701,6 +3719,8 @@ imapx_command_step_fetch_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	ifolder = (CamelIMAPXFolder *) job->folder;
 	isum = (CamelIMAPXSummary *) job->folder->summary;
 
@@ -3851,6 +3871,8 @@ imapx_job_scan_changes_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	service = CAMEL_SERVICE (is->store);
 	settings = camel_service_get_settings (service);
 
@@ -4054,6 +4076,8 @@ imapx_job_scan_changes_start (CamelIMAPXJob *job,
 		"UID FETCH %s:* (UID FLAGS)", uid ? uid : "1");
 	camel_imapx_command_set_job (ic, job);
 	ic->complete = imapx_job_scan_changes_done;
+
+	data->scan_changes = TRUE;
 	ic->pri = job->pri;
 	data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 	imapx_command_queue (is, ic);
@@ -4133,6 +4157,8 @@ imapx_command_fetch_new_uids_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	qsort (
 		data->infos->data,
 		data->infos->len,
@@ -4197,6 +4223,8 @@ imapx_job_fetch_new_messages_start (CamelIMAPXJob *job,
 		data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 		ic->pri = job->pri;
 
+		data->scan_changes = TRUE;
+
 		if (fetch_order == CAMEL_SORT_DESCENDING)
 			ic->complete = imapx_command_fetch_new_uids_done;
 		else
@@ -4298,6 +4326,8 @@ imapx_job_fetch_messages_start (CamelIMAPXJob *job,
 		data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 		ic->pri = job->pri;
 
+		data->scan_changes = TRUE;
+
 		if (fetch_order == CAMEL_SORT_DESCENDING)
 			ic->complete = imapx_command_fetch_new_uids_done;
 		else