I guess this reverts commit 73bdc0ce85
"xwm: Fix fd leak in weston_wm_send_data()"
That commit closes the send half of a pipe in weston_wm_send_data,
claiming that it's dup()licated later, and we'll leak the fd if
we don't close it.
That may have been true at the time? But currently that fd is only
duplicated by wl_event_loop_add_fd() in its normal operation, and
closing our original before that fd handler ever fires results
in an EBADF on write, and the data never reaching its intended
destination.
Worse, by the time that handler is called there might be another
use for that fd, and we could push data into it and close it.
To provoke the problem, launch an app like FireFox over Xwayland,
cut something to the clipboard, then close the app (this is the
path where the wm has stored the clipboard contents and the
app has gone away). relaunch it and paste the clipboard content
back in. clipboard_client_data() will EBADF on write, and the
data won't be pasted.
Reported-by: Hideyuki Nagase <hideyukn@microsoft.com>
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Toy toolkit apps already do this since commit 807cd2e589
Co-authored-by: Steve Pronovost <spronovo@microsoft.com>
Co-authored-by: Brenton DeGeer <brdegeer@microsoft.com>
Signed-off-by: Hideyuki Nagase <hideyukn@microsoft.com>
Signed-off-by: Steve Pronovost <spronovo@microsoft.com>
Signed-off-by: Brenton DeGeer <brdegeer@microsoft.com>
We need to remove our listener link before we free the structure
it's inside, or the signal list walk will try to dereference it.
Remove the improbable NULL check at the same time.
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
This leg of the if/else ladder is a duplicate of the previous conditional.
It appears to have been intended to log enter events, but we already
handle those earlier. The drop event is already logged as well,
so let's just discard this branch entirely.
Fixes#552
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Memleak found by ASAN:
Direct leak of 21 byte(s) in 1 object(s) allocated from:
#0 0x7fe7a917fe8f in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.6+0xa9e8f)
#1 0x7fe7a9129874 (/usr/lib/x86_64-linux-gnu/libasan.so.6+0x53874)
#2 0x7fe7a5a23469 in weston_wm_window_read_properties ../xwayland/window-manager.c:574
#3 0x7fe7a5a28d3b in weston_wm_handle_map_request ../xwayland/window-manager.c:1178
#4 0x7fe7a5a31660 in weston_wm_handle_event ../xwayland/window-manager.c:2291
#5 0x7fe7a8c261a1 in wl_event_loop_dispatch ../src/event-loop.c:1027
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Memleak found by ASAN:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
#0 0x7fe7a917fe8f in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.6+0xa9e8f)
#1 0x7fe7a5a40736 in theme_create ../shared/cairo-util.c:419
#2 0x7fe7a5a3363c in weston_wm_create ../xwayland/window-manager.c:2619
#3 0x7fe7a5a2017e in weston_xwayland_xserver_loaded ../xwayland/launcher.c:313
#4 0x7fe7a90b4d14 in handle_sigusr1 ../compositor/xwayland.c:57
#5 0x7fe7a8c2585d in wl_event_source_signal_dispatch ../src/event-loop.c:685
#6 0x7ffcdb04ef6f ([stack]+0x1df6f)
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
There is more state than just the application window width and height that
affects whether calling weston_wm_window_configure() is necessary: everything
that affects the frame window, fullscreen state in particular. Therefore do not
skip the call by just width and height.
If send_configure() happens to be called "unnecessarily", this will now forward
some of those calls to the X11 clients. However, since it uses an idle task, it
should not result in a flood at least. And if send_configure() is spammed,
maybe that should then be fixed in its callers.
This patch should fix the misplacement of a fullscreen X11 window due to the
frame window being incorrectly sized and positioned, and the app window
incorrectly positioned inside the frame window.
The fullscreen window problems were observed in a case where the window does
not hit legacy_fullscreen() but first maps and then sets
_NET_WM_STATE_FULLSCREEN. Additionally the initial window size must match the
output size where it gets fullscreened. In that case the frame window was left
as if not fullscreened.
This practically reverts 3f53d9179b. I'm not sure
what problem that patch was fixing, but I couldn't make any resizing freeze.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Spotted this in debug log:
[xwm-wm-x11] XWM: configure window 4194324: x=32 y=32 width=1920 height=1080 border_width=0 stack_mode=0
[xwm-wm-x11] XWM: configure window 0: width=1984 height=1144
Trying to configure window 0 makes no sense. So do not try.
To avoid patching two different places with the same thing, refactor the code
into a common helper.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
It would lead to use-after-free if there was a pending idle callback to
weston_wm_window_configure() when the weston_wm_window gets destroyed. Make
sure the callback will not fire.
Found by inspection.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This function is called also directly from weston_wm_window_set_toplevel(). If
configure_source is set at that point, simply resetting the pointer will "leak"
the source until it fires and calls this function again.
Let's keep the variable up-to-date by removing the source when called,
dispatched or not. This removes the second call. I only hope it doesn't cause
issues. This is also necessary if we intend to remove the source on window
destruction too.
Found by inspection.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
It looks like commit ad0da4596d introduced a bug
for X11 windows that are initially fullscreen by adding code to the end of
xserver_map_shell_surface() while ignoring the 'return' that this patch
removes. That may have caused some annoying window state issues, but the
problem became more pronounced with 7ace831ca6
when used with an Xwayland version that honours _XWAYLAND_ALLOW_COMMITS.
In the latter case, there is a possiblity the window will never show up, as XWM
forgets to set allow_commits=true. However, the window may sometimes actually
show up due to an oversight in Xwayland: the Present code may be flipping the
window buffers and not checking _XWAYLAND_ALLOW_COMMITS if it is supposed
commit at all.
Since then, f568968f8a added more places where
allow_commits is set to true, masking the window-does-not-show-up issue. Window
pending state likely still remained an issue.
This patch properly fixes the "window never appears" issue by making sure
allow_commit=true is set. At the same time, it ensures the pending state
functions are called at the end of xserver_map_shell_surface(), which may fix
some window state issues like misplaced decorations and/or position of
initially-fullscreen windows. Unfortunately, it certainly does not fix all such
state problems.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Commit "weston-log: add function to avoid direct
access to compositor members in non-core code" added the
function weston_compositor_add_log_scope mainly to allow
libweston users to avoid direct accessing core structs, as
weston_compositor.
Replace weston_log_context_add_log_scope usage by
weston_compositor_add_log_scope.
Signed-off-by: Leandro Ribeiro <leandrohr@riseup.net>
Fixes missing prototypes compilation warnings emitted when a function
is defined before its prototype is declared.
These warnings were introduced over time since the switch to meson
because the -Wmissing-protoypes was not included in the compilation
arguments.
Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
There's a function named weston_compositor_log_scope_destroy()
but it doesn't take a struct weston_compositor argument.
Rename it to weston_log_scope_destroy(), as the argument is a
struct weston_log_scope.
Signed-off-by: Leandro Ribeiro <leandrohr@riseup.net>
There's a function named weston_compositor_add_log_scope()
but it doesn't take a struct weston_compositor argument.
Rename it to weston_log_ctx_add_log_scope(), as
the log_scope is being added to a log_context.
Also, bump libweston_major to 9.
Signed-off-by: Leandro Ribeiro <leandrohr@riseup.net>
The mask argument is uint16_t so declare the variable with the same.
Suggested-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Knowing the kind of decoration drawn will help track down issues with
unexpected decorations.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Print the changes to the debug scope, helping to figure out why Xwayland is or
is not committing.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Add the missing newline to printing a property that is of type cardinal array.
Fixes messed up debug scope output.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Initially, `_XWAYLAND_ALLOW_COMMITS` was introduced in commit 7ace831ca
to avoid drawing the window content before it's ready to be shown.
But a repaint might also be triggered by the client damages before the
XWM has finished drawing its window decorations and drop shadows, which
previously was not too much of an issue since the XWM could still
finish updating the X11 window after the buffer was submitted.
However, with the addition of multiple window buffers in Xwayland [1]
which are aimed at preventing the X11 clients from updating the buffer
after it's been committed, this is no longer possible.
As a result, the use of multiple window buffers in Xwayland can cause
ugly repainting effects of the decorations if the buffer is submitted
before the XWM has finished painting its decorations.
Use the X11 property `_XWAYLAND_ALLOW_COMMITS` can be used to avoid
this, by controlling when Xwayland should commit changes to the Wayland
surface.
[1] https://gitlab.freedesktop.org/xorg/xserver/merge_requests/316
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
This introduces a new convention of checking through the compositor destroy
listener if the plugin is already initialized. If the plugin is already
initialized, then the plugin entry function succeeds as a no-op. This makes it
safe to load the same plugin multiple times in a running compositor.
Currently module loading functions return failure if a plugin is already
loaded, but that will change in the future. Therefore we need this other method
of ensuring we do not double-initialize a plugin which would lead to list
corruptions the very least.
All plugins are converted to use the new helper, except:
- those that do not have a destroy listener already, and
- hmi-controller which does the same open-coded as the common code pattern
did not fit there.
Plugins should always have a compositor destroy listener registered since they
very least allocate a struct to hold their data. Hence omissions are
highlighted in code.
Backends do not need this because weston_compositor_load_backend() already
protects against double-init. GL-renderer does not export a standard module
init function so cannot be initialized the usual way and therefore is not
vulnerable to double-init.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
We have two kinds of libweston users: internal and external. Weston, the
frontend, counts as an external user, and should not have access to libweston
private headers. The shell plugins are external users as well, because we
intend people to be able to write them. Renderers, backends, and some plugins
are internal users who will need access to private headers.
Create two different Meson dependency objects, one for each kind.
This makes it less likely to accidentally use a private header.
Screen-share is a Weston plugin and therefore counts as an external user, but
it needs the backend API to deliver input. Until we are comfortable exposing
public API for that purpose, let it use internal headers.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
As 'new_subscription' can create additional objects, 'destroy_subscription'
will be needed when cleaning up.
As this requires a libweston_major bump (noticed by @pq), bump it up to
8.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Suggested-by: Daniel Stone <daniel.stone@collabora.com>
The xwayland plugin (XWM) is a libweston plugin, so it has no business using
Weston things. Luckily its not, this was just a left-over include. Remove it to
reduce confusion.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Define common_inc which includes both public_inc and the project root directory.
The project root directory will allow access to config.h and all the shared/
headers.
Replacing all custom '.', '..', '../..', '../shared' etc. include paths with
common_inc reduces clutter in the target definitions and enforces the common
#include directive style, as e.g. including shared/ headers without the
subdirectory name no longer works.
Unfortunately this does not prevent one from using private libweston headers
with the usual include pattern for public headers.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
When all shared/ headers are included in the same way, we can drop unnecessary
include seach paths from the compiler.
This include style was chosen because it is prevalent in the code base. Doing
anything different would have been a bigger patch.
This also means that we need to keep the project root directory in the include
search path, which means that one could accidentally include private headers
with
#include "libweston/dbus.h"
or even
#include <libweston/dbus.h>
IMO such problem is smaller than the churn caused by any of the alternatives,
and we should be able to catch those in review. We might even be able to catch
those with grep in CI if necessary.
The "bad" include style was found with:
$ for h in shared/*.h; do git grep -F $(basename $h); done | grep -vF '"shared/'
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Turns out dnd.c does not actually need cairo-util.h. This was found when
unifying the include directives of all shared/ headers. Removing this makes one
less place to fix.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
As we transition towards a more generic API for weston loggging
framework rename weston_debug_compositor to weston_log_context to show
the fact that this is not really debug but a logging context.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
The printf() format specifier "%m" is a glibc extension to print
the string returned by strerror(errno). While supported by other
libraries (e.g. uClibc and musl), it is not widely portable.
In Weston code the format string is often passed to a logging
function that calls other syscalls before the conversion of "%m"
takes place. If one of such syscall modifies the value in errno,
the conversion of "%m" will incorrectly report the error string
corresponding to the new value of errno.
Remove all the occurrences of the specifier "%m" in Weston code
by using directly the string returned by strerror(errno).
While there, fix some minor indentation issue.
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
weston.h is a Weston frontend header, while this is a libweston plugin. A
libweston plugin cannot depend on Weston. Luckily the header is not actually
needed.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
The main idea is to make libweston users use the form
#include <libweston/libweston.h>
instead of the plain
#include <compositor.h>
which is prone to name conflicts. This is reflected both in the installed
files, and the internal header search paths so that Weston would use the exact
same form as an external project using libweston would.
The public headers are moved under a new top-level directory include/ to make
them clearly stand out as special (public API).
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This crash was happening *during resizing* of an xwayland window that was destroyed.
Discovered by: John Good @archiesix
[@daniels: Moved tests below declarations.]
Use the pattern to avoid identing everything, when everything in the
file is under the one conditional.
Helps readability.
Signed-off-by: Pekka Paalanen <pq@iki.fi>
Derek Foreman
Hideyuki Nagase
shierote
Emmanuel Gil Peyrot
John Good
Marius Vlad
Vlad Zahorodnii
Jimmy Ohn
Pekka Paalanen
Leandro Ribeiro
Guillaume Champagne
Link Mauve
Olivier Fourdan
Antonio Borneo
Greg V
Pekka Paalanen