From c363982d057a82ef23edb4a506f06eedda732913 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 22 Nov 2016 11:10:37 +0100 Subject: [PATCH] Use per-tab QNAM for QtNetwork downloads again When starting a download due to unsupportedContent being emitted, we need to use (and later adopt) the page's QNetworkAccessManager. Since we need the whole adopting logic for that case anyways, let's keep things as they were and always run downloads in per-tab QNAMs. This reverts 53e360ec4b156c1e43ecceb6ef0787f64281c345 and fixes #2134. --- qutebrowser/browser/commands.py | 13 +++++- qutebrowser/browser/hints.py | 11 ++++- qutebrowser/browser/qtnetworkdownloads.py | 42 +++++++++++++++++-- .../browser/webkit/network/networkmanager.py | 28 +++++++++++++ qutebrowser/browser/webkit/webpage.py | 10 ++++- tests/end2end/features/downloads.feature | 13 ++++++ 6 files changed, 109 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 45320f011..220ef614f 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1332,11 +1332,22 @@ class CommandDispatcher: elif mhtml_: self._download_mhtml(dest) else: + tab = self._current_widget() + + # FIXME:qtwebengine have a proper API for this + # pylint: disable=protected-access + try: + qnam = tab._widget.page().networkAccessManager() + except AttributeError: + # QtWebEngine + qnam = None + # pylint: enable=protected-access + if dest is None: target = None else: target = downloads.FileDownloadTarget(dest) - download_manager.get(self._current_url(), target=target) + download_manager.get(self._current_url(), qnam=qnam, target=target) def _download_mhtml(self, dest=None): """Download the current page as an MHTML file, including all assets. diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index af1827b2f..40fbcd8d4 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -284,10 +284,19 @@ class HintActions: else: prompt = None + # FIXME:qtwebengine get a proper API for this + # pylint: disable=protected-access + try: + qnam = elem._elem.webFrame().page().networkAccessManager() + except AttributeError: + # QtWebEngine + qnam = None + # pylint: enable=protected-access + # FIXME:qtwebengine do this with QtWebEngine downloads? download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) - download_manager.get(url, prompt_download_directory=prompt) + download_manager.get(url, qnam=qnam, prompt_download_directory=prompt) def call_userscript(self, elem, context): """Call a userscript from a hint. diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index b75fee821..eeda5d9be 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -24,7 +24,7 @@ import shutil import functools import collections -from PyQt5.QtCore import pyqtSlot, QTimer +from PyQt5.QtCore import pyqtSlot, pyqtSignal, QTimer from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply from qutebrowser.utils import message, usertypes, log, urlutils @@ -67,9 +67,15 @@ class DownloadItem(downloads.AbstractDownloadItem): _reply: The QNetworkReply associated with this download. _autoclose: Whether to close the associated file when the download is done. + + Signals: + adopt_download: Emitted when a download is retried and should be + adopted by the QNAM if needed. + arg 0: The new DownloadItem """ _MAX_REDIRECTS = 10 + adopt_download = pyqtSignal(object) # DownloadItem def __init__(self, reply, manager): """Constructor. @@ -162,7 +168,9 @@ class DownloadItem(downloads.AbstractDownloadItem): assert self.done assert not self.successful new_reply = self._retry_info.manager.get(self._retry_info.request) - self._manager.fetch(new_reply, suggested_filename=self.basename) + new_download = self._manager.fetch(new_reply, + suggested_filename=self.basename) + self.adopt_download.emit(new_download) self.cancel() def _get_open_filename(self): @@ -335,6 +343,14 @@ class DownloadItem(downloads.AbstractDownloadItem): old_reply.deleteLater() return True + def _uses_nam(self, nam): + """Check if this download uses the given QNetworkAccessManager.""" + running_nam = self._reply is not None and self._reply.manager() is nam + # user could request retry after tab is closed. + retry_nam = (self.done and (not self.successful) and + self._retry_info.manager is nam) + return running_nam or retry_nam + class DownloadManager(downloads.AbstractDownloadManager): @@ -409,17 +425,20 @@ class DownloadManager(downloads.AbstractDownloadManager): suggested_filename=suggested_fn, **kwargs) - def _fetch_request(self, request, **kwargs): + def _fetch_request(self, request, *, qnam=None, **kwargs): """Download a QNetworkRequest to disk. Args: request: The QNetworkRequest to download. + qnam: The QNetworkAccessManager to use. **kwargs: passed to fetch(). Return: The created DownloadItem. """ - reply = self._networkmanager.get(request) + if qnam is None: + qnam = self._networkmanager + reply = qnam.get(request) return self.fetch(reply, **kwargs) @pyqtSlot('QNetworkReply') @@ -467,3 +486,18 @@ class DownloadManager(downloads.AbstractDownloadManager): message.global_bridge.ask(question, blocking=False) return download + + def has_downloads_with_nam(self, nam): + """Check if the DownloadManager has any downloads with the given QNAM. + + Args: + nam: The QNetworkAccessManager to check. + + Return: + A boolean. + """ + assert nam.adopted_downloads == 0 + for download in self.downloads: + if download._uses_nam(nam): # pylint: disable=protected-access + nam.adopt_download(download) + return nam.adopted_downloads diff --git a/qutebrowser/browser/webkit/network/networkmanager.py b/qutebrowser/browser/webkit/network/networkmanager.py index 100db9ab0..477a9958a 100644 --- a/qutebrowser/browser/webkit/network/networkmanager.py +++ b/qutebrowser/browser/webkit/network/networkmanager.py @@ -116,6 +116,11 @@ class NetworkManager(QNetworkAccessManager): """Our own QNetworkAccessManager. Attributes: + adopted_downloads: If downloads are running with this QNAM but the + associated tab gets closed already, the NAM gets + reparented to the DownloadManager. This counts the + still running downloads, so the QNAM can clean + itself up when this reaches zero again. _requests: Pending requests. _scheme_handlers: A dictionary (scheme -> handler) of supported custom schemes. @@ -137,6 +142,7 @@ class NetworkManager(QNetworkAccessManager): # http://www.riverbankcomputing.com/pipermail/pyqt/2014-November/035045.html super().__init__(parent) log.init.debug("NetworkManager init done") + self.adopted_downloads = 0 self._win_id = win_id self._tab_id = tab_id self._requests = [] @@ -328,6 +334,28 @@ class NetworkManager(QNetworkAccessManager): # switched from private mode to normal mode self._set_cookiejar() + @pyqtSlot() + def on_adopted_download_destroyed(self): + """Check if we can clean up if an adopted download was destroyed. + + See the description for adopted_downloads for details. + """ + self.adopted_downloads -= 1 + log.downloads.debug("Adopted download destroyed, {} left.".format( + self.adopted_downloads)) + assert self.adopted_downloads >= 0 + if self.adopted_downloads == 0: + self.deleteLater() + + @pyqtSlot(object) # DownloadItem + def adopt_download(self, download): + """Adopt a new DownloadItem.""" + self.adopted_downloads += 1 + log.downloads.debug("Adopted download, {} adopted.".format( + self.adopted_downloads)) + download.destroyed.connect(self.on_adopted_download_destroyed) + download.adopt_download.connect(self.adopt_download) + def set_referer(self, req, current_url): """Set the referer header.""" referer_header_conf = config.get('network', 'referer-header') diff --git a/qutebrowser/browser/webkit/webpage.py b/qutebrowser/browser/webkit/webpage.py index 464fc3785..7fc891c4c 100644 --- a/qutebrowser/browser/webkit/webpage.py +++ b/qutebrowser/browser/webkit/webpage.py @@ -210,7 +210,13 @@ class BrowserPage(QWebPage): """Prepare the web page for being deleted.""" self._is_shutting_down = True self.shutting_down.emit() - self.networkAccessManager().shutdown() + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) + nam = self.networkAccessManager() + if download_manager.has_downloads_with_nam(nam): + nam.setParent(download_manager) + else: + nam.shutdown() def display_content(self, reply, mimetype): """Display a QNetworkReply with an explicitly set mimetype.""" @@ -239,7 +245,7 @@ class BrowserPage(QWebPage): req = QNetworkRequest(request) download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) - download_manager.get_request(req) + download_manager.get_request(req, qnam=self.networkAccessManager()) @pyqtSlot('QNetworkReply*') def on_unsupported_content(self, reply): diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 1e1e59b34..14757c87a 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -100,6 +100,19 @@ Feature: Downloading things from a website. And I run :close Then qutebrowser should quit + # https://github.com/The-Compiler/qutebrowser/issues/2134 + @qtwebengine_skip + Scenario: Downloading, then closing a tab + When I set storage -> prompt-download-directory to false + And I open about:blank + And I open data/downloads/issue2134.html in a new tab + # This needs to be a download connected to the tabs QNAM + And I hint with args "links normal" and follow a + And I wait for "fetch: * -> drip" in the log + And I run :tab-close + And I wait for "Download drip finished" in the log + Then the downloaded file drip should contain 128 bytes + ## :download-retry Scenario: Retrying a failed download