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

X applications launched before i3 are buggy #2869

Open
ghost opened this Issue Aug 21, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@ghost

ghost commented Aug 21, 2017

Output of i3 --moreversion 2>&- || i3 --version:

Binary i3 version:  4.13 (2016-11-08) © 2009 Michael Stapelberg and contributors
Running i3 version: 4.13 (2016-11-08) (pid 8678)o abort…)
Loaded i3 config: /home/hadrien/.config/i3/config (Last modified: Mon 21 Aug 2017 10:24:21 AM CEST, 418 seconds ago)

The i3 binary you just called: /usr/bin/i3
The i3 binary you are running: i3

URL to a logfile as per http://i3wm.org/docs/debugging.html:
https://files.catbox.moe/t13m4i

What I did:
Start applications like this:

> tail -n5 .xinitrc
st -g 80x32+50+320 -c "floating" -e mutt &
st -g 80x32+50+847 -c "floating" -e newsbeuter &
st -g 92x65+705+320 -c "floating" -e cmus &
sleep 3
exec i3 -V >> ~/i3log-$(date +'%F-%k-%M-%S') 2>&1

The sleep 3 makes it reproducible. The first time launching X after booting the machine doesn't need this to get the bug.

What I saw:
https://files.catbox.moe/lp0wbk.webm
xwininfo/xprop can't detect those leftover windows.

What I expected instead:
Well, no workspace wide zombie windows.

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Aug 21, 2017

Member

What happens if you i3 restart in this case? This strikes me as really odd; in theory we do pick up on managing all windows upon startup.

Member

Airblader commented Aug 21, 2017

What happens if you i3 restart in this case? This strikes me as really odd; in theory we do pick up on managing all windows upon startup.

@i3 i3 deleted a comment from i3bot Aug 21, 2017

@Airblader Airblader removed the missing-log label Aug 21, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 21, 2017

It doesn't do anything. As in the zombie windows are still here.

ghost commented Aug 21, 2017

It doesn't do anything. As in the zombie windows are still here.

@orestisf1993

This comment has been minimized.

Show comment
Hide comment
@orestisf1993

orestisf1993 Sep 5, 2017

Member

Can reproduce this with the current next branch, ~/.xinitrc:

#!/bin/bash

urxvt &
sleep 3
exec i3 -V >> ~/i3log-$(date +'%F-%k-%M-%S') 2>&1  # Or exec i3

~/.i3/config:

set $mod Mod4

bindsym $mod+1 workspace 1 # Without this i3-migrate-config-to-v4 happens, which also adds a bar
bindsym $mod+e exec i3-msg exit
bindsym $mod+r exec i3-msg reload
Member

orestisf1993 commented Sep 5, 2017

Can reproduce this with the current next branch, ~/.xinitrc:

#!/bin/bash

urxvt &
sleep 3
exec i3 -V >> ~/i3log-$(date +'%F-%k-%M-%S') 2>&1  # Or exec i3

~/.i3/config:

set $mod Mod4

bindsym $mod+1 workspace 1 # Without this i3-migrate-config-to-v4 happens, which also adds a bar
bindsym $mod+e exec i3-msg exit
bindsym $mod+r exec i3-msg reload
@orestisf1993

This comment has been minimized.

Show comment
Hide comment
@orestisf1993

orestisf1993 Sep 5, 2017

Member

The problem doesn't appear with i3 -a. I think it has to do something with this code here:

i3/src/main.c

Lines 831 to 850 in 85eb097

if (autostart) {
LOG("This is not an in-place restart, copying root window contents to a pixmap\n");
xcb_screen_t *root = xcb_aux_get_screen(conn, conn_screen);
uint16_t width = root->width_in_pixels;
uint16_t height = root->height_in_pixels;
xcb_pixmap_t pixmap = xcb_generate_id(conn);
xcb_gcontext_t gc = xcb_generate_id(conn);
xcb_create_pixmap(conn, root->root_depth, pixmap, root->root, width, height);
xcb_create_gc(conn, gc, root->root,
XCB_GC_FUNCTION | XCB_GC_PLANE_MASK | XCB_GC_FILL_STYLE | XCB_GC_SUBWINDOW_MODE,
(uint32_t[]){XCB_GX_COPY, ~0, XCB_FILL_STYLE_SOLID, XCB_SUBWINDOW_MODE_INCLUDE_INFERIORS});
xcb_copy_area(conn, root->root, pixmap, gc, 0, 0, 0, 0, width, height);
xcb_change_window_attributes(conn, root->root, XCB_CW_BACK_PIXMAP, (uint32_t[]){pixmap});
xcb_flush(conn);
xcb_free_gc(conn, gc);
xcb_free_pixmap(conn, pixmap);
}

Member

orestisf1993 commented Sep 5, 2017

The problem doesn't appear with i3 -a. I think it has to do something with this code here:

i3/src/main.c

Lines 831 to 850 in 85eb097

if (autostart) {
LOG("This is not an in-place restart, copying root window contents to a pixmap\n");
xcb_screen_t *root = xcb_aux_get_screen(conn, conn_screen);
uint16_t width = root->width_in_pixels;
uint16_t height = root->height_in_pixels;
xcb_pixmap_t pixmap = xcb_generate_id(conn);
xcb_gcontext_t gc = xcb_generate_id(conn);
xcb_create_pixmap(conn, root->root_depth, pixmap, root->root, width, height);
xcb_create_gc(conn, gc, root->root,
XCB_GC_FUNCTION | XCB_GC_PLANE_MASK | XCB_GC_FILL_STYLE | XCB_GC_SUBWINDOW_MODE,
(uint32_t[]){XCB_GX_COPY, ~0, XCB_FILL_STYLE_SOLID, XCB_SUBWINDOW_MODE_INCLUDE_INFERIORS});
xcb_copy_area(conn, root->root, pixmap, gc, 0, 0, 0, 0, width, height);
xcb_change_window_attributes(conn, root->root, XCB_CW_BACK_PIXMAP, (uint32_t[]){pixmap});
xcb_flush(conn);
xcb_free_gc(conn, gc);
xcb_free_pixmap(conn, pixmap);
}

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Sep 5, 2017

Member

Yes, that seems reasonable. I was trying to think of why we are doing this at all, but then just git-blamed it to commit 4f5e0e794f61cdcea2652f964ec25d0b7418f69b which explains it. It does end with

This commit has some chance of breakage, so I’m prepared to revert it unless we can figure out the issues and roll forward.

but that was about four years ago. :-)

I think the approach of copying the pixmap is simply broken (as this issue demonstrates) and I would be inclined to revert that commit despite being there for so long. If someone uses -background none, they should change the setup not to do that or not to have things like gnome-settings-daemon interfere with it. It seems to me that i3 is trying to solve an issue that i3 is not even responsible for. We typically make the assertion that i3 does not handle the desktop background anyway.

@stapelberg What do you think?

I think this would also be a blocking issue for #536.

Member

Airblader commented Sep 5, 2017

Yes, that seems reasonable. I was trying to think of why we are doing this at all, but then just git-blamed it to commit 4f5e0e794f61cdcea2652f964ec25d0b7418f69b which explains it. It does end with

This commit has some chance of breakage, so I’m prepared to revert it unless we can figure out the issues and roll forward.

but that was about four years ago. :-)

I think the approach of copying the pixmap is simply broken (as this issue demonstrates) and I would be inclined to revert that commit despite being there for so long. If someone uses -background none, they should change the setup not to do that or not to have things like gnome-settings-daemon interfere with it. It seems to me that i3 is trying to solve an issue that i3 is not even responsible for. We typically make the assertion that i3 does not handle the desktop background anyway.

@stapelberg What do you think?

I think this would also be a blocking issue for #536.

orestisf1993 added a commit to orestisf1993/i3 that referenced this issue Sep 5, 2017

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Sep 6, 2017

Member

I’m okay with reverting the commit, but then we should definitely verify which login managers exhibit this issue in their current versions, and whether we can come up with a different fix. Maybe setting the background before launching i3 via the i3.desktop session would be an okay workaround.

Member

stapelberg commented Sep 6, 2017

I’m okay with reverting the commit, but then we should definitely verify which login managers exhibit this issue in their current versions, and whether we can come up with a different fix. Maybe setting the background before launching i3 via the i3.desktop session would be an okay workaround.

@Airblader

This comment has been minimized.

Show comment
Hide comment
@Airblader

Airblader Sep 6, 2017

Member

Given that we are at the start of 4.15, we could revert it and see whether someone complains. We can perhaps do a basic tes with GDM or so, but I doubt that we can reliably test a significant amount of combinations of setups.

Maybe setting the background before launching i3 via the i3.desktop session would be an okay workaround.

Is there really any reason we need to handle it? I'm sure that if the original issue was reported now, we'd reject the issue because we do not handle what's in the root window.

I wonder whether GDM & co even have good reasons to start without a background (?)

Member

Airblader commented Sep 6, 2017

Given that we are at the start of 4.15, we could revert it and see whether someone complains. We can perhaps do a basic tes with GDM or so, but I doubt that we can reliably test a significant amount of combinations of setups.

Maybe setting the background before launching i3 via the i3.desktop session would be an okay workaround.

Is there really any reason we need to handle it? I'm sure that if the original issue was reported now, we'd reject the issue because we do not handle what's in the root window.

I wonder whether GDM & co even have good reasons to start without a background (?)

@stapelberg

This comment has been minimized.

Show comment
Hide comment
@stapelberg

stapelberg Sep 6, 2017

Member

I wonder whether GDM & co even have good reasons to start without a background (?)

See the commit message — AIUI, it was a technique to do an animation / prevent flickering at login.

Member

stapelberg commented Sep 6, 2017

I wonder whether GDM & co even have good reasons to start without a background (?)

See the commit message — AIUI, it was a technique to do an animation / prevent flickering at login.

@psychon

This comment has been minimized.

Show comment
Hide comment
@psychon

psychon Nov 23, 2017

How about changing the code to only do that copy when the root window does not have a _XROOTPMAP_ID property? As far as I know, most tools that set backgrounds also set this property (and/or ESETROOT_PMAP_ID). The idea of course being: Only do this copy if no background is already set.

Note that setting _XROOTPMAP_ID correctly is a bit non-trivial since this property is set to the ID of a pixmap and that pixmap is supposed to be valid even after you disconnect from the X11 server. To make that need, you need xcb_set_close_down_mode(c, XCB_CLOSE_DOWN_RETAIN_PERMANENT), but in general this is a bad idea, so you need to open a second X11 connection to the server that is just used to allocate a pixmap in a way that it stays permanently valid...

Alternatively, the easy way is: Do not set _XROOTPMAP_ID and just query if it already set.

psychon commented Nov 23, 2017

How about changing the code to only do that copy when the root window does not have a _XROOTPMAP_ID property? As far as I know, most tools that set backgrounds also set this property (and/or ESETROOT_PMAP_ID). The idea of course being: Only do this copy if no background is already set.

Note that setting _XROOTPMAP_ID correctly is a bit non-trivial since this property is set to the ID of a pixmap and that pixmap is supposed to be valid even after you disconnect from the X11 server. To make that need, you need xcb_set_close_down_mode(c, XCB_CLOSE_DOWN_RETAIN_PERMANENT), but in general this is a bad idea, so you need to open a second X11 connection to the server that is just used to allocate a pixmap in a way that it stays permanently valid...

Alternatively, the easy way is: Do not set _XROOTPMAP_ID and just query if it already set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment