From da4f69cf7219d0c1c6116289a64a07239f3a2c23 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 4 Feb 2016 21:11:20 +0100 Subject: [PATCH 1/6] pdfjs: throw PDFJSNotFound from None Otherwise the stacktrace might be confusing since it will show the FileNotFoundException as the causing error, which is not true (it just happens to be the last checked place). The .path attribute was added so that we still have the requested path in the error log. See #1280. --- qutebrowser/browser/pdfjs.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/pdfjs.py b/qutebrowser/browser/pdfjs.py index b38d6edd1..aeb6435af 100644 --- a/qutebrowser/browser/pdfjs.py +++ b/qutebrowser/browser/pdfjs.py @@ -29,9 +29,16 @@ from qutebrowser.utils import utils class PDFJSNotFound(Exception): - """Raised when no pdf.js installation is found.""" + """Raised when no pdf.js installation is found. - pass + Attributes: + path: path of the file that was requested but not found. + """ + + def __init__(self, path): + self.path = path + message = "Path '{}' not found".format(path) + super().__init__(message) def generate_pdfjs_page(url): @@ -129,7 +136,7 @@ def get_pdfjs_res_and_path(path): try: content = utils.read_file(res_path, binary=True) except FileNotFoundError: - raise PDFJSNotFound + raise PDFJSNotFound(path) from None try: # Might be script/html or might be binary From 804b4750abda8de550b6fe94ca179c7b6700672f Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 4 Feb 2016 21:18:15 +0100 Subject: [PATCH 2/6] pdfjs: use list of tuples instead of dictionary Even though the dict seemed to be fine, this gives us a predictable replacement order and helps with debugging in the future. --- qutebrowser/browser/pdfjs.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/qutebrowser/browser/pdfjs.py b/qutebrowser/browser/pdfjs.py index aeb6435af..e6f6098e0 100644 --- a/qutebrowser/browser/pdfjs.py +++ b/qutebrowser/browser/pdfjs.py @@ -77,21 +77,21 @@ def fix_urls(asset): Args: asset: js file or html page as string. """ - new_urls = { - 'viewer.css': 'qute://pdfjs/web/viewer.css', - 'compatibility.js': 'qute://pdfjs/web/compatibility.js', - 'locale/locale.properties': - 'qute://pdfjs/web/locale/locale.properties', - 'l10n.js': 'qute://pdfjs/web/l10n.js', - '../build/pdf.js': 'qute://pdfjs/build/pdf.js', - 'debugger.js': 'qute://pdfjs/web/debugger.js', - 'viewer.js': 'qute://pdfjs/web/viewer.js', - 'compressed.tracemonkey-pldi-09.pdf': '', - './images/': 'qute://pdfjs/web/images/', - '../build/pdf.worker.js': 'qute://pdfjs/build/pdf.worker.js', - '../web/cmaps/': 'qute://pdfjs/web/cmaps/', - } - for original, new in new_urls.items(): + new_urls = [ + ('viewer.css', 'qute://pdfjs/web/viewer.css'), + ('compatibility.js', 'qute://pdfjs/web/compatibility.js'), + ('locale/locale.properties', + 'qute,//pdfjs/web/locale/locale.properties'), + ('l10n.js', 'qute://pdfjs/web/l10n.js'), + ('../build/pdf.js', 'qute://pdfjs/build/pdf.js'), + ('debugger.js', 'qute://pdfjs/web/debugger.js'), + ('viewer.js', 'qute://pdfjs/web/viewer.js'), + ('compressed.tracemonkey-pldi-09.pdf', ''), + ('./images/', 'qute://pdfjs/web/images/'), + ('../build/pdf.worker.js', 'qute://pdfjs/build/pdf.worker.js'), + ('../web/cmaps/', 'qute://pdfjs/web/cmaps/'), + ] + for original, new in new_urls: asset = asset.replace(original, new) return asset From 59c782f3835689607593af6d8c2bc7d771ed508f Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 5 Feb 2016 01:43:56 +0100 Subject: [PATCH 3/6] qutescheme: handle pdfjs failures more gracefully Now the browser does not crash anymore if an invalid pdfjs resource is requested, instead it will reply with a 404 error. --- qutebrowser/browser/network/qutescheme.py | 33 ++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/network/qutescheme.py b/qutebrowser/browser/network/qutescheme.py index 876d85e1f..8e8f0f200 100644 --- a/qutebrowser/browser/network/qutescheme.py +++ b/qutebrowser/browser/network/qutescheme.py @@ -58,6 +58,25 @@ def add_handler(name): return namedecorator +class QuteSchemeError(Exception): + + """Exception to signal that a handler should return an ErrorReply. + + Attributes correspond to the arguments in + networkreply.ErrorNetworkReply. + + Attributes: + errorstring: Error string to print. + error: Numerical error value. + """ + + def __init__(self, errorstring, error): + """Constructor.""" + + self.errorstring = errorstring + self.error = error + + class QuteSchemeHandler(schemehandler.SchemeHandler): """Scheme handler for qute: URLs.""" @@ -95,6 +114,9 @@ class QuteSchemeHandler(schemehandler.SchemeHandler): return networkreply.ErrorNetworkReply( request, str(e), QNetworkReply.ContentNotFoundError, self.parent()) + except QuteSchemeError as e: + return networkreply.ErrorNetworkReply( + request, e.errorstring, e.error, self.parent()) mimetype, _encoding = mimetypes.guess_type(request.url().fileName()) if mimetype is None: mimetype = 'text/html' @@ -212,4 +234,13 @@ def qute_settings(win_id, _request): def qute_pdfjs(_win_id, request): """Handler for qute://pdfjs. Return the pdf.js viewer.""" urlpath = request.url().path() - return pdfjs.get_pdfjs_res(urlpath) + try: + return pdfjs.get_pdfjs_res(urlpath) + except pdfjs.PDFJSNotFound as e: + # Logging as the error might get lost otherwise since we're not showing + # the error page if a single asset is missing. This way we don't lose + # information, as the failed pdfjs requests are still in the log. + log.misc.warning( + "pdfjs resource requested but not found: {}".format(e.path)) + raise QuteSchemeError("Can't find pdfjs resource '{}'".format(e.path), + QNetworkReply.ContentNotFoundError) From df8f87f8b6517454baf36a165a24e04fb54dafa4 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 5 Feb 2016 15:14:23 +0100 Subject: [PATCH 4/6] tests: add tests for qutescheme --- tests/unit/browser/network/test_qutescheme.py | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 tests/unit/browser/network/test_qutescheme.py diff --git a/tests/unit/browser/network/test_qutescheme.py b/tests/unit/browser/network/test_qutescheme.py new file mode 100644 index 000000000..1d9cfa5d5 --- /dev/null +++ b/tests/unit/browser/network/test_qutescheme.py @@ -0,0 +1,66 @@ +# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: + +# Copyright 2016 Daniel Schadt +# +# This file is part of qutebrowser. +# +# qutebrowser is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# qutebrowser is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with qutebrowser. If not, see . + +"""Tests for qutebrowser.browser.network.qutescheme.""" + +import pytest +import logging + +from PyQt5.QtCore import QUrl +from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply + +from qutebrowser.browser import pdfjs +from qutebrowser.browser.network import qutescheme + + +@pytest.fixture +def handler(): + return qutescheme.QuteSchemeHandler(win_id=0) + + +class TestPDFJSHandler: + + """Test the qute://pdfjs endpoint.""" + + @pytest.fixture(autouse=True) + def fake_pdfjs(self, monkeypatch): + + def get_pdfjs_res(path): + if path == '/existing/file': + return b'foobar' + raise pdfjs.PDFJSNotFound(path) + + monkeypatch.setattr('qutebrowser.browser.pdfjs.get_pdfjs_res', + get_pdfjs_res) + + def test_existing_resource(self, handler): + """Test with a resource that exists.""" + req = QNetworkRequest(QUrl('qute://pdfjs/existing/file')) + reply = handler.createRequest(None, req, None) + assert reply.readAll() == b'foobar' + + def test_nonexisting_resource(self, handler, caplog): + """Test with a resource that does not exist.""" + req = QNetworkRequest(QUrl('qute://pdfjs/no/file')) + with caplog.at_level(logging.WARNING, 'misc'): + reply = handler.createRequest(None, req, None) + assert reply.error() == QNetworkReply.ContentNotFoundError + assert len(caplog.records) == 1 + assert (caplog.records[0].message == + 'pdfjs resource requested but not found: /no/file') From d02ec62e33c89c1dae758c8529f28a5d654b57b7 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 5 Feb 2016 15:16:23 +0100 Subject: [PATCH 5/6] tests: fix tests for test_version Broken since we changed pdfjs.PDFJSNotFound to have a parameter, which was not given here. --- tests/unit/utils/test_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/utils/test_version.py b/tests/unit/utils/test_version.py index 14d819464..6c62fc0f9 100644 --- a/tests/unit/utils/test_version.py +++ b/tests/unit/utils/test_version.py @@ -536,7 +536,7 @@ class TestPDFJSVersion: def test_not_found(self, mocker): mocker.patch('qutebrowser.utils.version.pdfjs.get_pdfjs_res_and_path', - side_effect=pdfjs.PDFJSNotFound) + side_effect=pdfjs.PDFJSNotFound('/build/pdf.js')) assert version._pdfjs_version() == 'no' def test_unknown(self, monkeypatch): From 9676eab592b49759a5f66a456c2af6531aa82736 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 5 Feb 2016 15:18:27 +0100 Subject: [PATCH 6/6] qutescheme: call base __init__ in QuteSchemeError --- qutebrowser/browser/network/qutescheme.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/browser/network/qutescheme.py b/qutebrowser/browser/network/qutescheme.py index 8e8f0f200..1cc369694 100644 --- a/qutebrowser/browser/network/qutescheme.py +++ b/qutebrowser/browser/network/qutescheme.py @@ -72,9 +72,9 @@ class QuteSchemeError(Exception): def __init__(self, errorstring, error): """Constructor.""" - self.errorstring = errorstring self.error = error + super().__init__(errorstring) class QuteSchemeHandler(schemehandler.SchemeHandler):