DynamicNotebook tabs are never truly destroyed

[Cody Garver]

Revision 6210 caused a regression where granite tabs are not unloaded when closed. To reproduce, play a youtube (html5) video and close the tab to hear it still playing in the background. This does not happen with revision 6209.

ProblemType: Bug
DistroRelease: elementary OS 0.2
Package: midori-granite 0.5.2+r6215-0+pkg13~precise1 [origin: LP-PPA-elementary-os-daily]
ProcVersionSignature: Ubuntu 3.8.0-25.37~precise1-generic 3.8.13
Uname: Linux 3.8.0-25-generic x86_64
ApportVersion: 2.0.1-0ubuntu17.2+elementary3~precise1
Architecture: amd64
CrashDB: midori_granite
Date: Wed Jun 19 02:54:21 2013
ExecutablePath: /usr/bin/midori
InstallationMedia: elementary OS 0.2 "Luna" - Beta 2 amd64 (20130506)
MarkForUpload: True
ProcEnviron:
 PATH=(custom, no user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
SourcePackage: midori-granite
UpgradeStatus: No upgrade log present (probably fresh install)

[Cris Dywan (kalikiana)]

Julián, could you please have a look at this? Since you looked at that code before. I'm not sure if the fix was missing something or if it actually uncovered another problem.

[Julián Unrrein (junrrein)]

I will take a look at this.

[Julián Unrrein (junrrein)]

Feel free to revert rev6210. I don't know when I will have time to continue looking into this. Sorry for the inconvenience.

[Julián Unrrein (junrrein)]

I can reproduce this problem in revision 6209, so this regression isn't new.

[Julián Unrrein (junrrein)]

I can replicate this problem in Pantheon-Files.
Tabs aren't destroyed when closing them. As the content widget is being referenced by the tab, the widget doesn't get destroyed either.

[Julián Unrrein (junrrein)]

In Pantheon-Files this is evident because the default console output when destroying the tab content is (when the closed content was an icon view):

fm_abstract_icon_view_destroy
fm_abstract_icon_view_destroy
fm_abstract_icon_view_finalize

Using a revision before Dynamic Notebook got implemented in Files (rev1214 was the last one) shows that the content widgets are destroyed correctly. Under current trunk, however, the widgets aren't destroyed when closing a tab.

[Julián Unrrein (junrrein)]

I can also confirm this by connecting the following when creating a tab:

new_tab.destroy.connect (() => {
                message ("A tab was destroyed");
});

content.destroy.connect (() => {
                message ("A ViewContainer was destroyed");
});

This messages are never shown when closing a tab. They are shown only when closing Files.

[Julián Unrrein (junrrein)]

I can also replicate this in granite-demo. I modified a little piece of code:

        dynamic_notebook.tab_removed.connect ((t) => {
            print ("Going to remove %s\n", t.label);
            t.destroy.connect (() => {
                print ("A tab was destroyed\n");
            });
            return true;

And it works just as in pantheon-files. It never notifies me of the destruction of a tab when it's closed. It only notifies me if the window is closed with the tab opened.

With this in mind, I suspect this problem may lie in Granite after all.

[Julián Unrrein (junrrein)]

I forgot to add, that I added similar testing code in granite-demo connected to the tab_added signal. That's why I'm getting notified of the destruction of tabs when the window is closed while they are still there.

[Julián Unrrein (junrrein)]

I found one of the culprits here (http://bazaar.launchpad.net/~elementary-pantheon/granite/granite/view/head:/lib/Widgets/DynamicNotebook.vala#L698):

if (tab.label != "" && tab.restore_data != "") {
                closed_tabs.push (tab);
                restore_button.sensitive = !closed_tabs.empty;
                restore_tab_m.sensitive = !closed_tabs.empty;
}

"tab.restore_data" isn't initialized when creating a tab. So tabs always got pushed to the "closed_tabs" stack.

While initialiazing "tab.restore_data" prevents tabs from being pushed, it still isn't enough to solve this bug. Tabs still don't get destroyed after being removed.

[Julián Unrrein (junrrein)]

Sorry about the last comment.
While it's technically a bug, it doesn't have any relationship with tabs not being destroyed. That piece of code doesn't push the whole tab, just the relevant info necessary to restore it.

[Julián Unrrein (junrrein)]

For the record, this is the code I used to debug the issue in granite-demo.

I added this at the end of DynamicNotebook.remove_tab (http://bazaar.launchpad.net/~elementary-pantheon/granite/granite/view/head:/lib/Widgets/DynamicNotebook.vala#L686):

            warning ("Going to remove a tab");

            tab.destroy.connect (() => {
                warning ("The tab got destroyed");
            });

            if (old_tab == tab) {
                warning ("This tab is the same as the old one");
                old_tab = null;
            }

            if (_tabs.index (tab) != -1) {
                warning ("The tab is here: %d", _tabs.index (tab));
                _tabs.remove (tab);
            }

            tab.unref ();

A tab seems to be referenced from a lot of places. If we just switched from that tab before removing it, then "old_tab" is referencing it. A lot of times, a tab is also a part of the "_tabs" list. And just to see what happens, I added an "unref". And the thing still doesn't get destroyed.
Adding another "unref" gets the tab destroyed, and tends to crash everything along the way.

[Cassidy James Blaede (cassidyjames)]

Just a note: this is blocking Luna. No pressure. ;)

[Corentin Noël (tintou)]

After looking at the GtkNotebook sources, it seems that they unparent the tab's child.
I tried to add this at the end of the remove_tab function :
tab.page.unparent ();
And the music in midori disappears at least (and memory get freed).
But then when dragging a tab in order to create a new window in midori, it crashes…

[Cody Garver (codygarver)]

It should also be noted that closing a tab while it's still loading crashes Midori.

[Jeremy Wootten (jeremywootten)]

Need to check whether there is still a problem with FIles tabs not being destroyed.

[Jeremy Wootten (jeremywootten)]

The "fix-shift-delete" branch fixes this in passing - the ViewContainers are now destroyed when a tab is closed. It was necessary to disconnect some signals before closing the tab.