|
Description
Simon McVittie
2014-03-10 20:39:29 UTC
Created attachment 95534 [details] [review] textchan-group: improve debug Created attachment 95535 [details] [review] Use concrete dbus-glib parameterized types, not G_TYPE_VALUE_ARRAY dbus_g_value_build_g_variant() doesn't yet understand how to build a tuple from a G_TYPE_VALUE_ARRAY, so my branch to use GDBus for everything will rely on using the full parameterized types. Created attachment 95536 [details] [review] TpContact: mime_file_written: don't leak the file's path --- Might be a bugfix for master/stable also, I haven't checked. Could someone else? I'm busy shaving this yak. Created attachment 95537 [details] [review] TpTestsContactsConnection: don't leak list_manager Created attachment 95538 [details] [review] Ignore allocations in gobject_init_ctor Created attachment 95539 [details] [review] TpBaseContactList: don't leak groups hash table --- Not tested with any real CMs. Someone probably should before this reaches master, just to be safe. Created attachment 95540 [details] [review] TpProxy: finish_all_requests: don't leak copied GQueue Created attachment 95541 [details] [review] account test: don't leak copied strings Created attachment 95542 [details] [review] TpTestsSimpleChannelDispatcher: don't leak various struct members Created attachment 95543 [details] [review] cli-group test: don't leak GMainLoop Created attachment 95544 [details] [review] channel-introspect test: enable debug-logging Created attachment 95545 [details] [review] tests: extend timeout 10 seconds is too short under valgrind, and this is only really a safety-catch against never terminating. Created attachment 95546 [details] [review] GDBus can deliver more than one event per main-loop iteration --- The comment + * We shouldn't assert len == 1 above because this happens automatically, + * and when we do a _run_ call, GDBus can give us than one + * event per main loop iteration (dbus-glib went to some lengths + * not to do so). */ should maybe move further up the file; or not. This is the first of the commits that should maybe wait until we merge the GDBus branch. Created attachment 95547 [details] [review] dbus test: don't re-enter main loop to request/release names GDBus guarantees that it will alternate signalling names appearing and vanishing, and this reentrant usage probably breaks that. It was a terrible idea anyway. --- The commit message is maybe a little misleading. It still re-enters the main loop, but it re-enters in an idle, not in the name ownership change callback, which is less bad. I didn't really feel like rewriting this to use proper calls right now, the diffstat for this branch is terrifying anyway. Created attachment 95548 [details] [review] invalidated-while-invoking-signals test: move to GTest Created attachment 95549 [details] [review] invalidated-while-invoking-signals test: extend test coverage Since commit 6f153bca, we didn't wait for on_status_changed() to occur. At some point it ceased to be reached at all, which meant that commit db582a0d added an unbalanced unref for @client in what has now become teardown(). If on_status_changed() was executed, it would unref the client, and then the unref in teardown() would either be a use-after-free or indirectly lead to one. Porting telepathy-glib to GDBus exposed this bug, by making on_status_changed() reachable again. While fixing this I noticed that on_status_changed() is not guaranteed to be the last-unref, so the test does not necessarily even reproduce the original crash situation, which was the proxy being invalidated inside the signal callback. I've addressed that by testing once with the way the test has worked in practice since at least 2012, and once with explicit forced invalidation. Created attachment 95551 [details] [review] tp_tests_assert_last_unref: add Some tests find this functionality useful. --- I might omit this one if I discover that nothing actually needs it by the time I'm ready to land the GDBus branch. Comment on attachment 95534 [details] [review] textchan-group: improve debug Review of attachment 95534 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95535 [details] [review] Use concrete dbus-glib parameterized types, not G_TYPE_VALUE_ARRAY Review of attachment 95535 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95536 [details] [review] TpContact: mime_file_written: don't leak the file's path Review of attachment 95536 [details] [review]: ----------------------------------------------------------------- ++ master and stable too as well if needed. Comment on attachment 95537 [details] [review] TpTestsContactsConnection: don't leak list_manager Review of attachment 95537 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95538 [details] [review] Ignore allocations in gobject_init_ctor Review of attachment 95538 [details] [review]: ----------------------------------------------------------------- ok. Comment on attachment 95539 [details] [review] TpBaseContactList: don't leak groups hash table Review of attachment 95539 [details] [review]: ----------------------------------------------------------------- ++ stable/master as well. Comment on attachment 95540 [details] [review] TpProxy: finish_all_requests: don't leak copied GQueue Review of attachment 95540 [details] [review]: ----------------------------------------------------------------- ++ stable/master too. Comment on attachment 95541 [details] [review] account test: don't leak copied strings Review of attachment 95541 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95542 [details] [review] TpTestsSimpleChannelDispatcher: don't leak various struct members Review of attachment 95542 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95543 [details] [review] cli-group test: don't leak GMainLoop Review of attachment 95543 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95544 [details] [review] channel-introspect test: enable debug-logging Review of attachment 95544 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95545 [details] [review] tests: extend timeout Review of attachment 95545 [details] [review]: ----------------------------------------------------------------- ok Comment on attachment 95546 [details] [review] GDBus can deliver more than one event per main-loop iteration Review of attachment 95546 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95547 [details] [review] dbus test: don't re-enter main loop to request/release names Review of attachment 95547 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95548 [details] [review] invalidated-while-invoking-signals test: move to GTest Review of attachment 95548 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 95551 [details] [review] tp_tests_assert_last_unref: add Review of attachment 95551 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #21) > TpTestsContactsConnection: don't leak list_manager This was in fact incorrect: in 0.22, create_channel_managers() returns the ref, leaving list_manager as a borrowed ref to something tracked by the base class. That seems poor style, but is valid. It's possible that this changed between 0.22 and next. (In reply to comment #6) > TpBaseContactList: don't leak groups hash table In 0.22, this is OK, but redundant: the same function already does a tp_clear_pointer(). Again, perhaps this changed between 0.22 and next. Comment on attachment 95534 [details] [review] textchan-group: improve debug 0.99.8 Comment on attachment 95535 [details] [review] Use concrete dbus-glib parameterized types, not G_TYPE_VALUE_ARRAY 0.99.8 Comment on attachment 95536 [details] [review] TpContact: mime_file_written: don't leak the file's path 0.22.2, 0.23.3, 0.99.8 Comment on attachment 95538 [details] [review] Ignore allocations in gobject_init_ctor 0.99.8 Comment on attachment 95540 [details] [review] TpProxy: finish_all_requests: don't leak copied GQueue 0.22.2, 0.23.3, 0.99.8 Comment on attachment 95541 [details] [review] account test: don't leak copied strings 0.99.8 Comment on attachment 95542 [details] [review] TpTestsSimpleChannelDispatcher: don't leak various struct members 0.99.8 Comment on attachment 95543 [details] [review] cli-group test: don't leak GMainLoop 0.99.8 Comment on attachment 95544 [details] [review] channel-introspect test: enable debug-logging 0.99.8 Comment on attachment 95545 [details] [review] tests: extend timeout 0.99.8 Comment on attachment 95546 [details] [review] GDBus can deliver more than one event per main-loop iteration 0.99.8 Comment on attachment 95547 [details] [review] dbus test: don't re-enter main loop to request/release names 0.99.8 Comment on attachment 95548 [details] [review] invalidated-while-invoking-signals test: move to GTest 0.99.8 Comment on attachment 95549 [details] [review] invalidated-while-invoking-signals test: extend test coverage 0.99.8 Comment on attachment 95551 [details] [review] tp_tests_assert_last_unref: add 0.99.8 (In reply to comment #35) > (In reply to comment #6) > > TpBaseContactList: don't leak groups hash table > > In 0.22, this is OK, but redundant: the same function already does a > tp_clear_pointer(). Again, perhaps this changed between 0.22 and next. It did. It regressed in b91113d2 during early 'next' development. Fixed in git for 0.99.8. (In reply to comment #34) > (In reply to comment #21) > > TpTestsContactsConnection: don't leak list_manager > > This was in fact incorrect: in 0.22, create_channel_managers() returns the > ref, leaving list_manager as a borrowed ref to something tracked by the base > class. That seems poor style, but is valid. > > It's possible that this changed between 0.22 and next. Yes it did, when the base contact list ceased to be a channel manager, turning this weird memory management into a leak. Fixed in git for 0.99.8. |
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.