Bug 203069 - (steps) Adding tab (Ctrl+T) in detached (tab) window crashes Konqueror
Summary: (steps) Adding tab (Ctrl+T) in detached (tab) window crashes Konqueror
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Unlisted Binaries Linux
: NOR crash
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords: investigated, reproducible
: 192516 209763 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-08-08 12:32 UTC by Marcel Partap
Modified: 2009-10-16 13:47 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Possible fix (not very elegant) and new unit tests (5.27 KB, patch)
2009-10-08 02:15 UTC, Frank Reininghaus
Details
New patch (6.01 KB, patch)
2009-10-10 11:40 UTC, Frank Reininghaus
Details
Updated patch (also fixes and unit-tests bug 210686) (6.71 KB, patch)
2009-10-15 20:08 UTC, Frank Reininghaus
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel Partap 2009-08-08 12:32:46 UTC
Application that crashed: konqueror
Version of the application: 4.3.62 (KDE 4.3.62 (KDE 4.4 >= 20090728))
KDE Version: 4.3.62 (KDE 4.3.62 (KDE 4.4 >= 20090728))
Qt Version: 4.6.0
Operating System: Linux 2.6.31-rc4-00294-gf5886c7-dirty x86_64
Distribution: "Gentoo Base System release 2.0.1"

What I was doing when the application crashed:
Steps to reproduce: open window, add another tab (so the tab bar becomes visible), detach (any) tab. In the new window, press CTRL+T, voila: bo0m.

 -- Backtrace:
Application: Konqueror (kdeinit4), signal: Segmentation fault
[KCrash Handler]
#4  KonqView::url (this=0x0) at /usr/src/debug/kde-base/konqueror-scm/konqueror/apps/konqueror/src/konqview.cpp:906
#5  0x00007f5e61917721 in KonqMainWindow::slotSubstringcompletion (this=0x23f9f80, text=@0x7fff2ac02650) at /usr/src/debug/kde-base/konqueror-scm/konqueror/apps/konqueror/src/konqmainwindow.cpp:3120
#6  0x00007f5e619390b1 in KonqMainWindow::qt_metacall (this=0x23f9f80, _c=QMetaObject::InvokeMetaMethod, _id=<value optimized out>, _a=0x7fff2ac02400)
    at /usr/src/debug/kde-base/konqueror-scm/konqueror_build/apps/konqueror/src/konqmainwindow.moc:444
#7  0x00007f5e6a65a87c in QMetaObject::activate (sender=0x24085c0, from_signal_index=<value optimized out>, to_signal_index=54, argv=0x7fff2ac02400) at kernel/qobject.cpp:3176
#8  0x00007f5e68b27dd5 in KComboBox::substringCompletion (this=0x7fff2ac020f0, _t1=<value optimized out>) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm_build/kdeui/kcombobox.moc:183
#9  0x00007f5e68b2911c in KComboBox::qt_metacall (this=0x24085c0, _c=QMetaObject::InvokeMetaMethod, _id=<value optimized out>, _a=0x7fff2ac025b0)
    at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm_build/kdeui/kcombobox.moc:108
#10 0x00007f5e68b3bcc5 in KHistoryComboBox::qt_metacall (this=0x7fff2ac020f0, _c=QMetaObject::InvokeMetaMethod, _id=1637354016, _a=0x7fff2ac02400)
    at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm_build/kdeui/khistorycombobox.moc:77
#11 0x00007f5e6190c245 in KonqCombo::qt_metacall (this=0x7fff2ac020f0, _c=QMetaObject::InvokeMetaMethod, _id=1637354016, _a=0x7fff2ac02400)
    at /usr/src/debug/kde-base/konqueror-scm/konqueror_build/apps/konqueror/src/konqcombo.moc:69
#12 0x00007f5e6a65a87c in QMetaObject::activate (sender=0x240bab0, from_signal_index=<value optimized out>, to_signal_index=50, argv=0x7fff2ac02400) at kernel/qobject.cpp:3176
#13 0x00007f5e68b43dd5 in KLineEdit::substringCompletion (this=0x7fff2ac020f0, _t1=<value optimized out>) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm_build/kdeui/klineedit.moc:234
#14 0x00007f5e68b47ca1 in KLineEdit::keyPressEvent (this=0x240bab0, e=0x7fff2ac034a0) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kdeui/widgets/klineedit.cpp:1012
#15 0x00007f5e67a1807d in QWidget::event (this=0x240bab0, event=0x7fff2ac034a0) at kernel/qwidget.cpp:7568
#16 0x00007f5e68b492c5 in KLineEdit::event (this=0x240bab0, ev=0x7fff2ac034a0) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kdeui/widgets/klineedit.cpp:1322
#17 0x00007f5e67d469e9 in QComboBox::keyPressEvent (this=0x24085c0, e=0x7fff2ac034a0) at widgets/qcombobox.cpp:2889
#18 0x00007f5e68b3ba24 in KHistoryComboBox::keyPressEvent (this=0x24085c0, e=0x7fff2ac034a0) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kdeui/widgets/khistorycombobox.cpp:344
#19 0x00007f5e6190c528 in KonqCombo::keyPressEvent (this=0x7fff2ac020f0, e=0x0) at /usr/src/debug/kde-base/konqueror-scm/konqueror/apps/konqueror/src/konqcombo.cpp:428
#20 0x00007f5e67a1807d in QWidget::event (this=0x24085c0, event=0x7fff2ac034a0) at kernel/qwidget.cpp:7568
#21 0x00007f5e679c14ed in QApplicationPrivate::notify_helper (this=0x686b00, receiver=0x24085c0, e=0x7fff2ac034a0) at kernel/qapplication.cpp:4104
#22 0x00007f5e679cce69 in QApplication::notify (this=<value optimized out>, receiver=0x24085c0, e=0x7fff2ac034a0) at kernel/qapplication.cpp:3669
#23 0x00007f5e68a7918b in KApplication::notify (this=0x7fff2ac04810, receiver=0x24085c0, event=0x7fff2ac034a0) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kdeui/kernel/kapplication.cpp:302
#24 0x00007f5e6a645233 in QCoreApplication::notifyInternal (this=0x7fff2ac04810, receiver=0x24085c0, event=0x7fff2ac034a0) at kernel/qcoreapplication.cpp:625
#25 0x00007f5e67a4fc54 in QKeyMapper::sendKeyEvent (keyWidget=0x24085c0, grab=<value optimized out>, type=QEvent::KeyPress, code=84, modifiers={i = 717240768}, text=@0x7fff2ac039b0, 
    autorepeat=false, count=1, nativeScanCode=28, nativeVirtualKey=116, nativeModifiers=20) at kernel/qkeymapper_x11.cpp:1675
#26 0x00007f5e67a51cc0 in QKeyMapperPrivate::translateKeyEvent (this=0x6c6780, keyWidget=0x24085c0, event=0x7fff2ac04270, grab=false) at kernel/qkeymapper_x11.cpp:1645
#27 0x00007f5e67a2e825 in QApplication::x11ProcessEvent (this=0x7fff2ac04810, event=0x7fff2ac04270) at kernel/qapplication_x11.cpp:3533
#28 0x00007f5e67a5350c in x11EventSourceDispatch (s=0x68aa00, callback=0, user_data=0x0) at kernel/qguieventdispatcher_glib.cpp:146
#29 0x00007f5e66448771 in IA__g_main_context_dispatch (context=0x6895f0) at gmain.c:1824
#30 0x00007f5e6644be28 in g_main_context_iterate (context=0x6895f0, block=1, dispatch=1, self=<value optimized out>) at gmain.c:2455
#31 0x00007f5e6644bfec in IA__g_main_context_iteration (context=0x6895f0, may_block=1) at gmain.c:2518
#32 0x00007f5e6a66d12f in QEventDispatcherGlib::processEvents (this=0x614240, flags=<value optimized out>) at kernel/qeventdispatcher_glib.cpp:327
#33 0x00007f5e67a52d6f in QGuiEventDispatcherGlib::processEvents (this=0x7fff2ac020f0, flags=<value optimized out>) at kernel/qguieventdispatcher_glib.cpp:202
#34 0x00007f5e6a643ed2 in QEventLoop::processEvents (this=<value optimized out>, flags={i = 717243744}) at kernel/qeventloop.cpp:149
#35 0x00007f5e6a64406c in QEventLoop::exec (this=0x7fff2ac045a0, flags={i = 717243824}) at kernel/qeventloop.cpp:197
#36 0x00007f5e6a648a96 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:907
#37 0x00007f5e619674e7 in kdemain (argc=<value optimized out>, argv=<value optimized out>) at /usr/src/debug/kde-base/konqueror-scm/konqueror/apps/konqueror/src/konqmain.cpp:257
#38 0x00000000004078ca in launch (argc=2, _name=0x65db58 "/usr/kde/svn/bin/konqueror", args=<value optimized out>, cwd=0x0, envc=0, envs=0x65db84 "", reset_env=false, tty=0x0, avoid_loops=false, 
    startup_id_str=0x65db8c "localhost;1249727108;514811;26828_TIME235929998") at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kinit/kinit.cpp:705
#39 0x000000000040811a in handle_launcher_request (sock=7, who=<value optimized out>) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kinit/kinit.cpp:1197
#40 0x0000000000408639 in handle_requests (waitForPid=0) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kinit/kinit.cpp:1390
#41 0x0000000000409411 in main (argc=2, argv=0x7fff2ac06078, envp=0x7fff2ac06090) at /usr/src/debug/kde-base/kdelibs-scm/kdelibs-scm/kinit/kinit.cpp:1825

This bug may be a duplicate of or related to bug 192516

Reported using DrKonqi
Comment 1 Dario Andres 2009-08-08 16:32:35 UTC
I can reproduce the crash with the provided steps here using:

Qt: 4.5.2 (KDE-Qt git commit f9802f2bbbd23137acb5f80d1f131fa6b1a85752
        Date:   Fri Jun 12 15:06:29 2009 +0200)
KDE: 4.3.62 (KDE 4.3.62 (KDE 4.4 >= 20090728))
kdelibs svn rev. 1006918 / kdebase svn rev. 1006918
on ArchLinux i686 - Kernel 2.6.30.4

Getting the same backtrace.
Thanks
Comment 2 Dario Andres 2009-08-08 16:33:52 UTC
*** Bug 192516 has been marked as a duplicate of this bug. ***
Comment 3 FiNeX 2009-08-16 22:20:14 UTC
Could this be a dup of bug #197836 ?
Comment 4 Dario Andres 2009-08-16 22:25:43 UTC
(In reply to comment #3)
> Could this be a dup of bug #197836 ?

I have checked it and even when the summary is related the backtrace is not so I would choose to keep them separated. Regards
Comment 5 Frank Reininghaus 2009-10-07 20:24:51 UTC
Seems to be a regression caused by my fix for bug 174292 - sorry about that! I promise to have a thorough look at this before KDE 4.3.3.

It does not crash if the tab that is broken off contains split views.
Comment 6 Frank Reininghaus 2009-10-08 02:15:06 UTC
Created attachment 37441 [details]
Possible fix (not very elegant) and new unit tests

Some remarks about my patch:

1. In KonqViewManager::breakOffTab, it seems that KonqViewManager::loadViewProfileFromConfig (which was there before my fix for bug 174292) works well for tabs which are not split, and KonqViewManager::loadRootItem (with the tab container as the parent) works fine for split views. I don't know the internals of KonqViewManager well enough to understand why - maybe there is actually a bug in either of these functions which I haven't seen yet.

2. The patch contains unit tests for the two "break off tab" issues and for bug 160407.

3. I suggest to change the return type of KonqViewManager::breakOffTab and return the new main window. Without this change, unit testing the present bug seems difficult to me.
Comment 7 David Faure 2009-10-08 16:25:14 UTC
The unit test looks good, the API change is certainly fine; but the fix itself looks a bit strange indeed, I think it hides a bug somewhere.
Comment 8 David Faure 2009-10-08 17:11:50 UTC
SVN commit 1032808 by dfaure:

Allow to focus readonly form elements (like other browsers do, partial revert of the commits for 159682),
which makes it possible to copy their text, too (for tomj on irc).
On the other hand, don't allow Ctrl+X and Ctrl+V to work in such widgets.
Will backport for 4.3.3.
BUG: 203069
CCBUG: 159682


 M  +1 -1      html/html_formimpl.cpp  
 M  +18 -12    khtml_ext.cpp  
 M  +2 -2      rendering/render_form.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1032808
Comment 9 David Faure 2009-10-08 17:14:10 UTC
Gahh wrong bug number.
Comment 10 David Faure 2009-10-08 17:16:56 UTC
(for future reference, comment #8 was about bug 190188)
Comment 11 Dario Andres 2009-10-09 03:08:36 UTC
*** Bug 209763 has been marked as a duplicate of this bug. ***
Comment 12 Frank Reininghaus 2009-10-09 23:15:56 UTC
Maybe I got a bit closer to the hidden real bug: If I revert my fix for bug
174292 (except for the change to KonqMainWindow which I believe is correct),
breaking off split tabs leads to a crash because no tab container is created
for the new main window. In KonqViewManager::setLoading(), which is called
indirectly from KonqViewManager::loadItem(), the setLoading() method of the
NULL m_tabContainer is called.

[My fix for that bug worked because it calls
mainWindow->viewManager()->tabContainer() in KonqViewManager::breakOffTab(),
which creates the tab container.]

The reason why it works if the tab is not split is that the "View" branch of
KonqViewManager::loadItem() contains the code (added in r673169)

if ( parent == m_pMainWindow )
    parent = tabContainer();

which creates the tab container. The "Container" branch contains no such code.
I tried to add these two lines also in that branch, but then two problems
remain:

1. Because of the fix for bug 203166 it still crashes because that makes
loading one container with two views via loadViewProfileFromConfig()
impossible.

2. This change would breaks loading the file management profile because that
contains a container with tabs as a child -> everything gets messed up because
the tab container is then both the parent and the child of the
KonqFrameContainer.

So far, I couldn't find a good way to initialise the tab container without
breaking other things :-( The alternative approach would be to not revert my
fix for bug 174292 (such that the tab container gets created properly) and try
to find out why replacing loadViewProfileFromConfig() by loadRootItem() broke
breaking off tabs without split views...
Comment 13 Frank Reininghaus 2009-10-10 00:29:57 UTC
The cause of the crash in this bug report seems to be that the "active child" of the tab container of the new main window is 0 after the tab is broken off. Before my fix for bug 174292, this was not the case because KonqViewManager::loadViewProfileFromGroup() calls setActivePart(), which indirectly sets the active child via KonqViewManager::slotActivePartChanged().

If the tab is split, this problem does not occur: In the "Container" branch of KonqViewManager::loadItem(), the setActiveChild() method of the container is called, which also sets the active child of its parent, i.e., the tab container, in the line

m_pParentContainer->setActiveChild( this ).

Therefore, a possibility to fix this would be to make sure that the only child of the tab container is also its active child :-)
Comment 14 Frank Reininghaus 2009-10-10 11:40:02 UTC
Created attachment 37484 [details]
New patch

This patch makes sure that the tab container of the new window in KonqViewManager::breakOffTab() has an active child. There are also other ways (like calling setActivePart(), maybe that's better?).

I've also made two small changes to the unit tests:

1. Verify that the tab container has an active child, such that the test fails earlier.

2. Delete the new main windows.
Comment 15 Frank Reininghaus 2009-10-15 20:08:32 UTC
Created attachment 37600 [details]
Updated patch (also fixes and unit-tests bug 210686)
Comment 16 David Faure 2009-10-16 02:14:02 UTC
Frank: excellent analysis, thanks for that.

Now that I understand the problem ;) I wonder if simply doing
mainWindow->viewManager()->loadViewProfileFromConfig( config, QString(), currentProfile() );
in all cases would solve it? I.e. not creating the tabContainer until needed, like we do in all other cases (new window etc.). But I guess I need to re-read the file a bit more to check if we always set an active child when we create the tabContainer everywhere.
Comment 17 Frank Reininghaus 2009-10-16 10:44:56 UTC
(In reply to comment #16)
> I wonder if simply doing
> mainWindow->viewManager()->loadViewProfileFromConfig( config, QString(),
> currentProfile() ); in all cases would solve it?

No, it would not - it was like that before I tried to fix bug 174292. In that case, no tab container is created for split views (see comment 12).
Comment 18 David Faure 2009-10-16 11:06:00 UTC
Ah I see. The tab container is not really created on demand, but created already when the first view is created (in the normal case; cf createFirstView).

It seems to me that the two openSavedWindow methods have that same problem too. 

OK. Which leads me to something else:
as you said, loadItem takes care of this for the other cases (profiles with a tab container or splitted views, not just for a single frame). So I think it would be more logical and easier to understand/maintain if loadItem also took care of setting the active child in the case of a single frame. It would be less surprising for the caller of loadRootItem, and would fix all cases where we currently end up with no active child.

I'll test http://www.davidfaure.fr/2009/konqviewmanager.cpp.diff with your unittest ;)
Comment 19 David Faure 2009-10-16 11:47:28 UTC
It broke other tests due to tabContainer() instead of m_tabContainer, works now. (It was creating the tab container too early when e.g. loading a profile with a sidebar).

New diff for konqviewmanager.cpp (including your changes)
http://www.davidfaure.fr/2009/konqviewmanager.v2.diff

All unittests (including the ones you added) pass with this patch.

Looks ok to you?
Comment 20 Frank Reininghaus 2009-10-16 13:12:56 UTC
I can't test it now because I'm not at home, but the updated patch looks good to me :-) It looks like that should work in any case (at least the ones we've seen so far).

Do you want to commit it with the new unit tests, or shall I do it (could do it tonight)?
Comment 21 David Faure 2009-10-16 13:47:48 UTC
SVN commit 1036042 by dfaure:

Fix "Adding tab (Ctrl+T) in detached (tab) window crashes Konqueror".
Excellent investigation, unit-test, and suggested fixes by Frank Reininghaus.
Will backport for 4.3.3.
BUG: 203069


 M  +11 -2     konqviewmanager.cpp  
 M  +3 -2      konqviewmanager.h  
 M  +60 -0     tests/konqviewmgrtest.cpp  
 M  +2 -0      tests/konqviewmgrtest.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1036042