Commits

Yuya Nishihara committed 274d74b

update comment, etc.

Comments (0)

Files changed (1)

dlgkeep-next.diff

 # HG changeset patch
 # Parent 01b0f961500bf8a4f79d0b9d5cf8af2d8753b513
+dialogkeeper: avoid crash caused by deletion inside finished signal
+
+MergeDialog segfaults on close by "Finish" button, because it can be garbage-
+collected inside finished signal. MergeDialog doesn't override accept(), so it
+isn't referenced by Python world once DialogKeeper removes it form the list.
+
+DialogKeeper shouldn't change the refcount of the dialog inside signal handler.
 
 diff --git a/tortoisehg/hgqt/qtlib.py b/tortoisehg/hgqt/qtlib.py
 --- a/tortoisehg/hgqt/qtlib.py
 +++ b/tortoisehg/hgqt/qtlib.py
-@@ -1197,6 +1197,20 @@ class DialogKeeper(QObject):
+@@ -1197,6 +1197,33 @@ class DialogKeeper(QObject):
  
          self._dialogs = DialogKeeper(self._createDialog)
          self._dialogs = DialogKeeper(lambda *args: Foo(self))
 +
-+    XXX description here!
++    When to delete reference:
++
++    If a dialog is not referenced, and if accept(), reject() and done() are
++    not overridden, it could be garbage-collected during finished signal and
++    lead to hard crash::
++
++        #0  isSignalConnected (signal_index=..., this=0x0)
++        #1  QMetaObject::activate (...)
++        #2  ... in sipQDialog::done (this=0x1d655b0, ...)
++        #3  ... in sipQDialog::reject (this=0x1d655b0)
++        #4  ... in QDialog::closeEvent (this=this@entry=0x1d655b0, ...)
++
++    To avoid crash, a finished dialog is referenced explicitly by DialogKeeper
++    until next event processing.
 +
 +    >>> dialogs = DialogKeeper(QDialog)
 +    >>> dlgref = weakref.ref(dialogs.open())
 +    >>> dialogs.count()
 +    1
 +    >>> esckeyev = QKeyEvent(QEvent.KeyPress, Qt.Key_Escape, Qt.NoModifier)
-+    >>> QApplication.postEvent(dlgref(), esckeyev)
-+    >>> QApplication.processEvents()
++    >>> QApplication.postEvent(dlgref(), esckeyev)  # close without reference
++    >>> QApplication.processEvents()  # should not crash
 +    >>> dialogs.count()
 +    0
-+    >>> dlgref() is None
++    >>> dlgref() is None  # nobody should have reference
 +    True
      """
  
      def __init__(self, createdlg, genkey=None, parent=None):
-@@ -1206,6 +1220,10 @@ class DialogKeeper(QObject):
+@@ -1206,6 +1233,10 @@ class DialogKeeper(QObject):
          self._keytodlgs = {}  # key: [dlg, ...]
          self._dlgtokey = {}   # dlg: key
  
 +        self._garbagedlgs = []
-+        self._disposedlgslater = QTimer(self, interval=0, singleShot=True)
-+        self._disposedlgslater.timeout.connect(self._disposedlgs)
++        self._emptygarbagelater = QTimer(self, interval=0, singleShot=True)
++        self._emptygarbagelater.timeout.connect(self._emptygarbage)
 +
      def open(self, *args, **kwargs):
          """Create new dialog or reactivate existing dialog"""
          dlg = self._preparedlg(self._genkey(self.parent(), *args, **kwargs),
-@@ -1251,6 +1269,14 @@ class DialogKeeper(QObject):
+@@ -1251,6 +1282,14 @@ class DialogKeeper(QObject):
          if not self._keytodlgs[key]:
              del self._keytodlgs[key]
  
-+        # avoid deletion inside finished signal, which leads to crash
++        # avoid deletion inside finished signal
 +        self._garbagedlgs.append(dlg)
-+        self._disposedlgslater.start()
++        self._emptygarbagelater.start()
 +
 +    @pyqtSlot()
-+    def _disposedlgs(self):
++    def _emptygarbage(self):
 +        del self._garbagedlgs[:]
 +
      def count(self):