-
-
Notifications
You must be signed in to change notification settings - Fork 169
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
resolve bytes vs strings conversion issues #3229
Comments
After experimenting with removing the lock used to make the non-cythonized version of rencode safe: https://github.com/totaam/rencode/tree/orig-nolock A better approach is just to "fork" this code directly into the xpra code: Supporting multiple connections with different capabilities is only really a problem for servers. |
@basilgello : as per the discussion in bb867cd, I had introduced a surprising bug in a last minute change... And here's the cause:
The expression is evaluated in a different order than I had expected... |
Latest trunk (fixed). Server: Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_window_base.py", line 2092, in do_delete_event
self._client.window_close_event(self._id)
File "/usr/lib/python3/dist-packages/xpra/client/mixins/window_manager.py", line 1128, in window_close_event
matching_title_close = [x for x in TITLE_CLOSEEXIT if x and title.startswith(x)]
File "/usr/lib/python3/dist-packages/xpra/client/mixins/window_manager.py", line 1128, in <listcomp>
matching_title_close = [x for x in TITLE_CLOSEEXIT if x and title.startswith(x)]
TypeError: startswith first arg must be bytes or a tuple of bytes, not str |
Also, Xpra system-wide proxy creates
|
Finally, removing
|
Xvfb-for-Xpra also do not gracefully shutdown on xpra server exit :( Same applies for dbus-daemon Clear permission error on Xpra server shutdown:
|
I solved this by adding: os.lchown(session_dir, uid, gid) before https://github.com/Xpra-org/xpra/blob/master/xpra/scripts/server.py#L875 |
Fixed in c12bca9.
That's #3217.
I've done something almost identical in 119cc8a except it only chowns it if we've just created the directory.
@basilgello that's strange because the html5 client is what I used primarily for developing this new |
Yes via proxy! |
for now we may get either and we should not care, convert to strings to make the code easier to read
Ah, yes: b20fefd fixes that. |
for now we may get either and we should not care, convert to strings to make the code easier to read
Yeah, this forks! :D |
(this is not compatible with the previous version)
|
@basilgello that's the same as: #3232 (comment) |
I still use master trunk from yesterday around 22:00 EET :) Let me build the trunk right now. Also, two more issues spotted:
|
As part of this work on rencode I found a DoS: totaam/rencode@f6254ab Here's the exploit to cause the xpra server to go unresponsive and consume 100%CPU until it eventually reaches out-of-memory:
Mitigations:
The performance loss is not critical.
|
You should CVE it as you forked it from aresch I guess :) |
|
With latest html5 trunk I saw only a couple of glitches 👍 I am testing on rendered PDFs now to be sure HTML5 client scrolling is usable again. |
Anything specific? I was going to release |
Well, not something reproducible in stable manner :) BTW, for me html5 client sound was totally broken regardless of xpra server side. Overload errors in JS types all the way. Now when recent Xpra has SoundPipeline broken (and I have yet to investigate why), I can not give you exact reproducer for html5 sound bug. |
Stopping xpra via
That is strange because I have no |
Is this a regression? Do you know the last version that worked for you?
I assume that's in |
TBH sound never worked for me within html5 client.
I assume yes. Just check you can hear a Youtube video via html5 client there :) |
Latest trunk of Xpra. GTK3 client again can play sound (choppy but still 👍 ) Server spews this: 2021-08-09 09:14:18,251 Error: invalid access from thread <Thread(parse, started daemon 140516473685760)>
File "/usr/lib/python3.9/threading.py", line 912, in _bootstrap
self._bootstrap_inner()
File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
self.run()
File "/usr/lib/python3.9/threading.py", line 892, in run
self._target(*self._args, **self._kwargs)
File "/usr/lib/python3/dist-packages/xpra/net/protocol.py", line 802, in _read_parse_thread_loop
self.do_read_parse_thread_loop()
File "/usr/lib/python3/dist-packages/xpra/net/protocol.py", line 1030, in do_read_parse_thread_loop
self._process_packet_cb(self, packet)
File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 823, in process_packet
XpraClientBase.process_packet(self, proto, packet)
File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 1060, in process_packet
call_handler()
File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 1057, in call_handler
handler(packet)
File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 908, in _process_hello
if not self.server_connection_established(caps):
File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 419, in server_connection_established
if not XpraClientBase.server_connection_established(self, caps):
File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 922, in server_connection_established
if not self.parse_server_capabilities(caps):
File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 428, in parse_server_capabilities
if not cb.parse_server_capabilities(self, c):
File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 208, in parse_server_capabilities
self.start_sending_sound()
File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 295, in start_sending_sound
enabled = self.start_sound_source(device)
File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 316, in start_sound_source
ss = start_sending_sound(plugins, self.sound_source_plugin, device or self.microphone_device,
File "/usr/lib/python3/dist-packages/xpra/sound/wrapper.py", line 375, in start_sending_sound
plugin, options = parse_sound_source(plugins, sound_source_plugin, device, want_monitor_device, remote)
File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 867, in parse_sound_source
options = get_sound_source_options(simple_str, options_str, device, want_monitor_device, remote)
File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 825, in get_sound_source_options
options = defaults_fn(device, want_monitor_device, remote)
File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 741, in get_pulse_source_defaults
return get_pulse_defaults(device_name_match, want_monitor_device,
File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 579, in get_pulse_defaults
device = get_pulse_device(device_name_match, want_monitor_device, input_or_output, remote, env_device_name)
File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 630, in get_pulse_device
pa_id = get_pulse_id()
File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_pactl_util.py", line 120, in get_pulse_id
return get_pulse_id_x11_property()
File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_common_util.py", line 59, in get_pulse_id_x11_property
return get_x11_property("PULSE_ID")
File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_common_util.py", line 32, in get_x11_property
with xswallow:
File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 206, in __enter__
trap.Xenter()
File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 100, in Xenter
verify_main_thread()
File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 57, in verify_main_thread
traceback.print_stack() |
master trunk of xpra + xpra-html5 built 15 minutes ago:
Keeping latest Xpra server but reverting HTML5 client to b28eadd resolves an issue |
My guess is that this was before 094169d and therefore using |
The latest trunk of html5 client still does not resolve the broken JPEG decoding. I guess I have to bisect the bug again in a separate issue in html5 |
@basilgello are you sure that you are using |
Yes, in both cases,it does use, $ xpra info :1 | grep client.connection.encoder
client.connection.encoder=rencodeplus |
I tried to check which commit introduces the regression: lz4 removal or webp/jpeg decoding in separate worker. The reference point which has smooth Youtube video playback in Firefox window on server while viewed on Chromium for Android client is b28eadd Both issues (broken notification window and stuttering playback) are introduced with a0f50fd7121a246722666314e331210dfc6435df Adding separate lz4 decryptor makes no change. I am now going to backport lz4 decryptor removal omitting worker thread commits to check if lz4 internal decoder also adds to performance drops. |
5704c7cf24c4c817ab1fc707f8a652968394f77f fixes notification pop-up but the main issue with stutter still persists… With b28eadd the network usage graph on my Android tab usually flows around ~300-1200kB/second, notification pop-ups are infrequent and literally no spinners. With separate worker commit and beyond, up to currently latest trunk, the playback freezes and network usage spikes to ~6mB/sec often accompanied by spinner. Should I debug |
@basilgello Please create a separate issue on the https://github.com/Xpra-org/xpra-html5 issue tracker, as this has nothing to do with rencode (AFAICT) and the commit links are all broken. The broken notification is already fixed: Xpra-org/xpra-html5@a23c611 |
always send strings as they are, bencode and rencode will encode them to utf8 bytes rencodeplus will just preserve them on the receiving side, if we get a string then there should be nothing to do (rencodeplus) and if we get bytes then we need to decode as utf8
let the packet layer handle it, mostly. (still use net_utf8() to retrieve string values that may have been converted to bytes)
This change totaam/rencode@4509f95 would allow us to use rencode the way we always wanted to use it: strings come out as strings and bytes come out as bytes...
Deluge hit the same issue: https://dev.deluge-torrent.org/ticket/2216
Before this can happen:
The text was updated successfully, but these errors were encountered: