Merge lp:~unity-team/unity/unity-sru-fix-mem-leaks into lp:unity/3.0

Proposed by Jay Taoko
Status: Merged
Approved by: Neil J. Patel
Approved revision: no longer in the source branch.
Merged at revision: 1198
Proposed branch: lp:~unity-team/unity/unity-sru-fix-mem-leaks
Merge into: lp:unity/3.0
Diff against target: 51 lines (+11/-2)
2 files modified
src/PanelMenuView.cpp (+5/-1)
src/unityshell.cpp (+6/-1)
To merge this branch: bzr merge lp:~unity-team/unity/unity-sru-fix-mem-leaks
Reviewer Review Type Date Requested Status
Neil J. Patel (community) Approve
Daniel van Vugt Needs Fixing
Review via email: mp+62927@code.launchpad.net

Description of the change

* _window_buttons was not properly destroyed in PanelMenuView::~PanelMenuView, resulting in a memory leak.
* In UnityScreen::initLauncher, self->launcherWindow and self->launcher where no properly reference counted.
* In UnityScreen::~UnityScreen, do not call layout->UnReference (). The layout is managed and freed by its parent widget (Unless SinkReference was called on the layout before it was added in the widget tree).

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Those changes to unityshell.cpp are similar to what I had implemented and was going to move into a new bug at some point.

Two concerns though:

1. layout should now be a local variable, because it is effectively lost to the UnityScreen class once it is assigned to the launcherWindow. The layout member should then be removed completely. Alternatively, if you keep layout as a member then a more clear and elegant solution is to reintroduce its UnReference and just give it a SinkReference too.

2. My fixes to unityshell.cpp also have this leak-fix you seem to be missing:

   LOGGER_START_PROCESS ("initLauncher-Panel");
   self->panelController = new PanelController ();
+ self->panelController->SinkReference();
   self->AddChild (self->panelController);
   LOGGER_END_PROCESS ("initLauncher-Panel");

Maybe I was mistaken?...

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

See also similar fixes are required for PanelController:
http://bazaar.launchpad.net/~vanvugt/+junk/unity/revision/87#src/PanelController.cpp

I know there are more such leaks to fix but never bothered committing the fixes. It really should be an exercise it ticking off a list of valgrind results in a new bug later on.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Needs fixing as described above.

review: Needs Fixing
Revision history for this message
Jay Taoko (jaytaoko) wrote :

1. We could remove the layout member variable if it is not a problem for the SRU (change in the .h file)
2. You don't need to call SinkReference on the panelController because it does not have a floating reference. Only objects that inherit from Nux::InitiallyUnownedObject have a floating reference. In Nux, widgets (see Nux::Area) have a floating reference. Calling SinkReference on panelController will do nothing.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I believe it is safe to change unityshell.h because it is not exported in an SDK or seen by any other packages.

Revision history for this message
Neil J. Patel (njpatel) wrote :

Approved.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/PanelMenuView.cpp'
--- src/PanelMenuView.cpp 2011-04-14 13:19:11 +0000
+++ src/PanelMenuView.cpp 2011-05-30 22:25:57 +0000
@@ -168,7 +168,11 @@
168 _title_tex->UnReference ();168 _title_tex->UnReference ();
169169
170 _menu_layout->UnReference ();170 _menu_layout->UnReference ();
171 _window_buttons->UnReference ();171 if (_window_buttons->OwnsTheReference ())
172 _window_buttons->UnReference ();
173 else
174 _window_buttons->Dispose ();
175
172 _panel_titlebar_grab_area->UnReference ();176 _panel_titlebar_grab_area->UnReference ();
173177
174 UBusServer* ubus = ubus_server_get_default ();178 UBusServer* ubus = ubus_server_get_default ();
175179
=== modified file 'src/unityshell.cpp'
--- src/unityshell.cpp 2011-05-26 15:28:38 +0000
+++ src/unityshell.cpp 2011-05-30 22:25:57 +0000
@@ -988,7 +988,6 @@
988 delete placesController;988 delete placesController;
989 panelController->UnReference ();989 panelController->UnReference ();
990 delete controller;990 delete controller;
991 layout->UnReference ();
992 launcher->UnReference ();991 launcher->UnReference ();
993 launcherWindow->UnReference ();992 launcherWindow->UnReference ();
994993
@@ -1016,9 +1015,14 @@
10161015
1017 LOGGER_START_PROCESS ("initLauncher-Launcher");1016 LOGGER_START_PROCESS ("initLauncher-Launcher");
1018 self->launcherWindow = new nux::BaseWindow(TEXT("LauncherWindow"));1017 self->launcherWindow = new nux::BaseWindow(TEXT("LauncherWindow"));
1018 self->launcherWindow->SinkReference ();
1019
1019 self->launcher = new Launcher(self->launcherWindow, self->screen);1020 self->launcher = new Launcher(self->launcherWindow, self->screen);
1021 self->launcher->SinkReference ();
1022
1020 self->launcher->hidden_changed.connect (sigc::mem_fun (self, &UnityScreen::OnLauncherHiddenChanged));1023 self->launcher->hidden_changed.connect (sigc::mem_fun (self, &UnityScreen::OnLauncherHiddenChanged));
1021 1024
1025
1022 self->AddChild (self->launcher);1026 self->AddChild (self->launcher);
10231027
1024 self->layout = new nux::HLayout();1028 self->layout = new nux::HLayout();
@@ -1035,6 +1039,7 @@
1035 self->launcherWindow->ShowWindow(true);1039 self->launcherWindow->ShowWindow(true);
1036 self->launcherWindow->EnableInputWindow(true, "launcher", false, false);1040 self->launcherWindow->EnableInputWindow(true, "launcher", false, false);
1037 self->launcherWindow->InputWindowEnableStruts(true);1041 self->launcherWindow->InputWindowEnableStruts(true);
1042
1038 self->launcherWindow->SetEnterFocusInputArea (self->launcher);1043 self->launcherWindow->SetEnterFocusInputArea (self->launcher);
10391044
1040 /* FIXME: this should not be manual, should be managed with a1045 /* FIXME: this should not be manual, should be managed with a

Subscribers

People subscribed via source and target branches

to all changes: