Fix bug #5189: Wireshark crashes when cancelling a large sort operation.

Essentially: Don't try to sort if the pre-requisite "columnization" step is stopped
             via the progressbar dialog window before the step completes.
Also: Fix a (very) minor bug wherein the sort-indicator not always cleared on the
      "previous column" when "No Sorting" was selected from a column context menu.
Also: Do minor code, comments & whitespace cleanup.

svn path=/trunk/; revision=36180

3 files changed, 129 insertions, 90 deletions

diff --git a/gtk/new_packet_list.c b/gtk/new_packet_list.c
index 80c12133d0..ea2b24f0af 100644
--- a/gtk/new_packet_list.c
+++ b/gtk/new_packet_list.c
@@ -356,7 +356,7 @@ col_details_edit_dlg (gint col_id, GtkTreeViewColumn *col)
 	gtk_misc_set_alignment(GTK_MISC(label), 1.0f, 0.5f);
 	gtk_tooltips_set_tip (tooltips, label,
 			      "Select which packet information to present in the column.", NULL);
- 
+
 	format_cmb = gtk_combo_box_new_text();
 	for (i = 0; i < NUM_COL_FMTS; i++) {
 	  gtk_combo_box_append_text(GTK_COMBO_BOX(format_cmb), col_format_desc(i));
@@ -412,7 +412,7 @@ col_details_edit_dlg (gint col_id, GtkTreeViewColumn *col)
 	g_signal_connect(ok_bt, "clicked", G_CALLBACK(col_title_change_ok), win);
 
 	cur_fmt = get_column_format (col_id);
-        gtk_combo_box_set_active(GTK_COMBO_BOX(format_cmb), cur_fmt);
+	gtk_combo_box_set_active(GTK_COMBO_BOX(format_cmb), cur_fmt);
 	if (cur_fmt == COL_CUSTOM) {
 		gtk_entry_set_text(GTK_ENTRY(field_te), get_column_custom_field(col_id));
 		g_snprintf(custom_occurrence_str, sizeof(custom_occurrence_str), "%d", get_column_custom_occurrence(col_id));
@@ -431,19 +431,37 @@ col_details_edit_dlg (gint col_id, GtkTreeViewColumn *col)
 	gtk_widget_show_all(win);
 }
 
+/* Process sort request;
+ *   order:          GTK_SORT_ASCENDING or GTK_SORT_DESCENDING
+ *   sort_indicator: TRUE: set sort_indicator on column; FALSE: don't set ....
+ *
+ * If necessary, columns are first "columnized" for all rows in the packet-list; If this
+ *  is not completed (i.e., stopped), then the sort request is aborted.
+ */
 static void
-new_packet_list_sort_column (gint col_id, GtkTreeViewColumn *col, GtkSortType order)
+new_packet_list_sort_column (gint col_id, GtkTreeViewColumn *col, GtkSortType order, gboolean sort_indicator)
 {
-	GtkTreeViewColumn *prev_col = (GtkTreeViewColumn *)
+	GtkTreeViewColumn *prev_col;
+
+	if (col == NULL) {
+		col = gtk_tree_view_get_column(GTK_TREE_VIEW(packetlist->view), col_id);
+	}
+	g_assert(col);
+
+	if (!packet_list_do_packet_list_dissect_and_cache_all(packetlist, col_id)) {
+		return;  /* "stopped": do not try to sort */
+	}
+
+	prev_col = (GtkTreeViewColumn *)
 	  g_object_get_data(G_OBJECT(packetlist->view), E_MPACKET_LIST_PREV_COLUMN_KEY);
 
 	if (prev_col) {
 		gtk_tree_view_column_set_sort_indicator(prev_col, FALSE);
 	}
-	gtk_tree_view_column_set_sort_indicator(col, TRUE);
+	gtk_tree_view_column_set_sort_indicator(col, sort_indicator);
 	gtk_tree_view_column_set_sort_order (col, order);
 	g_object_set_data(G_OBJECT(packetlist->view), E_MPACKET_LIST_PREV_COLUMN_KEY, col);
-	gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(packetlist), col_id, order);
+	gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(packetlist), col_id, order); /* triggers sort callback */
 
 	scroll_to_current ();
 }
@@ -462,13 +480,11 @@ new_packet_list_column_clicked_cb (GtkTreeViewColumn *col, gpointer user_data _U
 		return;
 
 	if (!gtk_tree_view_column_get_sort_indicator(col)) {
-		new_packet_list_sort_column (col_id, col, GTK_SORT_ASCENDING);
+		new_packet_list_sort_column (col_id, col, GTK_SORT_ASCENDING, TRUE);
 	} else if (order == GTK_SORT_ASCENDING) {
-		new_packet_list_sort_column (col_id, col, GTK_SORT_DESCENDING);
+		new_packet_list_sort_column (col_id, col, GTK_SORT_DESCENDING, TRUE);
 	} else {
-		gtk_tree_view_column_set_sort_indicator(col, FALSE);
-		gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(packetlist), 0, GTK_SORT_ASCENDING);
-		scroll_to_current ();
+		new_packet_list_sort_column (0, NULL, GTK_SORT_ASCENDING, FALSE);
 	}
 }
 
@@ -610,15 +626,13 @@ new_packet_list_column_menu_cb (GtkWidget *w, gpointer user_data _U_, COLUMN_SEL
 
 	switch (action) {
 	case COLUMN_SELECTED_SORT_ASCENDING:
-		new_packet_list_sort_column (col_id, col, GTK_SORT_ASCENDING);
+		new_packet_list_sort_column (col_id, col, GTK_SORT_ASCENDING, TRUE);
 		break;
 	case COLUMN_SELECTED_SORT_DESCENDING:
-		new_packet_list_sort_column (col_id, col, GTK_SORT_DESCENDING);
+		new_packet_list_sort_column (col_id, col, GTK_SORT_DESCENDING, TRUE);
 		break;
 	case COLUMN_SELECTED_SORT_NONE:
-		gtk_tree_view_column_set_sort_indicator(col, FALSE);
-		gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(packetlist), 0, GTK_SORT_ASCENDING);
-		scroll_to_current ();
+		new_packet_list_sort_column (0, NULL, GTK_SORT_ASCENDING, FALSE);
 		break;
 	case COLUMN_SELECTED_TOGGLE_RESOLVED:
 		new_packet_list_toggle_resolved (w, col_id);
@@ -819,7 +833,7 @@ create_view_and_model(void)
 		}
 
 		gtk_tree_view_append_column(GTK_TREE_VIEW(packetlist->view), col);
-		
+
 		/* XXX Breaks the GTK+ API, but this is the only way to attach a signal to
 		 * a GtkTreeView column header. See GTK bug #141937.
 		 */
@@ -864,9 +878,7 @@ new_packet_list_clear(void)
 	/* XXX is this correct in all cases?
 	 * Reset the sort column, use packetlist as model in case the list is frozen.
 	 */
-	gtk_tree_sortable_set_sort_column_id(GTK_TREE_SORTABLE(packetlist),
-			0, GTK_SORT_ASCENDING);
-
+	new_packet_list_sort_column(0, NULL, GTK_SORT_ASCENDING, FALSE);
 }
 
 void
@@ -1023,8 +1035,8 @@ scroll_to_and_select_iter(GtkTreeModel *model, GtkTreeSelection *selection, GtkT
 			0.5,	/* row_align determines where the row is placed, 0.5 means center */
 			0); 	/* The horizontal alignment of the column */
 
-        /* "cursor-changed" signal triggers new_packet_list_select_cb() */
-        /*  which will update the middle and bottom panes.              */
+	/* "cursor-changed" signal triggers new_packet_list_select_cb() */
+	/*  which will update the middle and bottom panes.              */
 	gtk_tree_view_set_cursor(GTK_TREE_VIEW(packetlist->view),
 			path,
 			NULL,
@@ -1170,8 +1182,8 @@ new_packet_list_set_selected_row(gint row)
 	selection = gtk_tree_view_get_selection(GTK_TREE_VIEW(packetlist->view));
 	gtk_tree_selection_select_iter (selection, &iter);
 
-        /* "cursor-changed" signal triggers new_packet_list_select_cb() */
-        /*  which will update the middle and bottom panes.              */
+	/* "cursor-changed" signal triggers new_packet_list_select_cb() */
+	/*  which will update the middle and bottom panes.              */
 	gtk_tree_view_set_cursor(GTK_TREE_VIEW(packetlist->view),
 			path,
 			NULL,
@@ -1224,12 +1236,12 @@ new_packet_list_select_cb(GtkTreeView *tree_view, gpointer data _U_)
 
 	cf_select_packet(&cfile, row);
 	/* If searching the tree, set the focus there; otherwise, focus on the packet list */
-	if (cfile.search_in_progress && (cfile.decode_data || cfile.decode_data)) { 
+	if (cfile.search_in_progress && (cfile.decode_data || cfile.decode_data)) {
 		gtk_widget_grab_focus(tree_view_gbl);
 	} else {
 		gtk_widget_grab_focus(packetlist->view);
 	}
-	
+
 	/* Add newly selected frame to packet history (breadcrumbs) */
 	packet_history_add(row);
 }
@@ -1485,14 +1497,14 @@ mark_all_displayed_frames(gboolean set)
 void
 new_packet_list_mark_all_displayed_frames_cb(GtkWidget *w _U_, gpointer data _U_)
 {
-        mark_all_displayed_frames(TRUE);
-        mark_frames_ready();
+	mark_all_displayed_frames(TRUE);
+	mark_frames_ready();
 }
 
 void
 new_packet_list_unmark_all_displayed_frames_cb(GtkWidget *w _U_, gpointer data _U_)
 {
-        mark_all_displayed_frames(FALSE);
+	mark_all_displayed_frames(FALSE);
 	mark_frames_ready();
 }
 
diff --git a/gtk/packet_list_store.c b/gtk/packet_list_store.c
index d465b487f8..2bb4abe1a3 100644
--- a/gtk/packet_list_store.c
+++ b/gtk/packet_list_store.c
@@ -629,9 +629,9 @@ packet_list_append_record(PacketList *packet_list, frame_data *fdata)
 	g_return_val_if_fail(PACKETLIST_IS_LIST(packet_list), -1);
 
 	newrecord = se_alloc(sizeof(PacketListRecord));
-	newrecord->columnized = FALSE;
-	newrecord->colorized = FALSE;
-	newrecord->fdata = fdata;
+	newrecord->columnized   = FALSE;
+	newrecord->colorized    = FALSE;
+	newrecord->fdata        = fdata;
 	newrecord->physical_pos = PACKET_LIST_RECORD_COUNT(packet_list->physical_rows);
 
 	if (fdata->flags.passed_dfilter || fdata->flags.ref_time) {
@@ -643,14 +643,11 @@ packet_list_append_record(PacketList *packet_list, frame_data *fdata)
 
 	PACKET_LIST_RECORD_APPEND(packet_list->physical_rows, newrecord);
 
-	if (packet_list->columnized) {
-		/* XXX, dissect? */
-		packet_list->columnized = FALSE;
-	}
+	packet_list->columnized = FALSE;   /* XXX, dissect? */
 
-	/* 
+	/*
 	 * Issue a row_inserted signal if the model is connected
-	 * and the row is vissible.
+	 * and the row is visible.
 	 */
 	if((model)&&(newrecord->visible_pos!=-1))
 		packet_list_row_inserted(packet_list, newrecord->visible_pos);
@@ -776,7 +773,15 @@ packet_list_column_contains_values(PacketList *packet_list, gint sort_col_id)
 		return FALSE;
 }
 
-static void
+/* packet_list_dissect_and_cache_all()
+ *  returns:
+ *   TRUE   if columnization completed;
+ *            packet_list->columnized set to TRUE;
+ *   FALSE: columnization did not complete (i.e., was stopped by the user);
+ *            packet_list->columnized unchanged (i.e., FALSE).
+ */
+
+static gboolean
 packet_list_dissect_and_cache_all(PacketList *packet_list)
 {
 	PacketListRecord *record;
@@ -809,47 +814,70 @@ packet_list_dissect_and_cache_all(PacketList *packet_list)
 	main_window_update();
 
 	for (progbar_loop_var = 0; progbar_loop_var < progbar_loop_max; ++progbar_loop_var) {
-	  record = PACKET_LIST_RECORD_GET(packet_list->physical_rows, progbar_loop_var);
-	  packet_list_dissect_and_cache_record(packet_list, record, TRUE, FALSE);
-
-	  /* Create the progress bar if necessary.
-		 We check on every iteration of the loop, so that it takes no
-		 longer than the standard time to create it (otherwise, for a
-		 large file, we might take considerably longer than that standard
-		 time in order to get to the next progress bar step). */
-	  if (progbar == NULL)
-		 progbar = delayed_create_progress_dlg("Construct", "Columns",
-						TRUE, &progbar_stop_flag,
-						&progbar_start_time, progbar_val);
-
-	  if (progbar_loop_var >= progbar_nextstep) {
-		/* let's not divide by zero. We should never be started
-		 * with count == 0, so let's assert that */
-		g_assert(progbar_loop_max > 0);
-
-		progbar_val = (gfloat) progbar_loop_var / progbar_loop_max;
-
-		if (progbar != NULL) {
-		  g_snprintf(progbar_status_str, sizeof(progbar_status_str),
-					 "%u of %u frames", progbar_loop_var+1, progbar_loop_max);
-		  update_progress_dlg(progbar, progbar_val, progbar_status_str);
-		}
+		record = PACKET_LIST_RECORD_GET(packet_list->physical_rows, progbar_loop_var);
+		packet_list_dissect_and_cache_record(packet_list, record, TRUE, FALSE);
+
+		/* Create the progress bar if necessary.
+		   We check on every iteration of the loop, so that it takes no
+		   longer than the standard time to create it (otherwise, for a
+		   large file, we might take considerably longer than that standard
+		   time in order to get to the next progress bar step). */
+		if (progbar == NULL)
+			/* Note: The following may call gtk_main_iteration() which will */
+			/*       allow certain "interupts" to happen during this code.  */
+			/*       (Note that the progress_dlg window is set to "modal"   */
+			/*        so that clicking on other windows is disabled).       */
+			progbar = delayed_create_progress_dlg("Construct", "Columns",
+							      TRUE, &progbar_stop_flag,
+							      &progbar_start_time, progbar_val);
+
+		if (progbar_loop_var >= progbar_nextstep) {
+			/* let's not divide by zero. We should never be started
+			 * with count == 0, so let's assert that */
+			g_assert(progbar_loop_max > 0);
+
+			progbar_val = (gfloat) progbar_loop_var / progbar_loop_max;
+
+			if (progbar != NULL) {
+				g_snprintf(progbar_status_str, sizeof(progbar_status_str),
+					   "%u of %u frames", progbar_loop_var+1, progbar_loop_max);
+				/* Note: See comment above re use of gtk_main_iteration() */
+				update_progress_dlg(progbar, progbar_val, progbar_status_str);
+			}
 
-		progbar_nextstep += progbar_quantum;
-	  }
+			progbar_nextstep += progbar_quantum;
+		}
 
-	  if (progbar_stop_flag) {
-		/* Well, the user decided to abort the resizing... */
-		break;
-	  }
+		if (progbar_stop_flag) {
+			/* Well, the user decided to abort ... */
+			break;
+		}
 	}
 
-	/* We're done resizing the columns; destroy the progress bar if it
-	   was created. */
+	/* We're done; destroy the progress bar if it was created. */
 	if (progbar != NULL)
-	  destroy_progress_dlg(progbar);
+		destroy_progress_dlg(progbar);
+
+	if (progbar_stop_flag) {
+		return FALSE; /* user aborted before columnization completed */
+	}
 
 	packet_list->columnized = TRUE;
+	return TRUE;
+}
+
+/* packet_list_do_packet_list_dissect_and_cache_all()
+ *  returns:
+ *    TRUE:  if columnization not needed or columnization completed;
+ *    FALSE: columnization did not complete (i.e., stopped by the user)
+ */
+gboolean
+packet_list_do_packet_list_dissect_and_cache_all(PacketList *packet_list, gint sort_col_id)
+{
+	if (!packet_list_column_contains_values(packet_list, sort_col_id)) {
+		return packet_list_dissect_and_cache_all(packet_list);
+	}
+	return TRUE;
 }
 
 static void
@@ -874,9 +902,6 @@ packet_list_sortable_set_sort_column_id(GtkTreeSortable *sortable,
 	if(PACKET_LIST_RECORD_COUNT(packet_list->physical_rows) == 0)
 		return;
 
-	if (!packet_list_column_contains_values(packet_list, sort_col_id))
-		packet_list_dissect_and_cache_all(packet_list);
-
 	packet_list_resort(packet_list);
 
 	/* emit "sort-column-changed" signal to tell any tree views
@@ -933,7 +958,7 @@ packet_list_compare_custom(gint sort_id, PacketListRecord *a, PacketListRecord *
 		/* Attempt to convert to numbers */
 		double num_a = atof(a->fdata->col_text[sort_id]);
 		double num_b = atof(b->fdata->col_text[sort_id]);
-		
+
 		if (num_a < num_b)
 			return -1;
 		else if (num_a > num_b)
@@ -958,17 +983,12 @@ packet_list_compare_records(gint sort_id, PacketListRecord *a,
 	g_assert(b->fdata->col_text[sort_id]);
 
 	if(a->fdata->col_text[sort_id] == b->fdata->col_text[sort_id])
-		return 0; /* not always NULL, but anyway no need to call strcmp() */
-
-	if((a->fdata->col_text[sort_id]) && (b->fdata->col_text[sort_id])) {
-		if (cfile.cinfo.col_fmt[sort_id] == COL_CUSTOM) {
-			return packet_list_compare_custom (sort_id, a, b);
-		}
-		return strcmp(a->fdata->col_text[sort_id], b->fdata->col_text[sort_id]);
-	} else
-		return (a->fdata->col_text[sort_id] == NULL) ? -1 : 1;
+		return 0; /* no need to call strcmp() */
 
-	g_assert_not_reached();
+	if (cfile.cinfo.col_fmt[sort_id] == COL_CUSTOM) {
+		return packet_list_compare_custom (sort_id, a, b);
+	}
+	return strcmp(a->fdata->col_text[sort_id], b->fdata->col_text[sort_id]);
 }
 
 static gint
@@ -1091,7 +1111,13 @@ packet_list_dissect_and_cache_record(PacketList *packet_list, PacketListRecord *
 	gboolean create_proto_tree;
 	union wtap_pseudo_header pseudo_header; /* Packet pseudo_header */
 	guint8 pd[WTAP_MAX_PACKET_SIZE];  /* Packet data */
-    
+
+	/* XXX: Does it work to check if the record is already columnized/colorized ?
+	 *      i.e.: test record->columnized and record->colorized and just return
+	 *            if they're both TRUE.
+	 *      Note: Part of the patch submitted with Bug #4273 had code to do this but it
+	 *            was commented out in the patch and was not included in SVN #33011.
+	 */
 	fdata = record->fdata;
 
 	if (dissect_columns)
@@ -1121,7 +1147,7 @@ packet_list_dissect_and_cache_record(PacketList *packet_list, PacketListRecord *
 
 	if (dissect_columns) {
 		/* "Stringify" non frame_data vals */
-		epan_dissect_fill_in_columns(&edt, FALSE, FALSE /* fill_fd_colums */);
+		epan_dissect_fill_in_columns(&edt, FALSE, FALSE /* fill_fd_columns */);
 
 		for(col = 0; col < cinfo->num_cols; ++col) {
 			/* Skip columns based om frame_data because we already store those. */
@@ -1264,7 +1290,7 @@ packet_list_get_widest_column_string(PacketList *packet_list, gint col)
 		guint widest_column_len = 0;
 
 		if (!packet_list->columnized)
-			packet_list_dissect_and_cache_all(packet_list);
+			packet_list_dissect_and_cache_all(packet_list); /* XXX: need to handle case of "incomplete" ? */
 
 		for(vis_idx = 0; vis_idx < PACKET_LIST_RECORD_COUNT(packet_list->visible_rows); ++vis_idx) {
 			record = PACKET_LIST_RECORD_GET(packet_list->visible_rows, vis_idx);
diff --git a/gtk/packet_list_store.h b/gtk/packet_list_store.h
index 2780fb2348..0651a03049 100644
--- a/gtk/packet_list_store.h
+++ b/gtk/packet_list_store.h
@@ -116,6 +116,7 @@ gboolean packet_list_visible_record(PacketList *packet_list, GtkTreeIter *iter);
 gint packet_list_append_record(PacketList *packet_list, frame_data *fdata);
 void packet_list_change_record(PacketList *packet_list, guint row, gint col, column_info *cinfo);
 void packet_list_dissect_and_cache_iter(PacketList *packet_list, GtkTreeIter *iter, gboolean dissect_columns, gboolean dissect_color);
+gboolean packet_list_do_packet_list_dissect_and_cache_all(PacketList *packet_list, gint sort_col_id);
 void packet_list_reset_colorized(PacketList *packet_list);
 const char* packet_list_get_widest_column_string(PacketList *packet_list, gint col);