After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756074 - model+transient for dialogs sometimes appear non-modal
model+transient for dialogs sometimes appear non-modal
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.18.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2015-10-05 09:21 UTC by Timm Bäder
Modified: 2015-10-06 20:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test case (850 bytes, text/plain)
2015-10-05 09:21 UTC, Timm Bäder
  Details
xprops: Fix reading Window and XSyncCounter properties (1.45 KB, patch)
2015-10-05 16:03 UTC, Rui Matos
committed Details | Review

Description Timm Bäder 2015-10-05 09:21:57 UTC
Created attachment 312664 [details]
test case

Contrary to https://bugzilla.gnome.org/show_bug.cgi?id=685064, this only started happening since 3.18.

The dialog that appears after clicking the button in the test case is sometimes properly attached to the parent window (which gets darkened), but sometimes it just appears as a normal non-modal dialog. I've failed to find any way to trigger this reliably.
Comment 1 Rui Matos 2015-10-05 16:03:02 UTC
Created attachment 312681 [details] [review]
xprops: Fix reading Window and XSyncCounter properties

Both Window and XSyncCounter are XIDs which on 64 bit X clients are 8
bytes wide. But the values on the wire are 32 bit so, for these types,
we always copy 4 bytes into results->prop. As such copying them out
with a cast such as *(Window *) means that we are actually reading 8
bytes which depending on whether the higher addressed 4 bytes are zero
means that sometimes this works while others it gives us a bogus
value.
Comment 2 Jasper St. Pierre (not reading bugmail) 2015-10-05 16:31:39 UTC
Review of attachment 312681 [details] [review]:

gah, the xcb transition strikes again. Sorry about that one.

::: src/x11/xprops.c
@@ +508,3 @@
     return FALSE;
 
+  *window_p = *(uint32_t *) results->prop;

Hm. Given window_p is a Window*, I think this will warn, and also still not be correct -- I'm not sure if C will implicitly upcast during the pointer set.

If you think it will, go for it, otherwise:

    Window w = *(uint32_t *) results->prop;
    *window_p = w;

is how I would write it.

(Also, perhaps use xcb_window_t instead of uint32_t?)
Comment 3 Rui Matos 2015-10-06 20:45:00 UTC
(In reply to Jasper St. Pierre from comment #2)
> ::: src/x11/xprops.c
> @@ +508,3 @@
>      return FALSE;
>
> +  *window_p = *(uint32_t *) results->prop;

> Hm. Given window_p is a Window*, I think this will warn, and also
> still not be correct -- I'm not sure if C will implicitly upcast
> during the pointer set.

It doesn't warn. It's essentially the same as:

uint64_t a;
uint32_t b = 123;

a = b;

which should be safe AFAICT.

Attachment 312681 [details] pushed as 54557f0 - xprops: Fix reading Window and XSyncCounter properties