Skip to content
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

Factor out switcheroos for the different wayland shell protocols #359

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Nasah-Kuma
Copy link
Contributor

No description provided.

@philn
Copy link
Member

philn commented Sep 14, 2021

The rename fdo to wl in the source and build files commit was merged in master. Can you rebase please?

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 4 times, most recently from 616a173 to 33a8e45 Compare September 14, 2021 21:33
@Nasah-Kuma
Copy link
Contributor Author

The rename fdo to wl in the source and build files commit was merged in master. Can you rebase please?

Ok I have

Copy link
Contributor

@Cwiiis Cwiiis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start. The two biggest correctness issues here are that the shell functions pointer is accessed without allocating it (null pointer dereference), as mentioned below, and also that in registry_global, if none of the cases that set shell functions are hit, the shell functions will be undefined (rather than leading to a default set of functions that would print an error message, or some other appropriate action). In practice, both of these issues would result in crashing in the best case.

Once these issues are tackled, the next big thing will be figuring out how to get these into separate files. Separating the data necessary for these functions from win_data will play a large part in that, but let's tackle that when we get there!

code-style.diff Outdated
@@ -0,0 +1,54 @@
--- cog.c (before formatting)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this file isn't intended to be in this commit.

@@ -757,14 +961,30 @@ registry_global (void *data,
wl_data.subcompositor = wl_registry_bind(registry, name, &wl_subcompositor_interface, version);
} else if (strcmp(interface, wl_shell_interface.name) == 0) {
wl_data.shell = wl_registry_bind(registry, name, &wl_shell_interface, version);
win_data.shell_functions->enter_fullscreen = &wl_shell_enter_fullscreen;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting each function, it might be worthwhile just having each set of functions pre-defined, so something like:

   static const shell_operations wayland_shell_operations = {
     .enter_fullscreen = &wl_shell_enter_fullscreen,
     .exit_fullscreen = &wl_shell_exit_fullscreen,
     ...
   };
   
   ...
   
     win_data.shell_functions = &wayland_shell_operations

As it is, win_data.shell_functions is a pointer and this is referencing invalid memory as I don't see anywhere where shell_functions is set.

{
g_clear_pointer(&wl_data.event_src, g_source_destroy);
win_data.shell_functions->destroy_shell();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's worth clearing the shell_functions pointer after calling destroy_shell - this would make the situation where a shell function is erroneously called after this function easier to debug.

#endif

typedef struct shell_operations {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good to be more consistent with naming - we have shell_operations * shell_functions just below here. Also, I notice that two extra xdg-related pointers are added to the win_data struct. Maybe it would be better to have a structure "shell_context" that encapsulates both the functions and any implementation-specific data necessary, and calling this "shell_protocol". Use of unions here might save some memory and make organisation a bit easier, for example:

typedef struct {
  void (*enter_fullscreen)();
  void (*exit_fullscreen)();
} ShellFunctions;

struct xdg_shell_protocol = {
  ShellFunctions functions;
  struct xdg_surface * xdg_surface;
  struct xdg_toplevel * xdg_toplevel;
};

struct f_shell_protocol {
  ShellFunctions functions;
  // ... Any data that fshell needs specifically ...
};

typedef union {
  ShellFunctions functions;
  struct xdg_shell_protocol xdg_context;
} ShellContext;

static ShellContext xdg_shell = {
  .functions = {
    &xdg_enter_fullscreen,
    &xdg_exit_fullscreen
  }
};

(excuse any incorrect syntax here, let's call this pseudo-code :))
So after this, you could have a ShellContext* ctx in win_data, and accessing functions would work much the same, like win_data.ctx->functions.enter_fullscreen() and individual data could be accessed like win_data.ctx->xdg_context.xdg_surface.

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from f230dd8 to e8baaff Compare September 20, 2021 20:32
Copy link
Contributor

@Cwiiis Cwiiis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely moving in the right direction - some of my comments from before still apply and I've added some new ones for this current revision.

static void resize_to_largest_output();
static void resize_window();
static void configure_surface_geometry();
static void xdg_surface_on_configure(void *data, struct xdg_surface *surface, uint32_t serial);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for ease of reading, it'd be nice to put an empty line between the forward declarations above and the shell function definitions here.

int32_t height,
struct wl_array * states);
static void xdg_toplevel_on_close(void *data, struct xdg_toplevel *xdg_toplevel);
static void shell_surface_ping(void *data, struct wl_shell_surface *shell_surface, uint32_t serial);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the xdg and shell surface/popup listener declarations mixed together like this is a little confusing, better to keep them together and separated by newlines to make reading easier. I would also consider putting them closer to where they're used, rather than just at the very top like this.

@@ -373,6 +616,23 @@ wl_src_finalize(GSource *base)
{
}

static void
default_no_shell(shell_functions *shell_functions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this function is actually intended to do at the moment.

if (wl_data.shell != NULL) {
win_data.shell_functions = &wayland_shell_functions;
} else {
default_no_shell(&wayland_shell_functions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess default_no_shell should be setting up default shell functions, but currently it checks whether various function pointers exist in the given shell_functions struct and then calls the default implementation of the first one it finds and returns?

I would suggest that much like you now have wayland_shell_functions, xdg_shell_functions and f_shell_functions, you should also have no_shell_functions (or whatever you'd like to call it) and that you just set that up immediately before this big if/else block. That would mean you no longer need all these else clauses.

I may have misinterpreted the intension of default_no_shell though, please let me know if so!

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from 630a199 to 4e3d79f Compare September 26, 2021 19:05
Copy link
Contributor

@Cwiiis Cwiiis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's been a misunderstand of how unions work here, as there are multiple parts of the code that access multiple members of it. Only one member of the shell_context union is going to be valid for the lifecycle of the application - only the shell_functions member should be accessed directly by code outside of shell functions (inside of which you can know which member of the union is valid).

Are you testing cog after making these changes? It seems unlikely that you wouldn't see crashing - make sure to test at least a couple of backends as you work, most of these issues would very likely be flagged by basic testing.

} shell_functions;

typedef struct {
shell_functions * functions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any reason to make this a pointer.

.maximize_surface = &maximize_xdg_shell_surface,
.create_window = &create_xdg_shell_window,
.create_popup = &create_xdg_shell_popup};
xdg_shell_data xdg_data = {.functions = &xdg_shell_functions};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If functions wasn't a pointer, you could just declare the functions struct directly here;

.functions = { .enter_fullscreen = ...,
                      ... }

g_assert_not_reached();
}
static void
maximize_no_shell()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with what you've done above, this should be called no_shell_maximize.

platform/wayland/cog-platform-wl.c Show resolved Hide resolved

win_data.wl_surface = wl_compositor_create_surface (wl_data.compositor);
g_assert (win_data.wl_surface);
win_data.shell_context->wl_shell_data.wl_surface = wl_compositor_create_surface(wl_data.compositor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has wl_surface been moved to wl_shell_data? It doesn't seem to be specific to wl_shell and putting it here means that you're going to corrupt data in some way or another when using any shell that isn't wl_shell.

g_clear_pointer (&win_data.xdg_surface, xdg_surface_destroy);
g_clear_pointer (&win_data.shell_surface, wl_shell_surface_destroy);
g_clear_pointer (&win_data.wl_surface, wl_surface_destroy);
g_clear_pointer(&win_data.shell_context->xdg_shell_data.xdg_toplevel, xdg_toplevel_destroy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shell_context is only going to be one type of shell, accessing it as multiple types like this almost certainly means you're calling one or more of these functions with invalid data. This should be factored out into a surface_destroy function in shell_functions.

g_clear_pointer (&popup_data.xdg_positioner, xdg_positioner_destroy);
g_clear_pointer (&popup_data.shell_surface, wl_shell_surface_destroy);
g_clear_pointer (&popup_data.wl_surface, wl_surface_destroy);
g_clear_pointer(&popup_data.shell_context->xdg_shell_data.xdg_popup, xdg_popup_destroy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this block, you can't access multiple members of shell_context like this, only one is going to be valid.

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 4e3d79f to d86b37a Compare October 4, 2021 10:34
Copy link
Contributor

@Cwiiis Cwiiis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely looking better, but lots of small things to address. I wouldn't mind seeing another commit that addresses these smaller details, but feel free to go ahead with splitting this out into separate file(s) if it makes sense for you.

xdg_toplevel_unset_fullscreen(win_data.shell_context->xdg_shell_data.xdg_toplevel);
}
static void
destroy_xdg_shell()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called xdg_shell_destroy_shell, to match the other functions.

xdg_wm_base_destroy(wl_data.xdg_shell);
}
static void
maximize_xdg_shell_surface()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, xdg_shell_maximize_surface.

};

static void
create_xdg_shell_window()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xdg_shell_create_window.

wl_surface_commit(win_data.wl_surface);
}
static void
create_xdg_shell_popup()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xdg_shell_create_popup

}

static void
destroy_xdg_surfaces()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xdg_shell_surface_destroy

platform/wayland/cog-platform-wl.c Show resolved Hide resolved
.enter_fullscreen = &f_shell_enter_fullscreen}};

//default executed when no shell exist
static void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would declare these before all the shell implementations, so that when shells are missing capabilities (like with f_shell above), you can reference these default implementations. I also note that there doesn't seem to be a full set of functions or an accompanying shell_functions struct for these.

wl_surface_damage(win_data.wl_surface, 0, 0, INT32_MAX, INT32_MAX);
request_frame ();
wl_surface_commit (win_data.wl_surface);
wl_surface_attach(win_data.shell_context->wl_shell_data.wl_surface, buffer->buffer, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be win_data.wl_surface, right? This wouldn't build if HAVE_SHM_EXPORTED_BUFFER is defined.

platform/wayland/cog-platform-wl.c Show resolved Hide resolved
g_clear_pointer (&popup_data.xdg_positioner, xdg_positioner_destroy);
g_clear_pointer (&popup_data.shell_surface, wl_shell_surface_destroy);
g_clear_pointer (&popup_data.wl_surface, wl_surface_destroy);
win_data.shell_context->functions.surface_destroy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing to me - perhaps this function should be called popup_surface_destroy to be clearer?

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from d86b37a to bbcc7e5 Compare October 9, 2021 10:35
Copy link
Contributor

@Cwiiis Cwiiis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better, but still some issues to address.

static void
xdg_shell_create_window()
{
win_data.shell_context->xdg_shell_data.xdg_surface =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make these functions read a bit more easily, it might be worthwhile to have a xdg_shell_data* xdg_data = (xdg_shell_data*)win_data.shell_context; at the top to stop from having to type win_data.shell_context->xdg_shell_data so much.


const char * app_id = NULL;
GApplication *app = g_application_get_default();
if (app) {
Copy link
Contributor

@Cwiiis Cwiiis Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the documentation for g_application_get_application_id, this could just be written directly as

const char * app_id = g_application_get_application_id(g_application_get_default());
if (!app_id)
    app_id = COG_DEFAULT_APPID;

as g_application_get_application_id accepts NULL as a parameter (apparently - worth testing before committing to, in case the documentation is incorrect).


static struct {
shell_context * shell_context;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this added, but I don't see it set anywhere.

static void
xdg_shell_create_popup()
{
popup_data.shell_context->xdg_shell_data.xdg_positioner = xdg_wm_base_create_positioner(wl_data.xdg_shell);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this function uses popup_data.shell_context, but I don't see that set anywhere? If it was set, given it's a pointer, you would need to copy it to avoid using the same variables as win_data. I think you're going in the right direction, but there's a misunderstanding here of what pointers are - You can't use the same surface pointers for both win_data and popup_data. As before, I think if this code was actually run as, it'd very likely crash. Have you tested this?

@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch 2 times, most recently from 0db356b to 42ac42a Compare October 19, 2021 20:24
@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 42ac42a to 7976a2d Compare November 1, 2021 13:22
@Nasah-Kuma Nasah-Kuma force-pushed the factor-out-switcheroos-for-the-different-Wayland-shell-protocols branch from 7976a2d to e56bdca Compare November 2, 2021 15:43
Change text mentioning the "fdo" platform plug-in to "wl", which was
missing from Igalia#351.
Arrange to call drmFreeDevices() inside check_drm() and init_drm()
to avoid leaking the arrays of devices returned by drmGetDevices2().
Pass the correct pkg-config module name to gi-docgen depending on the
version of libsoup in use: WPE WebKit builds with libsoup2 have API
version 1.0, and builds with libsoup3 have API version 1.1.
This avoids not finding any environment setup script due to the CPU name
and features changing when SDKs are updated: use a wildcard to make sure
at least one gets picked.
Use GObjectClass.finalize instead of CogPlatformClass.teardown. Now that
CogPlatform derives from GObject, it is possible to reuse its functionality
instead of having an ad-hoc vfunc that gets called immediately before
destroying an instance. As a bonus, not only the implementations follow
a more idiomatic GObject approach, but also it is not possible anymore
to forget a call to cog_platform_teardown() before destruction.
Update the check-style script and the CI job to use clang-format 13, which
is the first version that supports combining The "PointerAlignment: Right"
with "AlignConsecutiveDeclarations: true". This was unimplemented until
recently, see https://reviews.llvm.org/D103245

While at it, do not let clang-format choose a different way of doing
pointer alignment by setting "DerivePointerAlignment: false".
Retry the curl command up to ten times in case of download failures.
This should alleviate build issues like the following, where the server
sometimes seems to wrongly close HTTP/2 streams, or when some connection
issue results in partial downloads:

    https://github.com/Igalia/cog/runs/4176134469
    https://github.com/Igalia/cog/runs/4176255670

While at it, pass "-C -" to let curl determine from which point to retry
the download. As the toolchain packages are big, this should help avoid
timeouts.

Finally, use HTTP/1.1; because using HTTP/2 seems to be a source of
sporadic issues, and for a big, bulky download, the gains from HTTP/2
(like multiple streams and faster connection open) are basically none
due to the transfer itself taking most of the time.
@aperezdc
Copy link
Member

I have taken a brief look at this, and it looks like there is not much left to do before it can be merged—thanks everybody involved!

Nevertheless, I would like to make today the 0.12 stable branch, so I am going to defer to tag it for 0.14 😸

@aperezdc aperezdc added the enhancement New feature or request label Nov 11, 2021
@aperezdc aperezdc added this to the Cog 0.14 milestone Nov 11, 2021
@aperezdc
Copy link
Member

As agreed with @Nasah-Kuma, I have done a rebase and squashed some of the commits together, and added one more commit with a small cleanup—I will push it momentarily.

While testing, I have noticed that with the changes applied Cog segfaults when running under wlroots-based compositors (which used to work fine). The traceback is as follows (log edited to show the interesting bits):

% WAYLAND_DISPLAY=wayland-1 G_MESSAGES_DEBUG=Cog,Cog-Core,Cog-FDO COG_MODULEDIR=b/modules \
    gdb --args b/cog -P wl ddg.gg
[...]
(cog:755159): Cog-DEBUG: 20:11:18.071: platform_setup: Platform name: wl
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'wl_compositor' interface obtained from the Wayland registry.
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'wl_subcompositor' interface obtained from the Wayland registry.
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.083: Using 'xdg_wm_base' interface obtained from the Wayland registry.
[...]
(cog:755159): Cog-FDO-DEBUG: 20:11:18.391: New XDG toplevel configuration: (0, 0)
(cog:755159): Cog-FDO-DEBUG: 20:11:18.391: Resized EGL buffer to: (1024, 768) @1x


Thread 1 "cog" received signal SIGSEGV, Segmentation fault.
0x00007ffff7f96928 in cog_popup_menu_get_buffer (popup_menu=0x0) at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-popup-menu-wl.c:318
318         if (popup_menu->pending_changes) {
(gdb) bt
#0  0x00007ffff7f96928 in cog_popup_menu_get_buffer (popup_menu=0x0) at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-popup-menu-wl.c:318
#1  0x00007ffff7f93a32 in display_popup () at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-platform-wl.c:2515
#2  0x00007ffff0f8cd4a in  () at /usr/lib/libffi.so.8
#3  0x00007ffff0f8c267 in  () at /usr/lib/libffi.so.8
#4  0x00007fffeec6f323 in  () at /usr/lib/libwayland-client.so.0
#5  0x00007fffeec6b5ab in  () at /usr/lib/libwayland-client.so.0
#6  0x00007fffeec6cf4c in wl_display_dispatch_queue_pending () at /usr/lib/libwayland-client.so.0
#7  0x00007ffff7f92531 in wl_src_dispatch (base=0x5555556b50c0, callback=<optimized out>, user_data=<optimized out>)
    at /home/aperez/devel/wpe/cog-master/platform/wayland/cog-platform-wl.c:704
#8  0x00007ffff2c34fd3 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#9  0x00007ffff2c8b049 in  () at /usr/lib/libglib-2.0.so.0
#10 0x00007ffff2c32545 in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#11 0x00007ffff2e526ee in g_application_run () at /usr/lib/libgio-2.0.so.0
#12 0x0000555555559b9f in main (argc=3, argv=0x7fffffffdb68) at /home/aperez/devel/wpe/cog-master/launcher/cog.c:41
(gdb)

I have not yet investigated why this happens, but will try to debug it in the next days.

Running under Weston works as expected, so I suppose that was the compositor used for testing 😄

@aperezdc
Copy link
Member

Ah, I just realized that of course I cannot push to @Nasah-Kuma's branch in their fork, so I have posted the rebase to the aperezdc/pr359-rebased branch for now.

@aperezdc aperezdc modified the milestones: Cog 0.14, Cog 0.16 Jun 29, 2022
@aperezdc aperezdc modified the milestones: Cog 0.16, Cog 0.18 Sep 29, 2022
@aperezdc aperezdc modified the milestones: Cog 0.18, Cog 0.20 Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants