From 8795f89c889fe60ffeeca6af30c997ecd96279a8 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 7 Jul 2016 00:08:13 +0200 Subject: [PATCH] tests: fix tests for downloads bdd test The test was failing because of two reasons: First, the old code had filename questions in DownloadManager.get and DownloadManager.fetch which were almost identical, thus the part in DownloadManager.get was removed in an earlier commit. All filename asking is now done by DownloadManager.fetch. The good part is code deduplication, the bad part is slightly modified behavior: The new code doesn't wait for a filename to start the download, instead it tries to fill the buffer immediately. This made the test fail because qute:// has no registered handler, so in order for the test to pass now, the "no crash" part is not enough, we also need to expect the "No handler" error. Secondly, and a rather rare (race) condition was the handling of errors in the DownloadItem. If an error occured after the registration of self.on_reply_error as error handler and before the check reply.error() != QNetworkReply.NoError at the end of the function, the error signal would be emitted twice: Once by _die() (called by on_reply_error), and once by the init_reply function directly (in the last if block). This lead to duplicated error messages. This is also explained in a comment in the file (with small "stack traces"). --- qutebrowser/browser/webkit/downloads.py | 19 ++++++++++++++++++- tests/end2end/features/downloads.feature | 3 +-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 45cd73baa..c684e1608 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -281,6 +281,7 @@ class DownloadItem(QObject): _read_timer: A Timer which reads the QNetworkReply into self._buffer periodically. _win_id: The window ID the DownloadItem runs in. + _dead: Whether the Download has _die()'d. Signals: data_changed: The downloads metadata changed. @@ -329,6 +330,7 @@ class DownloadItem(QObject): self.init_reply(reply) self._win_id = win_id self.raw_headers = {} + self._dead = False def __repr__(self): return utils.get_repr(self, basename=self.basename) @@ -396,6 +398,21 @@ class DownloadItem(QObject): def _die(self, msg): """Abort the download and emit an error.""" assert not self.successful + # Prevent actions if calling _die() twice. This might happen if the + # error handler correctly connects, and the error occurs in init_reply + # between reply.error.connect and the reply.error() check. In this + # case, the connected error handlers will be called twice, once via the + # direct error.emit() and once here in _die(). The stacks look like + # this then: + # -> on_reply_error -> _die -> + # self.error.emit() + # and + # [init_reply -> ->] -> + # self.error.emit() + # which may lead to duplicate error messages (and failing tests) + if self._dead: + return + self._dead = True self._read_timer.stop() self.reply.downloadProgress.disconnect() self.reply.finished.disconnect() @@ -442,7 +459,7 @@ class DownloadItem(QObject): # Here no signals are connected to the DownloadItem yet, so we use a # singleShot QTimer to emit them after they are connected. if reply.error() != QNetworkReply.NoError: - QTimer.singleShot(0, lambda: self.error.emit(reply.errorString())) + QTimer.singleShot(0, lambda: self._die(reply.errorString())) def get_status_color(self, position): """Choose an appropriate color for presenting the download's status. diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index f15466053..7548c0dda 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -31,8 +31,7 @@ Feature: Downloading things from a website. And I run :hint links download And I run :follow-hint a And I wait for "Asking question text='Save file to:'>, *" in the log - And I run :leave-mode - Then no crash should happen + Then the error "Download error: No handler found for qute://!" should be shown Scenario: Downloading a data: link (issue 1214) When I set completion -> download-path-suggestion to filename