Some of my code was giving an error code in a GError which was out of range for the enum that I'd registered for translation. So gerror_domaincode_to_dbus_error_name() crashed at about line 1351 (code_str = value->value_nick) because 'value' was NULL. We should check for that and cope. Hell, even if we make it an assert() and die, as long as we give a sane error message it would make life a lot easier for people who cause it. A NULL-dereferencing crash deep in dbus-glib is no fun to debug.
Created attachment 50287 [details] [review] Avoid crash on translation failure
I'd prefer this with a g_warning() and explicitly setting code_str to NULL, in the value == NULL case (code_str is provably NULL already, but relying on that for correctness doesn't help legibility). "Code %d out of range for GError domain %s" where the %s is the result of g_quark_to_string(), perhaps? I think g_warning() is probably the right amount of warning - getting an out-of-range value in an enum seems too insignificant for a g_critical() - but g_critical() would be fine too.
(In reply to comment #1) > Created an attachment (id=50287) [details] > Avoid crash on translation failure In my branch I committed this as "Don't crash in error_domaincode_to_dbus_error_name if code is out of range", which I think is more self-describing.
Created attachment 51722 [details] [review] [2/10] If an error code is out of range for its domain, warn about it
Created attachment 51723 [details] [review] [3/10] Form a valid D-Bus error name if an unmapped error has a negative code --- My first attempt at a regression test for Attachment #50287 [details] found this bug...
Created attachment 51724 [details] [review] [4/10] Add copyright/licensing information to test-dbus-glib I've tried to dig up the copyright holders from git history; possibly incomplete, but none of them cared enough to add their own copyright notices, so this is the best we'll get.
Created attachment 51725 [details] [review] [5/10] test-dbus-glib.c isn't GTest yet, but add bug numbers anyway
Created attachment 51726 [details] [review] [6/10] MyObject: make ThrowError, AsyncThrowError throw a caller-specified error This can be used to test arbitrary errors, but only in-process; in tests with the service out-of-process, like test-dbus-glib, the initial error (matching the error they previously threw) will always be used. This obsoletes ThrowErrorUnderscore, ThrowErrorMultiWord and ThrowNotSupported, but not ThrowUnregisteredError due to some strange assumptions about the validity of GError domains in that method (see GNOME#660731).
Created attachment 51727 [details] [review] [7/10] Add a new test for error mapping
Created attachment 51728 [details] [review] [8/10] Remove tests in test-dbus-glib which basically just test error mapping Also remove the methods on MyObject that only existed to support these tests; ThrowError is now versatile enough to implement them all. Leave ThrowUnregisteredError as it is, since it violates GError expectations (see GNOME#660371), but stop using it in test-dbus-glib - it's enough to use it in test-error-mapping.
Created attachment 51729 [details] [review] [9/10] Add a manual test for various invalid behaviour Most of this has been sitting in a branch since fd.o #30171; fixing fd.o #40151, another case of library-user error leading to undefined behaviour and a hard-to-diagnose crash, seems a good time to get this merged.
Created attachment 51730 [details] [review] [10/10] Move use of 0 as an error domain into the invalid-usage test I think this is invalid, although others might disagree. --- (If you agree or disagree, please do so on <https://bugzilla.gnome.org/show_bug.cgi?id=660371>.)
(In reply to comment #12) > I think this is invalid, although others might disagree. GLib upstream agree with me, and GLib 2.32 warns about this usage.
This branch is now a release blocker, because the warning that was added in GLib 2.32 causes the tests to fail (the test service runs with fatal warnings, and crashes). Someone please review this?
These patches all look reasonable to me.
(In reply to comment #15) > These patches all look reasonable to me. Thanks, fixed in git for 0.100.
Created attachment 61312 [details] [review] Fix the build with --disable-tests
(In reply to comment #17) > Fix the build with --disable-tests Sure, r+
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.