RebuildData structs are leaked in rebuild() / do_rebuild()

[JKL]

[Impact]
This issue affects most and any users of appgtk-menu, such as nm-applet, especially in environments (such as offices) where the detected wireless networks change a lot, and where roaming can occur frequently. See also: LP: #780602

[Test Case]
Run nm-applet for multiple hours:
 - Observe that the VPN Connections submenu still opens and lists connections (if VPN connections are configured)
 - Observe that the "More networks" submenu for additional detected wireless networks still opens and lists networks.

A common failure case is where such a submenu will open but show an empty list (a piece of menu a few milimeters long).

[Regression Potential]
Minimal, the fixes have been available for a while in the development release (and in other recent releases) with no regressions.
Possible issues could be introducing new memory leaks with the changes however, and although the risk is minimal, this could cause the same failure behavior as listed above under [Test Case].

--

Binary package hint: appmenu-gtk

The rebuild function creates a hash table mapping the toplevel to the g_timeout id responsible for doing the refresh.

The rebuild function allocates a RebuildData object to pass to the do_rebuild idle function. The do_rebuild callback function frees this object when it is called.

However, the do_rebuild function isn't guaranteed to be called. In fact the whole point of the hash table seems to be that the callback can be canceled. When that happens, the RebuildData object isn't freed, causing a leak.

Additionally, instead of storing a proper reference to the toplevel in the RebuldData object, a weak pointer is stored instead. But if the weak pointer gets set to null, the do_rebuld function will lose its only copy of that pointer, so it will have no way of cleaning up the associated entry in the hash table.

[JKL (jkl102001)]

One more thing -- app_menu_bridge_insert does not release the "attach-widget" reference it gets, causing another leak.

No patch. It's a one-liner.

[Ted Gould (ted)]

Thanks for the patch. As you see attached to this bug, I've put it into a Bazaar branch to make it slightly easier to track.

It seems that do_rebuild() needs to get passed the entry now instead of data because it needs to remove the entry from the rebuild_ids hash table not data->widget. Does that seem right to you?

[JKL (jkl102001)]

I'm not sure what you are saying, but looking back at it now, I think the patch I wrote was overcomplicated. The design can be simplified considerably, and in doing so two additional issues can be addressed.

The first issue has to do with scheduling. The existing design cancels the prior scheduled rebuild of the menu, which means that if rebuild ends up getting called at short intervals over a long period of time, the menu will never end up getting rebuilt.

I think it is better to simply check whether a rebuild is already scheduled, and if so, do nothing. That way you ensure that the rebuild doesn't happen too often, but you don't end up postponing it indefinitely in the worst case.

The second issue has to do with the life cycle of the hash table. As written, it is difficult to understand when the hash table is created and destroyed, and why it is safe to destroy it. In particular, we must prove there are no pending timeouts when we destroy the table, because we don't want to free the RebuildData out from under a pending timeout.

I think it is better to make the hash table a private member of the bridge rather than a global variable. That way it is possible to write a very simple argument showing why it is safe to destroy the hash table when the bridge is finalized.

I checked out your branch and wrote a patch on top of it demonstrating what I have in mind.

[Charles Kerr (charlesk)]

JKL's patch is a good improvement, but it's been a few months since it was submitted there are some newer complications in the code, specifically having a weak reference to each widget s.t. we can handle the case of a widget being destroyed while it's still got a rebuild pending.

lp:~charlesk/appmenu-gtk/lp-787736 is a similar patch which handles the weak references.

[Launchpad Janitor (janitor)]

This bug was fixed in the package appmenu-gtk - 12.10.1-0ubuntu1

---------------
appmenu-gtk (12.10.1-0ubuntu1) quantal; urgency=low

  * New upstream release.
    - Ignore menus from gtkmodelmenu in some circumstances.
    - Fix memory leak (LP: #787736)
 -- Ken VanDine <email address hidden> Wed, 19 Sep 2012 15:49:49 -0400

[Alex Chiang (achiang)]

Patch for precise SRU.

[Alex Chiang (achiang)]

Attached is an updated debdiff, since someone else already uploaded a 1.1 to precise-proposed.

[Peter Meiser (meiser79)]

Alex, your patch reverts some parts introduced in 1.1, e.g. blacklisting IBM Notes, maybe check again.

[Colin Watson (cjwatson)]

Hello JKL, or anyone else affected,

Accepted appmenu-gtk into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/appmenu-gtk/0.3.92-0ubuntu1.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Marlon Nelson (marlon-nelson)]

this package does not fix the bug for me

i installed and tested version 0.3.92-0ubuntu1.2

[Mathieu Trudel-Lapierre (cyphermox)]

How did you test it? Is there a way we can counter-check the tests that were done, or verify that the rebuild/do_rebuild cycles are really linked to the provided test case?

[Marlon Nelson (marlon-nelson)]

i test it by connecting to a wifi network and verify that the submenus
operate as expected
then i go off to work
10 hours later, i come home and see if they still work

they don't - i can't see my vpns, or my connection info, and i can't edit
my connections

did the package fix the problem for the developer?

On Mon, Mar 11, 2013 at 6:22 PM, Mathieu Trudel-Lapierre <
<email address hidden>> wrote:

> How did you test it? Is there a way we can counter-check the tests that
> were done, or verify that the rebuild/do_rebuild cycles are really
> linked to the provided test case?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/787736
>
> Title:
> RebuildData structs are leaked in rebuild() / do_rebuild()
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/appmenu-gtk/+bug/787736/+subscriptions
>

--
-eom-

[Martin Pitt (pitti)]

The version of appmenu-gtk in the proposed pocket of Precise that was purported to fix this bug report has been removed because the bugs that were to be fixed by the upload were not verified in a timely (105 days) fashion.