After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730389 - crash in sync_tab_label
crash in sync_tab_label
Status: RESOLVED FIXED
Product: gnome-terminal
Classification: Core
Component: general
3.12.x
Other Linux
: Normal critical
: ---
Assigned To: GNOME Terminal Maintainers
GNOME Terminal Maintainers
: 729273 730845 730935 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-05-19 16:47 UTC by Christian Persch
Modified: 2014-06-03 17:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix (693 bytes, patch)
2014-05-27 21:00 UTC, Egmont Koblinger
rejected Details | Review
Fix v2 (1.53 KB, patch)
2014-05-29 14:35 UTC, Egmont Koblinger
reviewed Details | Review
Fix v3 (1.62 KB, patch)
2014-05-30 13:09 UTC, Egmont Koblinger
committed Details | Review

Description Christian Persch 2014-05-19 16:47:09 UTC
From https://retrace.fedoraproject.org/faf/problems/1568584/ :

1 	g_type_check_instance_is_a 	/usr/lib64/libgobject-2.0.so.0 	/gtype.c 	3969
2 	gtk_widget_get_parent 	/usr/lib64/libgtk-3.so.0 	/usr/src/debug/gtk+-3.13.1/gtk/gtkwidget.c 	9026
3 	sync_tab_label 	/usr/libexec/gnome-terminal-server 	/terminal-tab-label.c 	76

No idea how that could happen, the code looks safe...
Comment 1 Debarshi Ray 2014-05-19 17:03:24 UTC
There are two ways to trigger this.

One, you can drag the tab outside the window and leave it. It will crash
gnome-terminal-server immediately.

Or, you can detach the tab and leave it inside the window. Then click another
tab in the same window.

It works fine on 3.10.x. I suspect it is broken on 3.12.x because I think the
person who showed this to me runs 3.12.x.
Comment 2 Debarshi Ray 2014-05-19 17:03:40 UTC
*** Bug 729273 has been marked as a duplicate of this bug. ***
Comment 3 Debarshi Ray 2014-05-19 17:04:33 UTC
I tried bisecting this, but somewhere along the line it stopped crashing predictably. :/
Comment 4 Egmont Koblinger 2014-05-27 18:53:07 UTC
Are there any tricks to reproduce this? I'm dragging tabs crazily, but no crash.
Comment 5 Debarshi Ray 2014-05-27 18:57:01 UTC
*** Bug 730845 has been marked as a duplicate of this bug. ***
Comment 6 Egmont Koblinger 2014-05-27 19:24:20 UTC
I can now reproduce a crash, but it requires that the tab's title changes while it's being dragged.  Debarshi: Any chance your tab title changes too?

Prior to the crash, once when dragging such a title gnome-terminal-server starts printing these four lines in an endless loop:

(gnome-terminal-server:31058): Gtk-CRITICAL **: gtk_widget_get_parent: assertion 'GTK_IS_WIDGET (widget)' failed

(gnome-terminal-server:31058): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'GtkLabel'

(gnome-terminal-server:31058): Gtk-CRITICAL **: gtk_label_set_text: assertion 'GTK_IS_LABEL (label)' failed

(gnome-terminal-server:31058): Gtk-CRITICAL **: gtk_widget_set_tooltip_text: assertion 'GTK_IS_WIDGET (widget)' failed
Comment 7 Egmont Koblinger 2014-05-27 21:00:40 UTC
Created attachment 277345 [details] [review]
Fix

I think this patch fixes the problem - Debarshi, could you please test?

It's reproducible even if the title changes after a drag-n-drop.

When the title changes, terminal_screen_window_title_changed() emits a "title" signal. terminal_tab_label_constructor() starts listening for this signal to call sync_tab_label() which will take care of the actual job.

When a tab is dragged around, the old TerminalTabLabel is dropped and a new one is created. However, the finalize method doesn't stop listening to the signal.

So we end up with many listeners on the signal, one is the current tab label, others are ceased tab labels; resulting in multiple calls to sync_tab_label() on a title change, most of them with incorrect parameters.
Comment 8 Christian Persch 2014-05-28 06:58:12 UTC
Comment on attachment 277345 [details] [review]
Fix

master and 3-12.
Comment 9 Egmont Koblinger 2014-05-28 10:20:59 UTC
Comment on attachment 277345 [details] [review]
Fix

[master 6c1ece7] [gnome-3-12 e2824de]
Comment 10 Egmont Koblinger 2014-05-28 10:22:42 UTC
Debarshi, I'm closing it for now, but I'd be still be happy to hear if the fix works for you too :)
Comment 11 Debarshi Ray 2014-05-28 10:52:37 UTC
(In reply to comment #10)
> Debarshi, I'm closing it for now, but I'd be still be happy to hear if the fix
> works for you too :)

Yes, it appears to fix the crash. Thanks for the patch, Egmont.
Comment 12 Egmont Koblinger 2014-05-28 11:03:45 UTC
You're welcome :)

It has just occurred to me that vte.sh sets the shell prompt to update the window title.  Even if it stays the same string, technically it's updated each time the prompt is presented.

I myself have removed this, that's why I had a harder time reproducing the bug.
Comment 13 Christian Persch 2014-05-28 11:16:46 UTC
We could add a g_strcmp0 between new and old in vte_sequence_handler_set_current_{file,directory}_uri() if you think that's necessary.
Comment 14 Egmont Koblinger 2014-05-28 11:21:59 UTC
It's not strictly speaking necessary, but it would be indeed nice not to emit the title-changed (or similar) signal if it didn't actually change.  Meh.
Comment 15 Christian Persch 2014-05-29 11:51:19 UTC
Reverted the patch; it causes a segfault every time a tab is closed by the close button.
Comment 16 Egmont Koblinger 2014-05-29 11:55:38 UTC
*** Bug 730935 has been marked as a duplicate of this bug. ***
Comment 17 Christian Persch 2014-05-29 12:21:12 UTC
There are some changes in gtknotebook tab-label behaviour, e.g. commit 325cf071d1b6de55eac2a97d8f38558efda17807 and a follow-up. It's quite likely these broke our expectations (ie the *same* tab label is re-inserted into the new window's notebook, not a new one!).
Comment 18 Egmont Koblinger 2014-05-29 14:35:47 UTC
Created attachment 277460 [details] [review]
Fix v2

Conclusion from IRC discussion:

When closing with the X button, the event is holding a reference to the button. This results in screen getting finalized first, followed by the tab label (instead of the other order which happens otherwise and on which my patch relied).

The solution is to hold a ref from the tab label to the screen, and disconnect the signal handler in dispose rather than finalize.

Pleaes test and review, thanks!
Comment 19 Egmont Koblinger 2014-05-29 14:42:56 UTC
Use cases to test:

- Drag-n-dropping tabs, dropping them in the same window or to a new window; to the terminal area, to the tab bar, to the desktop etc.

- Closing tabs by various means, including the X button, with or without an application running inside (which gives a confirmation dialog)

- Closing windows by various means, including the window manager's X button

- All these combined with tabs that frequently change their title, e.g. run:
while sleep 0.1; do echo -ne "\e]0;$(date +%S)\a"; done

Not a single warning should be printed by gnome-terminal-server.
Comment 20 Christian Persch 2014-05-30 11:37:42 UTC
Review of attachment 277460 [details] [review]:

With that fixed, ok to commit to master and 3-12.

::: src/terminal-tab-label.c
@@ +184,2 @@
   g_assert (priv->screen != NULL);
+  g_object_ref (priv->screen);

That's not the right place to add the ref. Instead simply exchange g_value_get_object() for g_value_dup_object() in terminal_tab_label_set_property.
Comment 21 Egmont Koblinger 2014-05-30 13:09:20 UTC
Created attachment 277543 [details] [review]
Fix v3
Comment 22 Egmont Koblinger 2014-05-30 13:16:02 UTC
Comment on attachment 277543 [details] [review]
Fix v3

[master f65261a] [gnome-3-12 f8713e1]
Comment 23 Hin-Tak Leung 2014-06-02 06:00:28 UTC
I am glad it is resolved - this has been bugging me since I upgraded to gnome 3.12 via fedora copr. It made gnome-terminal completely unusable for me, and I have switched to roxterm for important work that I don't want to see a crash (and all terminals disappearing!), and also to permanently run a gdb attached to gnome-terminal-server and about to post the full backtrace with debug.

FWIW, it affected me badly because I like to work with a mixture of terminals and tabs (for organizing different sets of work), like it was possible in gnome 3.10. With the "open terminal/tabs" options gone in 3.12, I found it most convenient to configure it default to tabs then detach, to have a mixture of tabs and terminals back. I think it is a bad choice to take away those options - surely just like with web browsers, people like a number of windows with tabs in each, rather than either all tabs or all single windows?
Comment 24 Hin-Tak Leung 2014-06-02 06:13:41 UTC
For me, the crash happens with *all* the terminals suddenly disappearing, under *normal daily usage*. By normal daily usage, it means a mixtures of terminals and tabs, some of which had been resized, and I use different tabs or terminals for different sets of work while multi-tasking. I have the standard vte.sh which sets the prompt strings and the tab title to current directory; so the crash pretty much happens on random commands being run on any of the terminals/tabs, eventually, after working on the computer for a while, with *all* on-going work in terminals disappearing.
Comment 25 Egmont Koblinger 2014-06-02 10:46:42 UTC
gnome-terminal is a single process taking care of all the windows and tabs. It definitely has many benefits, but one of the serious drawback is that indeed if it crashes, all your terminals are gone.

Please note that you still can open new windows or new tabs via different hotkeys (Ctrl+Alt+N and Ctrl+Alt+T by default), it's only the menu entries that were merged (I'm not sure why).
Comment 26 Hin-Tak Leung 2014-06-03 01:18:46 UTC
roxterm seems to have all the benefit of gnome-terminal without the drawbacks. - other than the icon - the blue "$" sign - is irritating. "terminals" as a class of application should be reliable but uninteresting itself, I think. I can accept irritating icon over all on-going work disappearing.

> Please note that you still can open new windows or new tabs via different
> hotkeys (Ctrl+Alt+N and Ctrl+Alt+T by default), it's only the menu entries that
> were merged (I'm not sure why).

I don't know what school of "usability" that change subscribes to - what you are suggesting is changing from a one-handed operation involving 1 finger and a hand movement, a mouse-click-drag-click, to a 3-finger and potentially two-hand operation. That's *not* progress.
Comment 27 Egmont Koblinger 2014-06-03 02:33:24 UTC
> I can accept irritating icon over all on-going work disappearing.

roxterm also uses a single process by default for all your windows and tabs. Meaning if there's a bug in that one, it will equally crash all your terminals. We've just fixed a crash in gnome-terminal. Needless to say, I have no idea how many similar fatal bugs reside in gnome-terminal or roxterm (let's nope none).

You can ask both gnome-terminal and roxterm to run as separate process for each window. In this case you should not use the terminal's "New Window" menu entry, but start a new one from command line or by similar means instead (with proper command line options). Also, you won't be able to drag-n-drop tabs between windows, which I assume you regularly do, since the currently fixed crash relied on this action.

So, in this regard, I see no difference between g-t and rox. Anyway, you're free to use whichever you prefer.

> I don't know what school of "usability" that change subscribes to

Please raise the issue in a separate enhancement request, it's totally irrelevant to this bug here.
Comment 28 Hin-Tak Leung 2014-06-03 17:07:34 UTC
> Needless to say, I have no idea how many similar fatal bugs reside in gnome-terminal or roxterm (let's nope none).

There is none in roxterm - I was trying all the other "terminal" class applications provided by fedora and picking one which is generally useful and also leaving a permanent gdb session attached to gnome-terminal-server's process to catch it as it crashes. The fact that *all* gnome terminal windows disappear as the terminal server process crashes means you cannot run a gdb session to monitor it.

I managed to catch a gdb backtrace 3 times (and installing *debuginfo* packages between those to get more info) to get line numbers, and was about to file and post the final one before I found this bug report already.

> > I don't know what school of "usability" that change subscribes to

> Please raise the issue in a separate enhancement request, it's totally
irrelevant to this bug here.

It is related - as commented in comment 1 by another person, the problem is easily seen in manipulating detached tabs in multiple windows. That gnome 3.12 *made it difficult* to work in such a manner, is the reason why it haven't been caught *before* release.

And it is not a "enhancement request" - it is a "regression". A old facility - click-drag-click to spawn tab/windows - was removed from 3.10 to 3.12. Now it needs 3 fingers with possibly two hands to do the same operation. (or configure to default to tabs, click-drag-click-click-drag-click to spawn tab then detach, if you really want to spawn a window in one hand, and routinely works with a mixture of tabs and windows).