Merge lp:~gue5t/midori/tabby-leaks into lp:midori

[gue5t gue5t]

Fix leaks of two references to the MidoriApp in Tabby

Tabby seems to assume it will never be deactivated, which is an inherently leaky philosophy. This nulls out at least the two fields that hold references to the MidoriApp when Tabby is (theoretically) deactivated.

[André Stösel (ivaldi)]

Well, this will most likely break things if some event (which is handled by tabby) is triggered after deactivated is called. Yes, you are right, tabby assumes it is never deactivated, but if we change it, we should do it the right way.

review: Needs Fixing

[gue5t gue5t (gue5t)]

Tabby won't be deactivated in normal use since it's essentially part of the core, but only when the browser is shut down. When Tabby *is* deactivated, it should not hold references to the MidoriApp.

[André Stösel (ivaldi)]

It doesn't matter - if we do it, we should do it all right!
(There could be some race conditions which triggers some (future) event after the deactivate call and midori could segfault,... you never know ;)

[gue5t gue5t (gue5t)]

I guess it's fair enough that we should make sure all signals are disconnected first. I'll update the branch when I get a chance.

[lp:~gue5t/midori/tabby-leaks]

6675. By gue5t <email address hidden> on 2014-04-24

Document vfuncs in tabby and disconnect some signals on deactivation

[Cris Dywan (kalikiana)]

60 - public virtual void close () {
61 + public void detach () {

Is this intended?

[lp:~gue5t/midori/tabby-leaks]

6676. By gue5t <email address hidden> on 2014-04-25

Factor detach back into close to avoid extra public API change and keep code simple

[André Stösel (ivaldi)]

looks good (thx for the comments ;)

review: Approve

=== modified file 'extensions/tabby.vala'
--- extensions/tabby.vala	2014-03-26 00:00:38 +0000
+++ extensions/tabby.vala	2014-04-25 19:50:26 +0000
@@ -16,7 +16,7 @@
 
     /* function called from Manager object */
     public interface IStorage : GLib.Object {
-        public abstract Katze.Array get_sessions ();
+        public abstract Katze.Array get_saved_sessions ();
         public abstract Base.Session get_new_session ();
         public abstract void restore_last_sessions ();
         public abstract void import_session (Katze.Array tabs);
@@ -24,10 +24,15 @@
 
     public interface ISession : GLib.Object {
         public abstract Katze.Array get_tabs ();
+        /* Add one tab to the database */
         public abstract void add_item (Katze.Item item);
+        /* Attach to a browser */
         public abstract void attach (Midori.Browser browser);
+        /* Attach to a browser and populate it with tabs from the database */
         public abstract void restore (Midori.Browser browser);
+        /* Remove all tabs from the database */
         public abstract void remove ();
+        /* Run when a browser is closed */
         public abstract void close ();
     }
 
@@ -43,7 +48,7 @@
         public abstract class Storage : GLib.Object, IStorage {
             public Midori.App app { get; construct; }
 
-            public abstract Katze.Array get_sessions ();
+            public abstract Katze.Array get_saved_sessions ();
             public abstract Base.Session get_new_session ();
 
             public void start_new_session () {
@@ -52,7 +57,7 @@
             }
 
             public void restore_last_sessions () {
-                Katze.Array sessions = this.get_sessions ();
+                Katze.Array sessions = this.get_saved_sessions ();
                 this.init_sessions (sessions);
             }
 
@@ -93,7 +98,7 @@
         public abstract class Session : GLib.Object, ISession {
             protected GLib.SList<double?> tab_sorting;
 
-            public Midori.Browser browser { get; protected set; }
+            public Midori.Browser? browser { get; protected set; default = null; }
             public SessionState state { get; protected set; default = SessionState.CLOSED; }
 
             public abstract void add_item (Katze.Item item);
@@ -215,12 +220,20 @@
             }
 
             public virtual void close () {
-                this.browser.add_tab.disconnect (this.tab_added);
-                this.browser.add_tab.disconnect (this.helper_data_changed);
-                this.browser.remove_tab.disconnect (this.tab_removed);
-                this.browser.switch_tab.disconnect (this.tab_switched);
-                this.browser.delete_event.disconnect (this.delete_event);
-                this.browser.notebook.page_reordered.disconnect (this.tab_reordered);
+                if (this.state == SessionState.CLOSED) {
+                    assert (this.browser == null);
+                } else {
+                    this.state = SessionState.CLOSED;
+
+                    this.browser.add_tab.disconnect (this.tab_added);
+                    this.browser.add_tab.disconnect (this.helper_data_changed);
+                    this.browser.remove_tab.disconnect (this.tab_removed);
+                    this.browser.switch_tab.disconnect (this.tab_switched);
+                    this.browser.delete_event.disconnect (this.delete_event);
+                    this.browser.notebook.page_reordered.disconnect (this.tab_reordered);
+
+                    this.browser = null;
+                }
             }
 
 #if HAVE_GTK3
@@ -229,7 +242,7 @@
             protected bool delete_event (Gtk.Widget widget, Gdk.Event event) {
 #endif
 
-                this.close();
+                this.close ();
                 return false;
 
             }
@@ -481,13 +494,15 @@
                 }
             }
 
-            public override void close() {
+            public override void close () {
+                /* base.close may unset this.browser, so hold onto it */
+                Midori.Browser? my_browser = this.browser;
                 base.close ();
 
                 bool should_break = true;
-                if (!this.browser.destroy_with_parent) {
+                if (my_browser != null && !my_browser.destroy_with_parent) {
                     foreach (Midori.Browser browser in APP.get_browsers ()) {
-                        if (browser != this.browser && !browser.destroy_with_parent) {
+                        if (browser != my_browser && !browser.destroy_with_parent) {
                             should_break = false;
                             break;
                         }
@@ -595,7 +610,7 @@
         private class Storage : Base.Storage {
             private Midori.Database database;
 
-            public override Katze.Array get_sessions () {
+            public override Katze.Array get_saved_sessions () {
                 Katze.Array sessions = new Katze.Array (typeof (Session));
 
                 string sqlcmd = """
@@ -717,7 +732,7 @@
         }
 
         private void browser_added (Midori.Browser browser) {
-            Base.Session session = browser.get_data<Base.Session> ("tabby-session");
+            Base.Session? session = browser.get_data<Base.Session> ("tabby-session");
             if (session == null) {
                 session = this.storage.get_new_session () as Base.Session;
                 browser.set_data<Base.Session> ("tabby-session", session);
@@ -726,7 +741,7 @@
         }
 
         private void browser_removed (Midori.Browser browser) {
-            Base.Session session = browser.get_data<Base.Session> ("tabby-session");
+            Base.Session? session = browser.get_data<Base.Session> ("tabby-session");
             if (session == null) {
                 GLib.warning ("missing session");
             } else {
@@ -764,6 +779,17 @@
             GLib.Idle.add (this.load_session);
         }
 
+        private void deactivated () {
+            /* set_open_uris will disconnect itself if called,
+               but it may have been called before we are deactivated */
+            APP.add_browser.disconnect (this.set_open_uris);
+            APP.add_browser.disconnect (this.browser_added);
+            APP.remove_browser.disconnect (this.browser_removed);
+            APP = null;
+
+            this.storage = null;
+        }
+
         internal Manager () {
             GLib.Object (name: _("Tabby"),
                          description: _("Tab and session management."),
@@ -771,6 +797,7 @@
                          authors: "André Stösel <[e-mail]>");
 
             this.activate.connect (this.activated);
+            this.deactivate.connect (this.deactivated);
         }
     }
 }