GNOME Bugzilla – Bug 774379
gdk: mingw64 builds segfault during initialization of Huion H610PRO wintab
Last modified: 2016-11-28 15:06:58 UTC
Created attachment 339775 [details] .tar.gz of (short) logs with--gdk-debug=input (Major because it can cause programs to exit prematurely with some hardware combinations) gtk3-demo exits during initialization for mingw64 builds, debugging or otherwise, if a Huion H610PRO tablet is plugged in during wintab initialization. This is a regression since before 3.22 - I do remember pressure working on Windows - but I am not sure when. Wacom tablets and their latest drivers may all be unaffected. I only have an Intuos5 PTH-650 to play with though. GTK version/ref, Arch, Tab. Connected, Tablet+Driver v., Working? ----------------, -------, --------------, ----------------, ----------- 3.22.1, mingw32, nothingplugged, h610pro+V12.2.14, ok 3.22.1, mingw64, nothingplugged, h610pro+V12.2.14, ok 3.22.1, mingw64, pluggedatstart, h610pro+V12.2.14, exits 3.22.1, mingw32, pluggedatstart, h610pro+V12.2.14, nopressure gtk-3-22@d3bdd38, mingw64, nothingplugged, h610pro+V12.2.14, ok gtk-3-22@d3bdd38, mingw32, nothingplugged, h610pro+V12.2.14, ok gtk-3-22@d3bdd38, mingw64, pluggedatstart, h610pro+V12.2.14, exits gtk-3-22@d3bdd38, mingw32, pluggedatstart, h610pro+V12.2.14, nopressure gtk-3-22@d3bdd38, MINGW64, pluggedatstart, h610pro+V12.2.16, exits gtk-3-22@d3bdd38, MINGW32, pluggedatstart, h610pro+V12.2.16, nopressure gtk-3-22@d3bdd38, MINGW64, nothingplugged, intuos5+6.3.18-5, ok gtk-3-22@d3bdd38, MINGW32, nothingplugged, intuos5+6.3.18-5, ok gtk-3-22@d3bdd38, MINGW32, pluggedatstart, intuos5+6.3.18-5, ok gtk-3-22@d3bdd38, MINGW64, pluggedatstart, intuos5+6.3.18-5, ok The second set with the @ are debugging builds from git, from demo/gtk-demo/gtk3-demo.exe. The first four are the gtk3-demo from their stock MSYS2 mingw-w64-{i686,x86_64}-gtk3 packages. Logs attached. Looking at these logs together with https://git.gnome.org/browse/gtk+/tree/gdk/win32/gdkdevicemanager-win32.c?h=3.22.3#n507, gtk3-demo is bombing out before calling GDK_WINDOW_HWND(). Conversely, the plugged Intuos5 paths go on to emit "opened Wintab device 0 0000000000000803" and "context for device 0 after WTOpen:", then continue to proceed normally. This GDK_WINDOW_HWND() business seems familiar, from bug 764664. Regression? Transitions between sets of these results were prepared by cleanly uninstalling, one wintab.dll, rebooting, installing another, and rebooting again.
Each of the logs has a bit where I opened the Touch and Drawing Tablets tester and scribbled briefly (if I could), so there may be all sorts later on. I guess it's only the init that really matters. Weird stuff for a future time: unlike the Wacom driver, the Huion driver incorrectly reports tablets as present when they are not plugged in. But it only bombs out in the case where the USB tablet is connected.
Affects the other demo exes from the same stable. It's segfaulting, but it's somewhere in the bowels of WTOpenA() as expected. Kicking off some proper debug packages now, since gdb isn't really working on the static ones. Let me know what you need.
Good news! This one is buggier when you go hunting for it! This is sure to result in a useful bug report! If you run gtk3-demo outside the debugger, it only crashes if the tablet is plugged in. However, running it inside the debugger produces a segfault every time. There are a couple of different types of crash I've observed. Variant 1, with tablet plugged IIRC: $ gdb -ex r --args gtk3-demo GNU gdb (GDB) 7.12 Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-w64-mingw32". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from gtk3-demo...done. Starting program: C:\msys64\mingw64\bin\gtk3-demo.exe [New Thread 3732.0x9dc] [New Thread 3732.0xc64] [New Thread 3732.0xf98] [New Thread 3732.0xa6c] [New Thread 3732.0xd34] [New Thread 3732.0x944] warning: WTContextManager() this:fb22e310 warning: WTRoundArray() this:fb22eb90 Thread 1 received signal SIGSEGV, Segmentation fault. gdk_event_translate (msg=msg@entry=0x22e8c0, ret_valp=ret_valp@entry=0x22e8bc) at gdkevents-win32.c:2159 2159 (gdb) bt full
+ Trace 236853
Variant 2, or "once more, with UpdateLayeredWindowIndirect()". The getting there is roughly the same though. There is no pattern to which variant you get; it happens seemingly at random whether you are running with the tablet plugged in, or with the cable removed. But the program always crashes. $ gdb -ex r --args gtk3-demo GNU gdb (GDB) 7.12 Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-w64-mingw32". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from gtk3-demo...done. Starting program: C:\msys64\mingw64\bin\gtk3-demo.exe [New Thread 5204.0x1484] [New Thread 5204.0x13cc] [New Thread 5204.0x148c] [New Thread 5204.0x14b4] [New Thread 5204.0x1290] [New Thread 5204.0xd24] warning: WTContextManager() this:fb22e310 warning: WTRoundArray() this:fb22eb90 Thread 1 received signal SIGSEGV, Segmentation fault. gdk_event_translate (msg=msg@entry=0x22df20, ret_valp=ret_valp@entry=0x22df1c) at gdkevents-win32.c:2159 2159 (gdb) bt full
+ Trace 236854
Tested with gtk-3-22 HEAD as of d3bdd384a18edaf90eab0d387a08beb8e7044f62.
Bumped importance since it's a crasher.
Anything more I can do on this to help diagnose the fault?
Created attachment 339991 [details] [review] 0001-Delay-calling-wintab_init_check.patch Ugly code time. Just delaying the call to wintab_init_check() using g_idle_add() makes the crash go away. There are probably hundreds of better ways to do this, however. This is just a code sketch :) This patch allows gtk3-demo.exe to start, and when run with... demos/gtk-demo/gtk3-demo --gdk-debug=input --run=event_axes ... I see that gdk_input_other_event() is now being being called for both tablets I've tried with. The Huion tablet does not get as far as the "WINTAB motion: %g,%g\n" message in my mingw64 x86_64 build, and there is no pressure. But we already know that the behaviour is different between 32 and 64-bit for this model. At least I can experiment now! Unrelated to this bug, but for comparison with a more sensible driver: my Wacom Intuos 5, with its drivers carefully installed once again, provides pressure but no tilt at this point; after the delayed call to wintab_init_check(). I can see output that indicates that wintab_init_check() has not been missed. For tilt, please can the patch in bug 774265 be merged onto gtk-3-22 and master?
Um...if you have gdb and can catch the crash in action, what is stopping you from actually looking at the crash point to figure out what exactly goes wrong? From the backtraces you've posted, it seems that the crash happens in gdkevents-win32.c:2159 , which is (in gtk-3-22-master): keyboard_grab = _gdk_display_get_last_device_grab (display, device_manager_win32->core_keyboard); The only thing i can think of that can crash in this line is device_manager_win32 object being NULL. However, it would be nice to actually look at it when the crash happens to verify that it is indeed NULL. Maybe something else is happening. There's also the DangerousForbiddenTechnique of inspecting the stack to look at the Windows exception record, which might contain more specific information (i.e. error code, and maybe memory address, if it's an access violation).
Sufficient information is in the backtrace to know that it's dereferencing a NULL device_manager (and device_manager_win32). That's why I sent the full trace. My analysis: it's hardly surprising that gdk_display_get_device_manager() returned NULL given that this is happening within the initialization of the device manager, while opening the display. https://git.gnome.org/browse/gtk+/tree/gdk/win32/gdkdisplay-win32.c?h=gtk-3-22#n445 is both where the display manager gets created, and also where it gets assigned to _gdk_display->device_manager. Now g_object_new() calls gdk_device_manager_win32_constructed() after construction, because it is the handler for ->constructed. This calls wintab_init_check(). Now wintab_init_check() when it first runs tries to call WTOpenA() in wintab32.dll, and this can do arbitrary things. It's basically a black box. One of the things the Huion driver likes to do is fire arbitrary messages at GDK, which have to be handled. And the code which does that: https://git.gnome.org/browse/gtk+/tree/gdk/win32/gdkevents-win32.c?h=gtk-3-22#n239 Calls core GDK code which returns the value of _gdk_display->device_manager https://git.gnome.org/browse/gtk+/tree/gdk/gdkdisplay.c?h=gtk-3-22#n1553 which at this point is NULL because g_object_new() has not returned.
We could improve the code here to add more return value checking (gdk_display_get_device_manager() may return NULL if the device manager for the display is still being initialized), and that would be a good idea: https://git.gnome.org/browse/gtk+/tree/gdk/win32/gdkevents-win32.c?h=gtk-3-22#n2156 Nevertheless, given the *extreme* arbitrariness of what Wintab drivers might do at a critical time, I think it would be actively wise to delay calls to them until after all the GDK structures are fully initialized but before we might have to start dealing with motion events from them. The code for doing this would be simpler than changing init orders all over the place.
I agree. Either WTOpenA() shouldn't be called during device manager initialization, if it does things like this. Or Windows message handling in GDK should be amended to be more robust towards being called mid-initialization. The latter is kind of complicated (as we can't predict with 100% certainty that some things will or will not happen during initialization), so the right fix is probably the former. That said, improving message handling procedure is still something that's been on my mind, because currently it's a giant function that does 100 different, orthogonal things with quite extensive common state-bulding code (and current crash is, basically, due to state-bulding code trying to use uninitialized objects which are not even needed - the message that i see in the backtraces is 13, i.e. WM_GETTEXT, and GDK does not handle it anyway, so the code for obtaining keyboard grabs and whatnot is just busywork in this case).
I was going to add but got sidetracked. Moving the wintab stuff later to a time of our choosing should also be more robust than relying only on fixes elsewhere. Obvious place to hook it is on the "opened" signal of the GdkDisplay. I think Carlos has some ideas about making it a GInitable. I have a sneaking suspicion that deferring with g_idle_add() may cure a separate problem relating to which device "was used to start the program" that I occasionally see in my testing. (No reason not to do both, ofc.)
I definitely do not like the g_idle_add there... instead if doing the GInitiable works I'd be happy to review a patch :)
Connecting to the "opened" signal also causes crashes with this driver, which means that an immediate g_initiable_init() would crash too, since that would be executed even earlier. It's clearly going to require a lot more fiddling before it's right. (Following patch DOES NOT FIX this bug) -----------------8<---------------- diff --git a/gdk/win32/gdkdevicemanager-win32.c b/gdk/win32/gdkdevicemanager-win32.c index 62c164d..fd69d2e 100644 --- a/gdk/win32/gdkdevicemanager-win32.c +++ b/gdk/win32/gdkdevicemanager-win32.c @@ -348,9 +348,8 @@ print_cursor (int index) #endif static void -wintab_init_check (GdkDeviceManagerWin32 *device_manager) +wintab_init_check (GdkDisplay *display, GdkDeviceManagerWin32 *device_manager) { - GdkDisplay *display = gdk_device_manager_get_display (GDK_DEVICE_MANAGER (device_manager)); GdkWindow *root = gdk_screen_get_root_window (gdk_display_get_default_screen (display)); static gboolean wintab_initialized = FALSE; GdkDeviceWintab *device; @@ -727,7 +726,13 @@ gdk_device_manager_win32_constructed (GObject *object) gdk_seat_default_add_slave (GDK_SEAT_DEFAULT (seat), device_manager->system_keyboard); g_object_unref (seat); - wintab_init_check (device_manager); + /* Wintab initialization must only run after the g_object_new() calling + * this function returns because it can run arbitrary code which may + * itself send messages. This has caused many driver-specific crashes. + * See https://bugzilla.gnome.org/show_bug.cgi?id=774379 + */ + GdkDisplay *display = gdk_device_manager_get_display (GDK_DEVICE_MANAGER (device_manager)); + g_signal_connect(display, "opened", G_CALLBACK (wintab_init_check), device_manager); } static GList * ---------------------------->8----------------------- (Patch above DOES NOT FIX this bug.)
Further testing of clean-built packages to clarify what I mean in comment 15: Build Patchset i686 x86_64 --------------------------------- -------- ---------- ---------- Stock gtk3-3.22.1 None nopressure crashes Local gtk3-git-3.22.3+ 24f5d99 "idle" nopressure nopressure Local gtk3-git-3.22.3+ 24f5d99 "opened" nopressure crashes Stepping for a moment outside the question of where these crashes are occurring, I don't see how using GInitable would help. I may be misunderstanding how it works, of course.
That crash above with the patch from comment 15, for comparison. Test User@win7test MINGW64 / $ gdb -ex r --args gtk3-demo --gdk-debug=input --run=event_axes GNU gdb (GDB) 7.12 Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-w64-mingw32". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from gtk3-demo...done. Starting program: C:\msys64\mingw64\bin\gtk3-demo.exe "--gdk-debug=input" "--run=event_axes" [New Thread 1208.0xe14] [New Thread 1208.0x8fc] [New Thread 1208.0xe48] [New Thread 1208.0x9cc] [New Thread 1208.0xff0] warning: WTContextManager() this:fb8fe310 warning: WTRoundArray() this:fb8feb90 Wintab interface version 1.1 NDEVICES: 1, NCURSORS: 1 Device 0: HUION Tablet Note: Driver did not provide device specific default context info despite claiming to support version 1.1 Default context: lcName = HUION Tablet Context lcOptions = CXO_SYSTEM lcStatus = lcLocks = CXL_INSIZE CXL_INASPECT CXL_SENSITIVITY CXL_MARGIN lcMsgBase = 0x7ff0, lcDevice = 0, lcPktRate = 450 lcPktData = PK_TIME PK_CURSOR PK_BUTTONS PK_X PK_Y PK_NORMAL_PRESSURE PK_TANGENT_PRESSURE PK_ORIENTATION lcPktMode = lcMoveMask = PK_TIME PK_CURSOR PK_BUTTONS PK_X PK_Y PK_NORMAL_PRESSURE PK_TANGENT_PRESSURE PK_ORIENTATION lcBtnDnMask = 0xffffffff, lcBtnUpMask = 0xffffffff lcInOrgX = 0, lcInOrgY = 0, lcInOrgZ = 0 lcInExtX = 40000, lcInExtY = 25000, lcInExtZ = 0 lcOutOrgX = 0, lcOutOrgY = 0, lcOutOrgZ = 0 lcOutExtX = 1920, lcOutExtY = 1080, lcOutExtZ = 0 lcSensX = 1, lcSensY = 1, lcSensZ = 1 lcSysMode = 0 lcSysOrgX = 0, lcSysOrgY = 0 lcSysExtX = 1920, lcSysExtY = 1080 lcSysSensX = 1, lcSysSensY = 1 context for device 0: lcName = HUION Tablet Context lcOptions = CXO_SYSTEM CXO_MESSAGES CXO_CSRMESSAGES lcStatus = lcLocks = CXL_INSIZE CXL_INASPECT CXL_SENSITIVITY CXL_MARGIN lcMsgBase = 0x7ff0, lcDevice = 0, lcPktRate = 0 lcPktData = PK_CONTEXT PK_CURSOR PK_BUTTONS PK_X PK_Y PK_NORMAL_PRESSURE PK_ORIENTATION lcPktMode = lcMoveMask = PK_CONTEXT PK_CURSOR PK_BUTTONS PK_X PK_Y PK_NORMAL_PRESSURE PK_ORIENTATION lcBtnDnMask = 0xffffffff, lcBtnUpMask = 0xffffffff lcInOrgX = 0, lcInOrgY = 0, lcInOrgZ = 0 lcInExtX = 40000, lcInExtY = 25000, lcInExtZ = 0 lcOutOrgX = 0, lcOutOrgY = 0, lcOutOrgZ = 0 lcOutExtX = 40001, lcOutExtY = -25001, lcOutExtZ = 0 lcSensX = 1, lcSensY = 1, lcSensZ = 1 lcSysMode = 0 lcSysOrgX = 0, lcSysOrgY = 0 lcSysExtX = 1920, lcSysExtY = 1080 lcSysSensX = 1, lcSysSensY = 1 Thread 1 received signal SIGSEGV, Segmentation fault. gdk_event_translate (msg=msg@entry=0x22ddf0, ret_valp=ret_valp@entry=0x22ddec) at gdkevents-win32.c:2159 warning: Source file is more recent than executable. 2159 keyboard_grab = _gdk_display_get_last_device_grab (display, (gdb) bt full
+ Trace 236864
Created attachment 340218 [details] [review] 0001-Fix-segfault-during-init-for-some-wintab32-DLLs.patch For now, just avoid dereferencing anything that might be NULL to handle the Huion driver's special oddities. This patch fixes my mingw64 x86_64 test build here, and I'll confirm that it doesn't break i686 or my Intuos when I have fully built packages. I still think the WTOpenA code should be deferred after all of the initialization's done, but I don't really know when would be right for that. You can't really know. g_idle_add() seems to work around the race condition for this tablet, but it's a very blunt tool. And seeing WM_GETTEXT seems very odd to me; what does it expect? The Huion H610PRO still has a regression on handling of its pressure axis, but that can now be investigated properly.
Review of attachment 340218 [details] [review]: See the comments ::: gdk/win32/gdkevents-win32.c @@ +2165,1 @@ device_manager_win32 = GDK_DEVICE_MANAGER_WIN32 (device_manager); I think you should not make this cast if device_manager is NULL, you would get most probably a warning here @@ +2166,3 @@ + if (device_manager_win32 == NULL) + { + GDK_NOTE (EVENTS, g_print (" (no GdkDeviceManagerWin32)")); there is no need to log this, you are already logging the previous one which is the same @@ +2171,1 @@ + if (display && device_manager_win32) be specific and check for NULL i.e display != NULL && device_manager_win32 != NULL
Created attachment 340242 [details] [review] 0001-Fix-segfault-during-init-for-some-wintab32-DLLs.patch Okay, hopefully this is better. It compiles. Will test later, and make sure it doesn't break working tablets.
attachment 340242 [details] [review] is ready. Tested against gtk-3-22, tag 3.22.4. It fixes the crash for the Huion and its drivers, although pressure is still broken there. It does not break the Wacom with its drivers. Please merge onto master and gtk-3-22 ASAP, and please consider making an early 3.22.5 to fix this critical bug. I will report the pressure bug separately.
The pressure bug also turned out to be something more serious: bug 774699
Review of attachment 340242 [details] [review]: Marking as reviewed so far, the approach looks good to me but I've got a code-related question below. Other misc questions: I take it this patch leaves the other attachment obsolete? Didn't the "delay wintab initialization until GdkDisplay::opened" approach fly? ::: gdk/win32/gdkevents-win32.c @@ +2179,3 @@ + { + keyboard_grab = _gdk_display_get_last_device_grab (display, device_manager_win32->core_keyboard); + pointer_grab = _gdk_display_get_last_device_grab (display, device_manager_win32->core_pointer); This looks like a reasonable assumption to do, if there's no devices yet, there's no ongoing grabs. However, the "might do anything" comment in the patch log makes me a bit wary. Are we expecting input events too in this phase or anything related to devices? In that case it seems like there could be stability issues further down event propagation.
I think we're agreed that g_idle_add() isn't right, but there's no patch up for review yet that actually obsoletes it. I think that the *approach* is valid, and that this is the best patch on this bug which expresses the idea of deferring the wintab init. I stand behind attachment 340242 [details] [review], those checks are a good to get in. If there's no default display or device manager, can there be a grab? No, not really. I've not seen any input-type messages (case WT_PACKET) during debugging, but I can't rule them out either. Only things we don't handle, presumably WT_* stuff which I didn't look up.
Carlos: merely moving _gdk_events_init() after the g_object_new(GDK_TYPE_DEVICE_MANAGER_WIN32) does not fix the crash.
Created attachment 340324 [details] [review] GDK W32: Don't assume that device_manager is never NULL This patch adds more device_manager != NULL checks to the event processing. In conjunction with the code that doesn't try to dereference NULL device_manager when trying to get keyboard/pointer grabs, it should be enough to be reasonably safe. Note that i didn't check deeper function calls, so one of them *could* be accessing device manager, and this is really a never-ending problem - we will basically have to ensure that no part of W32 GDK ever assumes that device manager is available, which seems very inconvenient. So i'm definitely cheering for you, guys, figuring out how to delay WTOpenA() until device manager is initialized.
Carlos: the patch you showed me on IRC earlier also doesn't make the crash go away in the way that g_idle_add() does. I'm appending it here as an inline bit of code so we don't forget about it (perhaps do it async?) Again, these are local debug builds which for some reason I cannot push through gdb. That takes a full debugging package generation, no idea why. ----------------8<------------------- diff --git a/gdk/win32/gdkdevicemanager-win32.c b/gdk/win32/gdkdevicemanager-win32.c index 62c164d..76a2eef 100644 --- a/gdk/win32/gdkdevicemanager-win32.c +++ b/gdk/win32/gdkdevicemanager-win32.c @@ -66,7 +66,11 @@ static t_WTOverlap p_WTOverlap; static t_WTPacket p_WTPacket; static t_WTQueueSizeSet p_WTQueueSizeSet; -G_DEFINE_TYPE (GdkDeviceManagerWin32, gdk_device_manager_win32, GDK_TYPE_DEVICE_MANAGER) +static void gdk_device_manager_win32_initable_iface_init (GInitableIface *iface); + +G_DEFINE_TYPE_WITH_CODE (GdkDeviceManagerWin32, gdk_device_manager_win32, + GDK_TYPE_DEVICE_MANAGER, + G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, gdk_device_manager_win32_initable_iface_init)) static GdkDevice * create_pointer (GdkDeviceManager *device_manager, @@ -726,8 +730,6 @@ gdk_device_manager_win32_constructed (GObject *object) gdk_seat_default_add_slave (GDK_SEAT_DEFAULT (seat), device_manager->system_pointer); gdk_seat_default_add_slave (GDK_SEAT_DEFAULT (seat), device_manager->system_keyboard); g_object_unref (seat); - - wintab_init_check (device_manager); } static GList * @@ -785,6 +787,21 @@ gdk_device_manager_win32_class_init (GdkDeviceManagerWin32Class *klass) device_manager_class->get_client_pointer = gdk_device_manager_win32_get_client_pointer; } +static gboolean +gdk_device_manager_win32_initable_init (GInitable *initable, + GCancellable *cancellable, + GError **error) +{ + wintab_init_check (GDK_DEVICE_MANAGER_WIN32 (initable)); + return TRUE; +} + +static void +gdk_device_manager_win32_initable_iface_init (GInitableIface *iface) +{ + iface->init = gdk_device_manager_win32_initable_init; +} + void _gdk_input_set_tablet_active (void) { diff --git a/gdk/win32/gdkdisplay-win32.c b/gdk/win32/gdkdisplay-win32.c index ceec5b3..db6d479 100644 --- a/gdk/win32/gdkdisplay-win32.c +++ b/gdk/win32/gdkdisplay-win32.c @@ -445,6 +445,7 @@ _gdk_win32_display_open (const gchar *display_name) _gdk_display->device_manager = g_object_new (GDK_TYPE_DEVICE_MANAGER_WIN32, "display", _gdk_display, NULL); + g_initable_init (G_INITABLE (_gdk_display->device_manager), NULL, NULL); _gdk_dnd_init ();
In this case it's probably trying to access an unassigned default display too. The async approach has to be the right idea, if we can assure ourselves that it will run when both have been assigned.
Created attachment 340334 [details] [review] wintab: init only after the display is assigned Getting the display manager to tell us when the default display is set for the first time may be the right approach here. gtkmodules.c does pretty much the same thing. This patch fixes the segfault when applied on top of unpatched gtk-2-33 HEAD. I'll test again more thoroughly when I have a proper build ready at my end.
[[This comment is duplicated into the 3 active Wintab bugs, sorry for the spam]] Okay, big testing rollup. Incorporating the following patches was *VERY* positive, getting rid of all inti-time crashes and almost all missing pressure. Tilt and eraser are back online and looking good for more advanced tablets. * bug 774699 comment 1 "0001-wintab-fix-skipping-of-odd-numbered-devices.patch" * bug 774379 comment 29 "0001-wintab-init-only-after-the-display-is-assigned.patch" * bug 774379 comment 20 "0001-Fix-segfault-during-init-for-some-wintab32-DLLs.patch" * Sorry LRN, your patch from bug 774379 comment 26 didn't apply cleanly onto the stable branch (gtk-3-22) alongside all my patches. I was not able to test it. * bug 774265 comment 12 "0001-win32-Fix-tilt-from-Wintab-devices.patch" I made a debug build and installation on MSYS2 of all the software needed, for both i686 and x86_64, and tested them all from their appropriate MSYS2 shell. Arch Tablet model Drivers gtk3-demo mypaint-git ---- ------------- ------- --------- ------------ x86_64 Huion H610PRO 12.2.16 OK OK i686 Huion H610PRO 12.2.16 OK OK x86_64 Wacom Intuos 5 M (PTH-650) 6.3.18-5 OK +T +E OK +T +E i686 Wacom Intuos 5 M (PTH-650) 6.3.18-5 OK +T +E OK +T +E x86_64 Genius EasyPen i405X (GT-100007) 2.5.1.1* OK OK i686 Genius EasyPen i405X (GT-100007) 2.5.1.1* NOEVENTS NOEVENTS My system is AMD64, so it is possible that the Genius driver installers made a decision about what to install for me. It is possible we've seen this behaviour before in MyPaint-land, so the lone failure above isn't a worry for me right now. Key --- NOEVENTS Device presents only as "System Aggregated Pointer", and does not send events with pressure. Evidence of wintab32.dll being loaded by wintab_init_check, though! OK Device presents as something other than "System Aggregated Pointer", and sends events with pressure. +T Device also correctly presents tilt. +E Device also presents the eraser end of the stylus as a named device. * Drivers are branded "ioTablet", manufacturer "KYE", but were downloaded from geniusnet.com as recommended.
For this one, using GInitable seems to be too complex an approach. It can be done with a signal from the display manager, so it should be unless there's a real technical reason not to connect or process rare signals inside of GDK. If those assertions ever trigger, they will indicate that we need to rethink this approach. But that's a problem for future GTK, I am happy for now. Connecting to "notify::default-display" and doing the wintab init there *by itself* fixes this bug. The other remaining attached patches are all nice to have too. Can the current set of remaining patches please be reviewed and pushed to both master and stable (gtk-3-22) ASAP, before the next release? Thanks.
Comment on attachment 340334 [details] [review] wintab: init only after the display is assigned Looks good to me.
Comment on attachment 340324 [details] [review] GDK W32: Don't assume that device_manager is never NULL Makes sense, although I'd hope this is not necessary anymore with the other patch, as this is trying to recover from a pretty broken situation... I'm marking as "reviewed" and will let the win32 maintainers decide whether they prefer the added resilience or not.
Review of attachment 340324 [details] [review]: This applies cleanly enough onto stable master in the absence of all the other patches floating around, but it does not compile: ------------------8<----------------- [...] make[5]: Entering directory '/usr/src/gtk-mingw64/gdk/win32' CC gdkevents-win32.lo In file included from gdkprivate-win32.h:44:0, from gdkevents-win32.c:44: gdkevents-win32.c: In function '_gdk_win32_window_procedure': gdkevents-win32.c:284:30: warning: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'WPARAM {aka long long unsigned int}' [-Wformat=] GDK_NOTE (EVENTS, g_print ("%s%*s%s %p %#x %#lx", ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c:284:30: warning: format '%lx' expects argument of type 'long unsigned int', but argument 8 has type 'LPARAM {aka long long int}' [-Wformat=] GDK_NOTE (EVENTS, g_print ("%s%*s%s %p %#x %#lx", ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c: In function 'find_window_for_mouse_event': gdkevents-win32.c:471:3: warning: 'gdk_display_get_device_manager' is deprecated: Use 'gdk_display_get_default_seat' instead [-Wdeprecated-declarations] device_manager = GDK_DEVICE_MANAGER_WIN32 (gdk_display_get_device_manager (display)); ^~~~~~~~~~~~~~ In file included from ../../gdk/gdkscreen.h:32:0, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdisplay.h:171:20: note: declared here GdkDeviceManager * gdk_display_get_device_manager (GdkDisplay *display); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdkevents-win32.c: In function 'send_crossing_event': gdkevents-win32.c:1129:3: warning: 'gdk_display_get_device_manager' is deprecated: Use 'gdk_display_get_default_seat' instead [-Wdeprecated-declarations] device_manager = GDK_DEVICE_MANAGER_WIN32 (gdk_display_get_device_manager (display)); ^~~~~~~~~~~~~~ In file included from ../../gdk/gdkscreen.h:32:0, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdisplay.h:171:20: note: declared here GdkDeviceManager * gdk_display_get_device_manager (GdkDisplay *display); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdkevents-win32.c: In function 'generate_button_event': gdkevents-win32.c:1716:3: warning: 'gdk_display_get_device_manager' is deprecated: Use 'gdk_display_get_default_seat' instead [-Wdeprecated-declarations] device_manager = GDK_DEVICE_MANAGER_WIN32 (gdk_display_get_device_manager (gdk_display_get_default ())); ^~~~~~~~~~~~~~ In file included from ../../gdk/gdkscreen.h:32:0, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdisplay.h:171:20: note: declared here GdkDeviceManager * gdk_display_get_device_manager (GdkDisplay *display); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from gdkprivate-win32.h:44:0, from gdkevents-win32.c:44: gdkevents-win32.c: In function 'handle_wm_sysmenu': gdkevents-win32.c:1984:30: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'LONG_PTR {aka long long int}' [-Wformat=] GDK_NOTE (EVENTS, g_print (" Handling WM_SYSMENU: style 0x%lx -> 0x%lx\n", style, tmp_style)); ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c:1984:30: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'LONG_PTR {aka long long int}' [-Wformat=] GDK_NOTE (EVENTS, g_print (" Handling WM_SYSMENU: style 0x%lx -> 0x%lx\n", style, tmp_style)); ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c:1994:30: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'LONG_PTR {aka long long int}' [-Wformat=] GDK_NOTE (EVENTS, g_print (" Handling WM_SYSMENU: style 0x%lx <- 0x%lx\n", style, tmp_style)); ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c:1994:30: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'LONG_PTR {aka long long int}' [-Wformat=] GDK_NOTE (EVENTS, g_print (" Handling WM_SYSMENU: style 0x%lx <- 0x%lx\n", style, tmp_style)); ^ ../../gdk/gdkinternals.h:121:10: note: in definition of macro 'GDK_NOTE' { action; }; } G_STMT_END ^~~~~~ gdkevents-win32.c: In function 'gdk_event_translate': gdkevents-win32.c:2156:3: warning: 'gdk_display_get_device_manager' is deprecated: Use 'gdk_display_get_default_seat' instead [-Wdeprecated-declarations] device_manager = gdk_display_get_device_manager (display); ^~~~~~~~~~~~~~ In file included from ../../gdk/gdkscreen.h:32:0, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdisplay.h:171:20: note: declared here GdkDeviceManager * gdk_display_get_device_manager (GdkDisplay *display); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdkevents-win32.c:3176:12: error: 'window_manager' undeclared (first use in this function) (window_manager != NULL)) ^~~~~~~~~~~~~~ gdkevents-win32.c:3176:12: note: each undeclared identifier is reported only once for each function it appears in gdkevents-win32.c:3178:9: warning: 'gdk_device_manager_get_client_pointer' is deprecated [-Wdeprecated-declarations] GdkDevice *device = gdk_device_manager_get_client_pointer (device_manager); ^~~~~~~~~ In file included from ../../gdk/gdkdisplay.h:32:0, from ../../gdk/gdkscreen.h:32, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdevicemanager.h:44:14: note: declared here GdkDevice * gdk_device_manager_get_client_pointer (GdkDeviceManager *device_manager); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdkevents-win32.c:3182:11: warning: 'gdk_device_ungrab' is deprecated: Use 'gdk_seat_ungrab' instead [-Wdeprecated-declarations] gdk_device_ungrab (device, msg -> time); ^~~~~~~~~~~~~~~~~ In file included from ../../gdk/gdkdnd.h:33:0, from ../../gdk/gdkevents.h:34, from ../../gdk/gdkdisplay.h:31, from ../../gdk/gdkscreen.h:32, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdevice.h:239:15: note: declared here void gdk_device_ungrab (GdkDevice *device, ^~~~~~~~~~~~~~~~~ gdkevents-win32.c:3546:9: warning: 'gdk_device_manager_get_client_pointer' is deprecated [-Wdeprecated-declarations] GdkDevice *device = gdk_device_manager_get_client_pointer (device_manager); ^~~~~~~~~ In file included from ../../gdk/gdkdisplay.h:32:0, from ../../gdk/gdkscreen.h:32, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdevicemanager.h:44:14: note: declared here GdkDevice * gdk_device_manager_get_client_pointer (GdkDeviceManager *device_manager); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdkevents-win32.c:3547:9: warning: 'gdk_device_ungrab' is deprecated: Use 'gdk_seat_ungrab' instead [-Wdeprecated-declarations] gdk_device_ungrab (device, msg -> time); ^~~~~~~~~~~~~~~~~ In file included from ../../gdk/gdkdnd.h:33:0, from ../../gdk/gdkevents.h:34, from ../../gdk/gdkdisplay.h:31, from ../../gdk/gdkscreen.h:32, from ../../gdk/gdkapplaunchcontext.h:31, from ../../gdk/gdk.h:32, from ../../gdk/gdkprivate.h:28, from gdkprivate-win32.h:37, from gdkevents-win32.c:44: ../../gdk/gdkdevice.h:239:15: note: declared here void gdk_device_ungrab (GdkDevice *device, ^~~~~~~~~~~~~~~~~ make[5]: *** [Makefile:789: gdkevents-win32.lo] Error 1 make[5]: Leaving directory '/usr/src/gtk-mingw64/gdk/win32' make[4]: *** [Makefile:850: all-recursive] Error 1 make[4]: Leaving directory '/usr/src/gtk-mingw64/gdk/win32' make[3]: *** [Makefile:1636: all-recursive] Error 1 make[3]: Leaving directory '/usr/src/gtk-mingw64/gdk' make[2]: *** [Makefile:1087: all] Error 2 make[2]: Leaving directory '/usr/src/gtk-mingw64/gdk' make[1]: *** [Makefile:721: all-recursive] Error 1 make[1]: Leaving directory '/usr/src/gtk-mingw64' make: *** [Makefile:615: all] Error 2
(Sorry for the duplicate)
Review of attachment 340334 [details] [review]: See the style issues ::: gdk/win32/gdkdevicemanager-win32.c @@ +683,3 @@ } + no need for 2 lines empty @@ +692,3 @@ + * https://bugzilla.gnome.org/show_bug.cgi?id=774379 + */ + no need for this empty line @@ +700,3 @@ + + if (default_display_opened) + { no need for {} if there is only one line @@ +715,3 @@ +} + + do not leave 2 empty lines together @@ -730,1 +765,6 @@ - wintab_init_check (device_manager); + /* Only call Wintab init stuff after the default display + * is globally known and accessible through the display manager + * singleton. Approach lifted from gtkmodules.c. ... 3 more ... declare at the start of the block
Created attachment 340513 [details] [review] 0001-wintab-init-only-after-the-display-is-assigned.patch Nacho: thanks for the review. I've applied your suggested changes. Building for test now.
Comment on attachment 340513 [details] [review] 0001-wintab-init-only-after-the-display-is-assigned.patch Now that this patch is pushed do we really need the others? As Carlos said I'd rather not have to check for the NULL on all the places we make use of the device manager.
I can confirm that the revised patch in comment 38 fixes the crash, on the stable gtk-3-22 branch. Thanks for the commits, nacho :) The reviewed patch in comment 21 is probably a good idea. We evidently can't be certain about messages coming in before the default display is set up on the Windows platform. I'll reword the commit message though, since it is no longer a "fix" for anything - just a safety measure.
Created attachment 340516 [details] [review] 0001-win32-event-check-for-NULL-display-or-dev-mgr.patch Reworded the commit message to make it clearer that this is a safety measure.
I think I'd consider this one fixed if that last patch were merged. Tests out fine on one of my weird tablets here :)
Comment on attachment 340516 [details] [review] 0001-win32-event-check-for-NULL-display-or-dev-mgr.patch I think looks good :)