-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RDY] Avoid crash with LC_ALL=C
and unicode filename
#3175
Conversation
LC_ALL=C
and unicode filenameLC_ALL=C
and unicode filename
return networkreply.FixedDataNetworkReply( | ||
request, data, 'text/html', self.parent()) | ||
except UnicodeEncodeError: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that one of the tests is failing because there's no test for the case you added. I'm not sure if there's a way to trigger this from the test suite, but if you think of one, could you add a unit test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I did not notice any failing errors (not more than those that normally fail at least).
I ran tox -e py36 tests/unit/browser/webkit/network/test_filescheme.py
and had 25 passing tests.
I modified test_file
in test_filescheme.py
to use φτωχικὸ
as a filename instead of foo
, but that will not raise an exception in tox even with LC_ALL=C
so I'm not sure what option there are to test this properly, any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was a bit too vauge there, the coverage tests are failing (meaning that coverage dropped on a perfect file) :P.
I actually can't test/verify anything about the webkit crash, I don't have webkit installed. Could you please provide a webkit crash stacktrace for me to look at?
The reason I'm asking is because (like you said) the test:
def test_file(self, tmpdir):
filename = tmpdir / 'φτωχικὸ'
os.environ['LC_ALL'] = "C"
filename.ensure()
url = QUrl.fromLocalFile(str(filename))
req = QNetworkRequest(url)
handler = filescheme.FileSchemeHandler(win_id=0)
reply = handler.createRequest(None, req, None)
assert reply is None
runs the changed code for me fine, without a patch. I'm not sure if setting the env var in python is sufficient or not (or if I need to be running all of qb, not just the test environment). A stacktrace would help with that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a stacktrace, it's actually the same as that in #1450 :)
$ LC_ALL=C python -m qutebrowser --temp-basedir --backend webkit xxφτωχικὸ
21:31:43 ERROR: Error in qutebrowser.browser.webkit.network.networkmanager.NetworkManager.createRequest
Traceback (most recent call last):
File "/home/l/.proj/qutebrowser/qutebrowser/utils/utils.py", line 635, in wrapper
return func(*args, **kwargs)
File "/home/l/.proj/qutebrowser/qutebrowser/browser/webkit/network/networkmanager.py", line 390, in createRequest
op, req, outgoing_data)
File "/home/l/.proj/qutebrowser/qutebrowser/browser/webkit/network/filescheme.py", line 130, in createRequest
if os.path.isdir(path):
File "/usr/lib/python3.6/genericpath.py", line 42, in isdir
st = os.stat(s)
UnicodeEncodeError: 'ascii' codec can't encode characters in position 28-72: ordinal not in range(128)
In any case yeah, I hadn't thought about coverage, in that case adding a tests for an unicode filename is useful even if, de facto, it doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take that back. Since the exception is never raised during tests, those two lines are never tested against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Feel free to ping me should I need to do more for this PR
Fixes #1450
This fixes the crash, instead displaying an error.
It's not the best option, but it's better than a crash
This change is