| Summary: | dbus_connection_setup_with_g_main cannot be used to switch contexts | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Mike Gorse <mgorse> |
| Component: | GLib | Assignee: | D-Bus Maintainers <dbus> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | CC: | bugzilla, yanhai.zhu |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Test case.
Proposed patch. Updated patch. New patch with test included. Patch to remove assert. Revised patch with a test. |
||
Created attachment 39126 [details] [review] Proposed patch. Created attachment 39282 [details] [review] Updated patch. Remove an assert which can fail if a main loop context was already set, and document behavior. Could the test case be added to the dbus-glib test infrastructure, maybe? The patch looks fine, though. Created attachment 39626 [details] [review] New patch with test included. Will committed this for dbus-glib 0.90; marking resolved. *** Bug 29663 has been marked as a duplicate of this bug. *** My original patch removed an assert from connection_setup_add_watch that would fail when switching a connection to a new main loop. I made a copy of this code in at-spi2-core (partly because I needed a fix that did not yet have a dbus-glib release and partly to avoid depending on dbus-glib, since it is deprecated, but I still need the *_setup_wht_g_main code), and, anyway, for a while I have been carrying around a similar change to connection_setup_add_timeout. I think that the code should just not asssert that the timeout's data is NULL, since it will not be NULL if the timeout is being moved from one connection to the other, and in that case it will be removed from the previous connection as a side-effect of freeing the data, which will be done when new data is added. So going to re-open this and attach a patch. Created attachment 66420 [details] [review] Patch to remove assert. (In reply to comment #7) > My original patch removed an assert from connection_setup_add_watch that > would fail when switching a connection to a new main loop. In what situation does this assertion fail? Could you please add a test case that exercises this? (The one on Bug #23633, which I'll review now, might be a good starting point.) In general I tend to assume that when there's an assertion, it's there for a reason: either something "can't happen", or the rest of the code assumes the assertion to be true and will be buggy if the assertion fails. On the other hand, this is dbus-glib, where "I can't believe this ever worked" is a recurring motto. So who knows really. Created attachment 70395 [details] [review] Revised patch with a test. From what I remember, I was seeing the assert intermittently and not very often, so I'm not sure how I was triggering it; presumably I was migrating a DBusConnection from one main loop context to another when a timeout was present. This is presumably intended to work, since connection_setup_new_from_old explicitly migrates watches and timeouts to the new main loop context, so, in any case, I think that it makes sense to test it, and this test reliably generates the assert and aborts if it is not removed. I also refactored the test some to plug some memory leaks and generally clean it up--the send_test_message(TRUE) is the new test. Could I get a review on the patch in comment 10? I more or less have a c&p of dbus-gmain.c in at-spi2-core because of crashes caused by this assert, I already removed a similar assert a few years ago from dbus_connection_add_watch (see comment 4 on this bug), and I've been asked to remove the duplicate code in at-spi2-core (see https://bugzilla.gnome.org/show_bug.cgi?id=710630). Your test more or less convinces me that this is both intentional and functional. Fixed in 0.104. |
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.
Created attachment 39125 [details] Test case. dbus_connection_setup_with_g_main and dbus_serer_setup_with_g_main are intended to switch the setup to a new main loop context if they are called with a context other than the one with which they were initially called. However, this does not work in practice. The problem has to do with libdbus maintaining a list of watches associated with the connection. When dbus_connection_set_watch_functions is called, it first adds the existing watches using the new function and data, then removes the watches using the old function and data. Remove_watch calls connection_setup_remove_watch, and the latter does not check that the watch's connection setup matches the connection setup passed to it, so it winds up removing the watch from the new connection setup. (The watch has already been removed from the old setup at this point, as a side effect of dbus_watch_set_data).