Fix issue where opening a file whose name contains characters not present in locale would cause a crash. #3177

lejar · 2017-10-21T19:04:52Z

Fixes qutebrowser/qutebrowser/#1450

This change is

present in locale would cause a crash. Fixes qutebrowser/qutebrowser/1450

lejar · 2017-10-21T19:09:26Z

If there is a respective test I could write then if someone tells me where it goes I'd be more than happy to make it.

jgkamat · 2017-10-21T19:29:36Z

qutebrowser/utils/urlutils.py

+                if os.path.exists(path):
+                    log.url.debug("URL is a local file")
+            except UnicodeEncodeError:
+                path = None


I think it's a good idea to log this as debug, the same way the other branch gets logged.

jgkamat · 2017-10-21T19:45:11Z

I took a look at this, but

os.environ["LC_ALL"] = "C"

dosen't seem to actually set the locale, so I can't find a way to trigger this under tox. Even running tox with LC_ALL=C dosen't seem to work for me. The actual test should be simple, something like:

    def test_get_path_existing_unicode(self, os_mock):
        """Test unicode input on non-unicode systems don't crash."""
        os.environ["LC_ALL"] = "C"
        # os_mock.path.exists.return_value = False
        url = urlutils.get_path_if_valid("/föö", check_exists=True)
        assert url is None

But I don't know how to get this to reproduce reliably in tox.

The-Compiler · 2017-10-21T20:19:19Z

qutebrowser/utils/urlutils.py

+                if os.path.exists(path):
+                    log.url.debug("URL is a local file")
+            except UnicodeEncodeError:
+                path = None
        else:
            path = None


No need for this else: anymore as we just do if path is not None: above now.

The-Compiler · 2017-10-21T20:21:31Z

tests/end2end/test_invocations.py is the right place to add a test for this, as LC_ALL needs to be set before qutebrowser is launched. There's already test_ascii_locale there, might want to rename that to test_downloads_with_ascii_locale or so and write something similar for :open. Let me know if you need help!

lejar · 2017-10-22T17:43:51Z

I don't have the latest Qt installed on my machine so I'm using travis to check that the test I wrote succeeds while I install it.

The-Compiler · 2017-10-22T17:53:39Z

qutebrowser/utils/urlutils.py

+                    log.url.debug("URL is a local file")
+            except UnicodeEncodeError:
+                log.url.debug(
+                    "URL contains characters which are not present in the " \


No need for the \ here, inside parentheses whitespace is ignored by Python.

The-Compiler · 2017-10-22T17:54:06Z

tests/end2end/test_invocations.py

+
+    https://github.com/qutebrowser/qutebrowser/issues/1450
+    """
+    args = ['--temp-basedir'] + _base_args(request.config)


Might want to also pass a file to open here on the commandline, as that seemed to trigger a slightly different bug?

It doesn't crash, but just shows URL_NOT_FOUND, which I think would be correct. Regardless, I added a test for it in case that breaks in the future.

The-Compiler

I haven't had time to take a closer look yet, but this seems to break the entire testsuite on Travis. Can you take a look at that please?

lejar · 2017-10-25T17:44:20Z

@The-Compiler I'm having a lot of trouble getting the tests to actually run on my machine. According to the tox output, the tests are forcing the webengine but they just fail. Do I need a special build of qt/pyqt?

The-Compiler · 2017-10-25T18:00:09Z

Does it help if you run tox with -- --qute-bdd-webengine?

The-Compiler · 2017-10-28T16:18:48Z

I've also tried to make the entire testsuite run with LC_ALL=C, with this patch:

diff --git a/pytest.ini b/pytest.ini
index 1dc9e3ba5..0cc5e49aa 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -26,6 +26,7 @@ markers =
     no_invalid_lines: Don't fail on unparseable lines in end2end tests
     issue2478: Tests which are broken on Windows with QtWebEngine, https://github.com/qutebrowser/qutebrowser/issues/2478
     fake_os: Fake utils.is_* to a fake operating system
+    unicode_locale: Tests which need an unicode locale to work
 qt_log_level_fail = WARNING
 qt_log_ignore =
     ^SpellCheck: .*
diff --git a/tests/conftest.py b/tests/conftest.py
index 301658d56..0c28ed17b 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -63,6 +63,8 @@ def _apply_platform_markers(config, item):
         ('no_ci', 'CI' in os.environ, "Skipped on CI."),
         ('issue2478', utils.is_windows and config.webengine,
          "Broken with QtWebEngine on Windows"),
+        ('unicode_locale', sys.getfilesystemencoding() == 'ascii',
+         "Skipped because of ASCII locale"),
     ]
 
     for searched_marker, condition, default_reason in markers:
diff --git a/tests/end2end/features/history.feature b/tests/end2end/features/history.feature
index 86e5d6f00..aa126c4f7 100644
--- a/tests/end2end/features/history.feature
+++ b/tests/end2end/features/history.feature
@@ -32,6 +32,7 @@ Feature: Page history
         Then the history should contain:
             http://localhost:(port)/data/title%20with%20spaces.html Test title
 
+    @unicode_locale
     Scenario: History item with umlauts
         When I open data/äöü.html
         Then the history should contain:
diff --git a/tests/end2end/test_invocations.py b/tests/end2end/test_invocations.py
index 1a91be2e0..921eb085a 100644
--- a/tests/end2end/test_invocations.py
+++ b/tests/end2end/test_invocations.py
@@ -70,6 +70,7 @@ def temp_basedir_env(tmpdir, short_tmpdir):
 
 
 @pytest.mark.linux
+@pytest.mark.unicode_locale
 def test_ascii_locale(request, server, tmpdir, quteproc_new):
     """Test downloads with LC_ALL=C set.
 
diff --git a/tests/unit/commands/test_userscripts.py b/tests/unit/commands/test_userscripts.py
index 9a6490aca..32f9a1bb9 100644
--- a/tests/unit/commands/test_userscripts.py
+++ b/tests/unit/commands/test_userscripts.py
@@ -241,9 +241,8 @@ def test_unicode_error(caplog, qtbot, py_proc, runner):
             runner.store_html('')
 
     assert len(caplog.records) == 1
-    expected = ("Invalid unicode in userscript output: 'utf-8' codec can't "
-                "decode byte 0x80 in position 0: invalid start byte")
-    assert caplog.records[0].message == expected
+    expected = "Invalid unicode in userscript output: "
+    assert caplog.records[0].message.startswith(expected)
 
 
 @pytest.mark.fake_os('unknown')

Continuing that effort once this PR is in.

lejar · 2017-10-29T17:31:30Z

@The-Compiler No I still get lots of errors. Here's a run-down of what I'm doing:
System information:

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.3 LTS
Release:	16.04
Codename:	xenial

$ python --version
Python 3.6.3

$ python -mpip freeze
attrs==17.2.0
colorama==0.3.9
cssutils==1.0.2
Jinja2==2.9.6
MarkupSafe==1.0
pkg-resources==0.0.0
Pygments==2.2.0
pyPEG2==2.15.2
PyYAML==3.12

PyQt is 5.9 (self compiled)
sip is 4.19.3 (self compiled)
Qt is 5.9.2 (self compiled)

Running the tests on commit 4c2aeb0

$ ls
config.py  doc  icons  LICENSE  MANIFEST.in  misc  pytest.ini  qutebrowser  qutebrowser.py  README.asciidoc  relative  requirements.txt  scripts  setup.py  tests  tox.ini  www
$ tox -e py36 -- --qute-bdd-webengine

The most common error I'm getting is

E       Failed: Got logging message on logger misc with level WARNING: YAML load took unusually long, please report this at https://github.com/qutebrowser/qutebrowser/issues/2777
E       duration: 14.380468s
E       PyYAML version: 3.12
E       C extension: False
E       Stack:

The-Compiler · 2017-10-30T05:50:52Z

I've commented in #2777 about that issue. Can you please reply there?

The-Compiler · 2017-11-07T16:46:20Z

I haven't had time to look at why the testsuite is failing for you in that weird way yet, so I took over here to merge things - I hope you don't mind! 😉

The reason the (existing) tests were failing is because path wasn't set to None correctly if the file didn't exist - I fixed that in 9f89861.

The new tests also needed some fixes to work properly - I added them in fdc4343.

Thanks!

Fix issue where opening a file whose name contains characters not

bdfb9c6

present in locale would cause a crash. Fixes qutebrowser/qutebrowser/1450

lejar mentioned this pull request Oct 21, 2017

UnicodeEncodeError with os.path.isdir in NetworkManager.createRequest #1450

Closed

jgkamat reviewed Oct 21, 2017

View reviewed changes

The-Compiler reviewed Oct 21, 2017

View reviewed changes

The-Compiler mentioned this pull request Oct 21, 2017

[RDY] Avoid crash with LC_ALL=C and unicode filename #3175

Merged

lejar added 2 commits October 22, 2017 18:41

Log when url contains characters not present in current locale.

96eff65

Add test for opening non-ascii paths.

f53d813

The-Compiler requested changes Oct 22, 2017

View reviewed changes

Add test for opening non-ascii paths from command line.

f67c445

The-Compiler requested changes Oct 24, 2017

View reviewed changes

The-Compiler mentioned this pull request Oct 30, 2017

configdata.yml performance issues / switch away from PyYAML #2777

Open

The-Compiler merged commit f67c445 into qutebrowser:master Nov 7, 2017

qutebrowser-bot added the status: stale label Dec 20, 2017

The-Compiler mentioned this pull request Jul 21, 2023

Clean up py3.6 ascii_locale related tests and code #7787

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where opening a file whose name contains characters not present in locale would cause a crash. #3177

Fix issue where opening a file whose name contains characters not present in locale would cause a crash. #3177

lejar commented Oct 21, 2017 •

edited

lejar commented Oct 21, 2017

jgkamat Oct 21, 2017

jgkamat commented Oct 21, 2017 •

edited

The-Compiler Oct 21, 2017

The-Compiler commented Oct 21, 2017

lejar commented Oct 22, 2017

The-Compiler Oct 22, 2017

The-Compiler Oct 22, 2017

lejar Oct 23, 2017

The-Compiler left a comment

lejar commented Oct 25, 2017

The-Compiler commented Oct 25, 2017

The-Compiler commented Oct 28, 2017

lejar commented Oct 29, 2017

The-Compiler commented Oct 30, 2017

The-Compiler commented Nov 7, 2017

Fix issue where opening a file whose name contains characters not present in locale would cause a crash. #3177

Fix issue where opening a file whose name contains characters not present in locale would cause a crash. #3177

Conversation

lejar commented Oct 21, 2017 • edited

lejar commented Oct 21, 2017

jgkamat Oct 21, 2017

Choose a reason for hiding this comment

jgkamat commented Oct 21, 2017 • edited

The-Compiler Oct 21, 2017

Choose a reason for hiding this comment

The-Compiler commented Oct 21, 2017

lejar commented Oct 22, 2017

The-Compiler Oct 22, 2017

Choose a reason for hiding this comment

The-Compiler Oct 22, 2017

Choose a reason for hiding this comment

lejar Oct 23, 2017

Choose a reason for hiding this comment

The-Compiler left a comment

Choose a reason for hiding this comment

lejar commented Oct 25, 2017

The-Compiler commented Oct 25, 2017

The-Compiler commented Oct 28, 2017

lejar commented Oct 29, 2017

The-Compiler commented Oct 30, 2017

The-Compiler commented Nov 7, 2017

lejar commented Oct 21, 2017 •

edited

jgkamat commented Oct 21, 2017 •

edited