Merge pull request spesmilo#10052 from SomberNight/202507_qt_crash_felix_2

crash reporter: rm `Never`, add confirm to `Send`, and only show window once per exc group

Author: SomberNight

electrum/base_crash_reporter.py

@@ -25,13 +25,15 @@
 import traceback
 import sys
 import queue
-from typing import TYPE_CHECKING, NamedTuple, Optional
+from typing import TYPE_CHECKING, NamedTuple, Optional, TypedDict
+from types import TracebackType
 
 from .version import ELECTRUM_VERSION
 from . import constants
 from .i18n import _
 from .util import make_aiohttp_session, error_text_str_to_safe_str
 from .logging import describe_os_version, Logger, get_git_version
+from .crypto import sha256
 
 if TYPE_CHECKING:
     from .network import ProxySettings
@@ -68,9 +70,16 @@ class BaseCrashReporter(Logger):
     USER_COMMENT_PLACEHOLDER = _("Do not enter sensitive/private information here. "
                                  "The report will be visible on the public issue tracker.")
 
-    def __init__(self, exctype, value, tb):
+    exc_args: tuple[type[BaseException], BaseException, TracebackType | None]
+
+    def __init__(
+        self,
+        exctype: type[BaseException],
+        excvalue: BaseException,
+        tb: TracebackType | None,
+    ):
         Logger.__init__(self)
-        self.exc_args = (exctype, value, tb)
+        self.exc_args = (exctype, excvalue, tb)
 
     def send_report(self, asyncio_loop, proxy: 'ProxySettings', *, timeout=None) -> CrashReportResponse:
         # FIXME the caller needs to catch generic "Exception", as this method does not have a well-defined API...
@@ -82,7 +91,7 @@ def send_report(self, asyncio_loop, proxy: 'ProxySettings', *, timeout=None) ->
         ] and ".electrum.org" in BaseCrashReporter.report_server):
             # Gah! Some kind of altcoin wants to send us crash reports.
             raise Exception(_("Missing report URL."))
-        report = self.get_traceback_info()
+        report = self.get_traceback_info(*self.exc_args)
         report.update(self.get_additional_info())
         report = json.dumps(report)
         coro = self.do_post(proxy, BaseCrashReporter.report_server + "/crash.json", data=report)
@@ -111,21 +120,38 @@ async def do_post(self, proxy: 'ProxySettings', url, data) -> str:
             async with session.post(url, data=data, raise_for_status=True) as resp:
                 return await resp.text()
 
-    def get_traceback_info(self):
-        exc_string = str(self.exc_args[1])
-        stack = traceback.extract_tb(self.exc_args[2])
-        readable_trace = self.__get_traceback_str_to_send()
-        id = {
+    @classmethod
+    def get_traceback_info(
+        cls,
+        exctype: type[BaseException],
+        excvalue: BaseException,
+        tb: TracebackType | None,
+    ) -> TypedDict('TBInfo', {'exc_string': str, 'stack': str, 'id': dict[str, str]}):
+        exc_string = str(excvalue)
+        stack = traceback.extract_tb(tb)
+        readable_trace = cls._get_traceback_str_to_send(exctype, excvalue, tb)
+        _id = {
             "file": stack[-1].filename if len(stack) else '<no stack>',
             "name": stack[-1].name if len(stack) else '<no stack>',
-            "type": self.exc_args[0].__name__
-        }
+            "type": exctype.__name__
+        }  # note: this is the "id" the crash reporter server uses to group together reports.
         return {
             "exc_string": exc_string,
             "stack": readable_trace,
-            "id": id
+            "id": _id,
         }
 
+    @classmethod
+    def get_traceback_groupid_hash(
+        cls,
+        exctype: type[BaseException],
+        excvalue: BaseException,
+        tb: TracebackType | None,
+    ) -> bytes:
+        tb_info = cls.get_traceback_info(exctype, excvalue, tb)
+        _id = tb_info["id"]
+        return sha256(str(_id))
+
     def get_additional_info(self):
         args = {
             "app_version": get_git_version() or ELECTRUM_VERSION,
@@ -142,15 +168,21 @@ def get_additional_info(self):
             pass
         return args
 
-    def __get_traceback_str_to_send(self) -> str:
+    @classmethod
+    def _get_traceback_str_to_send(
+        cls,
+        exctype: type[BaseException],
+        excvalue: BaseException,
+        tb: TracebackType | None,
+    ) -> str:
         # make sure that traceback sent to crash reporter contains
         # e.__context__ and e.__cause__, i.e. if there was a chain of
         # exceptions, we want the full traceback for the whole chain.
-        return "".join(traceback.format_exception(*self.exc_args))
+        return "".join(traceback.format_exception(exctype, excvalue, tb))
 
     def _get_traceback_str_to_display(self) -> str:
         # overridden in Qt subclass
-        return self.__get_traceback_str_to_send()
+        return self._get_traceback_str_to_send(*self.exc_args)
 
     def get_report_string(self):
         info = self.get_additional_info()

electrum/gui/qml/components/ExceptionDialog.qml

@@ -77,15 +77,16 @@ ElDialog
                 Layout.fillWidth: true
                 Layout.preferredWidth: 3
                 text: qsTr('Send Bug Report')
-                onClicked: AppController.sendReport(user_text.text)
-            }
-            Button {
-                Layout.fillWidth: true
-                Layout.preferredWidth: 2
-                text: qsTr('Never')
                 onClicked: {
-                    AppController.showNever()
-                    close()
+                    var dialog = app.messageDialog.createObject(app, {
+                        text: qsTr('Confirm to send bugreport?'),
+                        yesno: true,
+                        z: 1001  // assure topmost of all other dialogs
+                    })
+                    dialog.accepted.connect(function() {
+                        AppController.sendReport(user_text.text)
+                    })
+                    dialog.open()
                 }
             }
             Button {

electrum/gui/qml/qeapp.py

@@ -344,7 +344,7 @@ def isAndroid(self):
     @pyqtSlot(result='QVariantMap')
     def crashData(self):
         return {
-            'traceback': self.get_traceback_info(),
+            'traceback': self.get_traceback_info(*self.exc_args),
             'extra': self.get_additional_info(),
             'reportstring': self.get_report_string()
         }
@@ -379,10 +379,6 @@ def report_task():
         self.sendingBugreport.emit()
         threading.Thread(target=report_task, daemon=True).start()
 
-    @pyqtSlot()
-    def showNever(self):
-        self.config.SHOW_CRASH_REPORTER = False
-
     def _get_traceback_str_to_display(self) -> str:
         # The msg_box that shows the report uses rich_text=True, so
         # if traceback contains special HTML characters, e.g. '<',
@@ -541,6 +537,7 @@ def __init__(self, *, slot):
         Logger.__init__(self)
         assert self._INSTANCE is None, "Exception_Hook is supposed to be a singleton"
         self.wallet_types_seen = set()  # type: Set[str]
+        self.exception_ids_seen = set()  # type: Set[bytes]
 
         sys.excepthook = self.handler
         threading.excepthook = self.handler
@@ -551,14 +548,15 @@ def __init__(self, *, slot):
 
     @classmethod
     def maybe_setup(cls, *, wallet: 'Abstract_Wallet' = None, slot=None) -> None:
-        if not QEConfig.instance.config.SHOW_CRASH_REPORTER:
-            EarlyExceptionsQueue.set_hook_as_ready()  # flush already queued exceptions
-            return
         if not cls._INSTANCE:
             cls._INSTANCE = Exception_Hook(slot=slot)
         if wallet:
             cls._INSTANCE.wallet_types_seen.add(wallet.wallet_type)
 
     def handler(self, *exc_info):
         self.logger.error('exception caught by crash reporter', exc_info=exc_info)
+        groupid_hash = BaseCrashReporter.get_traceback_groupid_hash(*exc_info)
+        if groupid_hash in self.exception_ids_seen:
+            return  # to avoid annoying the user, only show crash reporter once per exception groupid
+        self.exception_ids_seen.add(groupid_hash)
         self._report_exception.emit(*exc_info)

electrum/gui/qt/exception_window.py

@@ -83,14 +83,10 @@ def __init__(self, config: 'SimpleConfig', exctype, value, tb):
         buttons = QHBoxLayout()
 
         report_button = QPushButton(_('Send Bug Report'))
-        report_button.clicked.connect(lambda _checked: self.send_report())
+        report_button.clicked.connect(lambda _checked: self._ask_for_confirm_to_send_report())
         report_button.setIcon(read_QIcon("tab_send.png"))
         buttons.addWidget(report_button)
 
-        never_button = QPushButton(_('Never'))
-        never_button.clicked.connect(lambda _checked: self.show_never())
-        buttons.addWidget(never_button)
-
         close_button = QPushButton(_('Not Now'))
         close_button.clicked.connect(lambda _checked: self.close())
         buttons.addWidget(close_button)
@@ -103,6 +99,10 @@ def __init__(self, config: 'SimpleConfig', exctype, value, tb):
         self.setLayout(main_box)
         self.show()
 
+    def _ask_for_confirm_to_send_report(self):
+        if self.question("Confirm to send bugreport?"):
+            self.send_report()
+
     def send_report(self):
         def on_success(response: CrashReportResponse):
             text = response.text
@@ -133,10 +133,6 @@ def on_close(self):
         Exception_Window._active_window = None
         self.close()
 
-    def show_never(self):
-        self.config.SHOW_CRASH_REPORTER = False
-        self.close()
-
     def closeEvent(self, event):
         self.on_close()
         event.accept()
@@ -181,23 +177,25 @@ def __init__(self, *, config: 'SimpleConfig'):
         assert self._INSTANCE is None, "Exception_Hook is supposed to be a singleton"
         self.config = config
         self.wallet_types_seen = set()  # type: Set[str]
+        self.exception_ids_seen = set()  # type: Set[bytes]
 
         sys.excepthook = self.handler
         self._report_exception.connect(_show_window)
         EarlyExceptionsQueue.set_hook_as_ready()
 
     @classmethod
     def maybe_setup(cls, *, config: 'SimpleConfig', wallet: 'Abstract_Wallet' = None) -> None:
-        if not config.SHOW_CRASH_REPORTER:
-            EarlyExceptionsQueue.set_hook_as_ready()  # flush already queued exceptions
-            return
         if not cls._INSTANCE:
             cls._INSTANCE = Exception_Hook(config=config)
         if wallet:
             cls._INSTANCE.wallet_types_seen.add(wallet.wallet_type)
 
     def handler(self, *exc_info):
         self.logger.error('exception caught by crash reporter', exc_info=exc_info)
+        groupid_hash = BaseCrashReporter.get_traceback_groupid_hash(*exc_info)
+        if groupid_hash in self.exception_ids_seen:
+            return  # to avoid annoying the user, only show crash reporter once per exception groupid
+        self.exception_ids_seen.add(groupid_hash)
         self._report_exception.emit(self.config, *exc_info)
 
 

electrum/simple_config.py

@@ -892,7 +892,6 @@ def __setattr__(self, name, value):
         long_desc=lambda: _("Select which language is used in the GUI (after restart)."),
     )
     BLOCKCHAIN_PREFERRED_BLOCK = ConfigVar('blockchain_preferred_block', default=None)
-    SHOW_CRASH_REPORTER = ConfigVar('show_crash_reporter', default=True, type_=bool)
     DONT_SHOW_TESTNET_WARNING = ConfigVar('dont_show_testnet_warning', default=False, type_=bool)
     RECENTLY_OPEN_WALLET_FILES = ConfigVar('recently_open', default=None)
     IO_DIRECTORY = ConfigVar('io_dir', default=os.path.expanduser('~'), type_=str)