More retapping fixups.

Disable the main file close and reload actions while we're retapping,
otherwise many of our dialogs will crash.

Disable the TapParameterDialog filter entry while we're retapping. This
keeps us from enabling the "Apply" button when we shouldn't.

Don't prematurely disconnect our signals in WiresharkDialog.

Change-Id: Iaf507eb4503b9c296766f109f2b8c71343263982
Reviewed-on: https://code.wireshark.org/review/10274
Petri-Dish: Gerald Combs <gerald@wireshark.org>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Gerald Combs <gerald@wireshark.org>

8 files changed, 56 insertions, 23 deletions

diff --git a/ui/qt/capture_file.h b/ui/qt/capture_file.h
index d31a9868f8..2edbf1c939 100644
--- a/ui/qt/capture_file.h
+++ b/ui/qt/capture_file.h
@@ -68,7 +68,7 @@ public:
      */
     const QString fileName();
 
-    /** Re;load the capture file
+    /** Reload the capture file
      */
     void reload();
 
diff --git a/ui/qt/main_window.cpp b/ui/qt/main_window.cpp
index b5483ce34c..905cffe833 100644
--- a/ui/qt/main_window.cpp
+++ b/ui/qt/main_window.cpp
@@ -392,6 +392,10 @@ MainWindow::MainWindow(QWidget *parent) :
             this, SLOT(captureFileRescanStarted()));
     connect(&capture_file_, SIGNAL(captureFileRescanFinished()),
             this, SLOT(captureFileReadFinished()));
+    connect(&capture_file_, SIGNAL(captureFileRetapStarted()),
+            this, SLOT(captureFileRetapStarted()));
+    connect(&capture_file_, SIGNAL(captureFileRetapFinished()),
+            this, SLOT(captureFileRetapFinished()));
     connect(&capture_file_, SIGNAL(captureFileClosing()),
             this, SLOT(captureFileClosing()));
     connect(&capture_file_, SIGNAL(captureFileClosed()),
diff --git a/ui/qt/main_window.h b/ui/qt/main_window.h
index b69cbb8a29..40e1233e5e 100644
--- a/ui/qt/main_window.h
+++ b/ui/qt/main_window.h
@@ -235,6 +235,8 @@ public slots:
     void captureFileReadFinished();
     void captureFileReloadStarted() { captureFileReadStarted(tr("Reloading")); }
     void captureFileRescanStarted() { captureFileReadStarted(tr("Rescanning")); }
+    void captureFileRetapStarted();
+    void captureFileRetapFinished();
     void captureFileClosing();
     void captureFileClosed();
     void captureFileSaveStarted(const QString &file_path);
diff --git a/ui/qt/main_window_slots.cpp b/ui/qt/main_window_slots.cpp
index 599edc738d..46f2c03439 100644
--- a/ui/qt/main_window_slots.cpp
+++ b/ui/qt/main_window_slots.cpp
@@ -677,6 +677,19 @@ void MainWindow::captureFileReadFinished() {
     emit setDissectedCaptureFile(capture_file_.capFile());
 }
 
+void MainWindow::captureFileRetapStarted()
+{
+    // XXX Push a status message?
+    main_ui_->actionFileClose->setEnabled(false);
+    main_ui_->actionViewReload->setEnabled(false);
+}
+
+void MainWindow::captureFileRetapFinished()
+{
+    main_ui_->actionFileClose->setEnabled(true);
+    main_ui_->actionViewReload->setEnabled(true);
+}
+
 void MainWindow::captureFileClosing() {
     setMenusForCaptureFile(true);
     setForCapturedPackets(false);
diff --git a/ui/qt/tap_parameter_dialog.cpp b/ui/qt/tap_parameter_dialog.cpp
index 93e9ec223d..e03858e413 100644
--- a/ui/qt/tap_parameter_dialog.cpp
+++ b/ui/qt/tap_parameter_dialog.cpp
@@ -197,7 +197,7 @@ void TapParameterDialog::setRetapOnShow(bool retap)
 {
     show_timer_->stop();
     if (retap) {
-        show_timer_->singleShot(0, this, SLOT(fillTreeWrapper()));
+        show_timer_->singleShot(0, this, SLOT(on_applyFilterButton_clicked()));
     }
 }
 
@@ -429,13 +429,6 @@ QByteArray TapParameterDialog::getTreeAsString(st_format_type format)
     return ba;
 }
 
-void TapParameterDialog::fillTreeWrapper()
-{
-    ui->applyFilterButton->setEnabled(false);
-    fillTree();
-    ui->applyFilterButton->setEnabled(true);
-}
-
 void TapParameterDialog::drawTreeItems()
 {
     if (ui->statsTreeWidget->model()->rowCount() < expand_all_threshold_) {
@@ -523,12 +516,24 @@ void TapParameterDialog::updateWidgets()
 
 void TapParameterDialog::on_applyFilterButton_clicked()
 {
+    beginRetapPackets();
     if (!ui->displayFilterLineEdit->checkFilter())
         return;
 
     QString filter = ui->displayFilterLineEdit->text();
-    emit updateFilter(filter, true);
-    fillTreeWrapper();
+    emit updateFilter(filter, false);
+    // If we wanted to be fancy we could add an isRetapping function to
+    // either WiresharkDialog or CaptureFile and use it in updateWidgets
+    // to enable and disable the apply button as needed.
+    // For now we use more simple but less useful logic.
+    bool df_enabled = ui->displayFilterLineEdit->isEnabled();
+    bool af_enabled = ui->applyFilterButton->isEnabled();
+    ui->displayFilterLineEdit->setEnabled(false);
+    ui->applyFilterButton->setEnabled(false);
+    fillTree();
+    ui->applyFilterButton->setEnabled(af_enabled);
+    ui->displayFilterLineEdit->setEnabled(df_enabled);
+    endRetapPackets();
 }
 
 void TapParameterDialog::on_actionCopyToClipboard_triggered()
diff --git a/ui/qt/tap_parameter_dialog.h b/ui/qt/tap_parameter_dialog.h
index 55915d2c8d..d79a30d75c 100644
--- a/ui/qt/tap_parameter_dialog.h
+++ b/ui/qt/tap_parameter_dialog.h
@@ -110,7 +110,6 @@ private:
 private slots:
     // Called by the constructor. The subclass should tap packets here.
     virtual void fillTree() = 0;
-    void fillTreeWrapper();
 
     void on_applyFilterButton_clicked();
     void on_actionCopyToClipboard_triggered();
diff --git a/ui/qt/wireshark_dialog.cpp b/ui/qt/wireshark_dialog.cpp
index 8ddfddc1cd..ec9baa579f 100644
--- a/ui/qt/wireshark_dialog.cpp
+++ b/ui/qt/wireshark_dialog.cpp
@@ -37,6 +37,7 @@
 // To do:
 // - Use a dynamic property + Q_PROPERTY for the subtitle.
 // - Save and load recent geometry.
+// - Make our nested event loop more robust. See tryDeleteLater for details.
 
 WiresharkDialog::WiresharkDialog(QWidget &, CaptureFile &capture_file) :
     QDialog(NULL, Qt::Window),
@@ -49,7 +50,7 @@ WiresharkDialog::WiresharkDialog(QWidget &, CaptureFile &capture_file) :
     setWindowTitleFromSubtitle();
 
     connect(&cap_file_, SIGNAL(captureFileRetapStarted()), this, SLOT(beginRetapPackets()));
-    connect(&cap_file_, SIGNAL(captureFileRetapFinished()), this, SLOT(endsRetapPackets()));
+    connect(&cap_file_, SIGNAL(captureFileRetapFinished()), this, SLOT(endRetapPackets()));
     connect(&cap_file_, SIGNAL(captureFileClosing()), this, SLOT(captureFileClosing()));
     connect(&cap_file_, SIGNAL(captureFileClosed()), this, SLOT(captureFileClosing()));
 }
@@ -93,13 +94,17 @@ void WiresharkDialog::setWindowTitleFromSubtitle()
 // See if we can destroy ourselves. The user may have clicked "Close" while
 // we were deep in the bowels of a routine that retaps packets. Track our
 // tapping state using retap_depth_ and our closed state using dialog_closed_.
+//
+// The Delta Object Rules (http://delta.affinix.com/dor/) page on nested
+// event loops effectively says "don't do that." However, we don't really
+// have a choice if we want to have a usable application that retaps packets.
+
 void WiresharkDialog::tryDeleteLater()
 {
-    // In many of our subclasses, if the user clicks "Apply" followed by
-    // "Close" we can end up calling fillTree after calling tryDeleteLater.
-    // Disconnecting our slots here prevents that (in Qt5 at least).
-    disconnect();
-    if (retap_depth_ < 1 && dialog_closed_) deleteLater();
+    if (retap_depth_ < 1 && dialog_closed_) {
+        disconnect();
+        deleteLater();
+    }
 }
 
 void WiresharkDialog::updateWidgets()
diff --git a/ui/qt/wireshark_dialog.h b/ui/qt/wireshark_dialog.h
index d90be2104b..754e5302a6 100644
--- a/ui/qt/wireshark_dialog.h
+++ b/ui/qt/wireshark_dialog.h
@@ -26,13 +26,18 @@
  * @file General dialog base class
  *
  * Base class which provides convenience methods for dialogs that handle
- * capture files. "General" is a misnomer but we already have a class named
- * "CaptureFileDialog". Suggestions for a better name from
- * https://code.wireshark.org/review/#/c/9739/:
- * BaseCaptureDialog, CaptureHelperDialog (or rename CaptureFileDialog to something else - WiresharkFileDialog).
- * TapDialog might make sense as well.
+ * capture files.
+ *
+ * This class attempts to destroy itself when closed. Doing this safely and
+ * properly can be a bit tricky while scanning and tapping packets since
  */
 
+// "General" is a misnomer but we already have a class named
+// "CaptureFileDialog". Suggestions for a better name from
+// https://code.wireshark.org/review/#/c/9739/:
+// BaseCaptureDialog, CaptureHelperDialog (or rename CaptureFileDialog to something else - WiresharkFileDialog).
+// TapDialog might make sense as well.
+
 #include "capture_file.h"
 
 #include <QDialog>