[geary: 3/9] Fix some serious run-time memory leaks.



commit 635d3a947a7d6540d4e5378a92184756158cd203
Author: Michael James Gratton <mike vee net>
Date:   Wed Feb 21 12:36:59 2018 +1100

    Fix some serious run-time memory leaks.
    
    * src/client/components/client-web-view.vala (ClientWebView): Fix
      instances being leaked due to circular ref when registering JS message
      handlers.
    
    * src/engine/util/util-idle-manager.vala,
      src/engine/util/util-timeout-manager.vala: Fix instances and classes
      using these being leaked due to reference loop when used with closures
      as callbacks.

 src/client/components/client-web-view.vala |   22 +++++++++++++++++++---
 src/engine/util/util-idle-manager.vala     |    8 +++++---
 src/engine/util/util-timeout-manager.vala  |   12 +++++++-----
 3 files changed, 31 insertions(+), 11 deletions(-)
---
diff --git a/src/client/components/client-web-view.vala b/src/client/components/client-web-view.vala
index c891bd7..5d38d4d 100644
--- a/src/client/components/client-web-view.vala
+++ b/src/client/components/client-web-view.vala
@@ -238,6 +238,9 @@ public class ClientWebView : WebKit.WebView, Geary.BaseInterface {
     private Gee.Map<string,Geary.Memory.Buffer> internal_resources =
         new Gee.HashMap<string,Geary.Memory.Buffer>();
 
+    private Gee.List<ulong> registered_message_handlers =
+        new Gee.LinkedList<ulong>();
+
 
     /**
      * Emitted when the view's content has finished loaded.
@@ -325,6 +328,14 @@ public class ClientWebView : WebKit.WebView, Geary.BaseInterface {
         base_unref();
     }
 
+    public override void destroy() {
+        foreach (ulong id in this.registered_message_handlers) {
+            this.user_content_manager.disconnect(id);
+        }
+        this.registered_message_handlers.clear();
+        base.destroy();
+    }
+
     /**
      * Loads a message HTML body into the view.
      */
@@ -417,11 +428,16 @@ public class ClientWebView : WebKit.WebView, Geary.BaseInterface {
      */
     protected inline void register_message_handler(string name,
                                                    JavaScriptMessageHandler handler) {
-        // XXX cant use the delegate directly: b.g.o Bug 604781
-        this.user_content_manager.script_message_received[name].connect(
+        // XXX cant use the delegate directly, see b.g.o Bug
+        // 604781. However the workaround below creates a circular
+        // reference, causing ClientWebView instances to leak. So to
+        // work around that we need to record handler ids and
+        // disconnect them when being destroyed.
+        ulong id = this.user_content_manager.script_message_received[name].connect(
             (result) => { handler(result); }
         );
-        if (!get_user_content_manager().register_script_message_handler(name)) {
+        this.registered_message_handlers.add(id);
+        if (!this.user_content_manager.register_script_message_handler(name)) {
             debug("Failed to register script message handler: %s", name);
         }
     }
diff --git a/src/engine/util/util-idle-manager.vala b/src/engine/util/util-idle-manager.vala
index f6bf90e..d99431d 100644
--- a/src/engine/util/util-idle-manager.vala
+++ b/src/engine/util/util-idle-manager.vala
@@ -43,7 +43,9 @@ public class Geary.IdleManager : BaseObject {
         get { return this.source_id >= 0; }
     }
 
-    private IdleFunc callback;
+    // Callback must be unowned to avoid reference loop with owner's
+    // class when a closure is used as the callback.
+    private unowned IdleFunc callback;
     private int source_id = -1;
 
 
@@ -53,8 +55,8 @@ public class Geary.IdleManager : BaseObject {
      * The idle function will be by default not running, and hence
      * needs to be started by a call to {@link schedule}.
      */
-    public IdleManager(owned IdleFunc callback) {
-        this.callback = (owned) callback;
+    public IdleManager(IdleFunc callback) {
+        this.callback = callback;
     }
 
     ~IdleManager() {
diff --git a/src/engine/util/util-timeout-manager.vala b/src/engine/util/util-timeout-manager.vala
index b060097..fef1617 100644
--- a/src/engine/util/util-timeout-manager.vala
+++ b/src/engine/util/util-timeout-manager.vala
@@ -50,7 +50,9 @@ public class Geary.TimeoutManager : BaseObject {
         get { return this.source_id >= 0; }
     }
 
-    private TimeoutFunc callback;
+    // Callback must be unowned to avoid reference loop with owner's
+    // class when a closure is used as the callback.
+    private unowned TimeoutFunc callback;
     private int source_id = -1;
 
 
@@ -60,10 +62,10 @@ public class Geary.TimeoutManager : BaseObject {
      * The timeout will be by default not running, and hence needs to be
      * started by a call to {@link start}.
      */
-    public TimeoutManager.seconds(uint interval, owned TimeoutFunc callback) {
+    public TimeoutManager.seconds(uint interval, TimeoutFunc callback) {
         this.use_seconds = true;
         this.interval = interval;
-        this.callback = (owned) callback;
+        this.callback = callback;
     }
 
     /**
@@ -72,10 +74,10 @@ public class Geary.TimeoutManager : BaseObject {
      * The timeout will be by default not running, and hence needs to be
      * started by a call to {@link start}.
      */
-    public TimeoutManager.milliseconds(uint interval, owned TimeoutFunc callback) {
+    public TimeoutManager.milliseconds(uint interval, TimeoutFunc callback) {
         this.use_seconds = false;
         this.interval = interval;
-        this.callback = (owned) callback;
+        this.callback = callback;
     }
 
     ~TimeoutManager() {


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]