Move management of QWebEngineProfile to WebEnginePage, to avoid crash on KMail exit or viewer window close
ClosedPublic

Authored by marten on Mar 6 2019, 9:59 AM.

Details

Summary

The crash happens with both Qt 5.12 and 5.13, either on quitting KMail or closing a separate mail viewer window. The backtrace is several pages long, but the hopefully relevant parts are:

Application: KMail (kmail), signal: Segmentation fault
Using host libthread_db library "/lib64/libthread_db.so.1".
[Current thread is 1 (Thread 0x7f68e493fd40 (LWP 4419))]

Thread 1 (Thread 0x7f68e493fd40 (LWP 4419)):
[KCrash Handler]
#7 0x00007f68d581e09a in QWebEnginePagePrivate::bindPageAndWidget (page=0x0, widget=0x5620d4c1c200) at /var/tmp/portage/dev-qt/qtwebengine-5.12.9999/work/qtwebengine-5.12.9999/src/webenginewidgets/api/qwebenginepage.h:358
#8 0x00007f68d582fa91 in QtWebEngineCore::RenderWidgetHostViewQtDelegateWidget::~RenderWidgetHostViewQtDelegateWidget (this=0x5620d4c1c200, in_chrg=<optimized out>) at /var/tmp/portage/dev-qt/qtwebengine-5.12.9999/work/qtwebengine-5.12.9999/src/webenginewidgets/render_widget_host_view_qt_delegate_widget.cpp:174
...
#31 QWebEnginePagePrivate::~QWebEnginePagePrivate (this=<optimized out>,
in_chrg=<optimized out>) at /var/tmp/portage/dev-qt/qtwebengine-5.12.9999/work/qtwebengine-5.12.9999/src/webenginewidgets/api/qwebenginepage.cpp:263
#32 0x00007f68d58241a9 in QWebEnginePagePrivate::~QWebEnginePagePrivate (this=0x5620d4a6d530, in_chrg=<optimized out>) at /var/tmp/portage/dev-qt/qtwebengine-5.12.9999/work/qtwebengine-5.12.9999/src/webenginewidgets/api/qwebenginepage.cpp:263
#33 0x00007f68d581adca in QWebEnginePagePrivate::releaseProfile (this=0x5620d4a6d530) at /usr/include/qt5/QtCore/qscopedpointer.h:162
#34 0x00007f68ce4bc61e in QtWebEngineCore::ProfileAdapter::~ProfileAdapter (this=0x5620d4abc080,
in_chrg=<optimized out>) at /usr/include/qt5/QtCore/qarraydata.h:211
...
#41 0x00007f68d5826b69 in QWebEngineProfile::~QWebEngineProfile (this=0x5620d4a950c0, in_chrg=<optimized out>) at /var/tmp/portage/dev-qt/qtwebengine-5.12.9999/work/qtwebengine-5.12.9999/src/webenginewidgets/api/qwebengineprofile.cpp:321
#42 0x00007f68de8b4ef4 in QObjectPrivate::deleteChildren() () at /var/tmp/portage/dev-qt/qtcore-5.12.9999/work/qtcore-5.12.9999/src/corelib/kernel/qobject.cpp:2010
#43 0x00007f68df6ac706 in QWidget::~QWidget() () at /var/tmp/portage/dev-qt/qtwidgets-5.12.9999/work/qtwidgets-5.12.9999/src/widgets/kernel/qwidget.cpp:1703
#44 0x00007f68db62a389 in MessageViewer::MailWebEngineView::~MailWebEngineView (this=0x5620d4aaea00,
in_chrg=<optimized out>) at messagelib/messageviewer/src/viewer/webengine/mailwebengineview.cpp:120
#45 0x00007f68db5c9754 in MessageViewer::ViewerPrivate::~ViewerPrivate (this=0x5620d492a900, in_chrg=<optimized out>) at messagelib/messageviewer/src/viewer/viewer_p.cpp:261
#46 0x00007f68db5c98c9 in MessageViewer::ViewerPrivate::~ViewerPrivate (this=0x5620d492a900,
in_chrg=<optimized out>) at messagelib/messageviewer/src/viewer/viewer_p.cpp:256
#47 0x00007f68de8b4ef4 in QObjectPrivate::deleteChildren() () at /var/tmp/portage/dev-qt/qtcore-5.12.9999/work/qtcore-5.12.9999/src/corelib/kernel/qobject.cpp:2010
#48 0x00007f68df6ac706 in QWidget::~QWidget() () at /var/tmp/portage/dev-qt/qtwidgets-5.12.9999/work/qtwidgets-5.12.9999/src/widgets/kernel/qwidget.cpp:1703
#49 0x00007f68db5bc9a9 in MessageViewer::Viewer::~Viewer (this=0x5620d4af8f40, __in_chrg=<optimized out>) at messagelib/messageviewer/src/viewer/viewer.cpp:84

An ominous debug message is also printed just before the crash, by QWebEnginePagePrivate::releaseProfile() in ./src/webenginewidgets/api/qwebenginepage.cpp:

kmail/default unknown: Release of profile requested but WebEnginePage still not deleted. Expect troubles !

I haven't been able to resolve the root cause of this, but there is a clue in the above message and in the QWebEnginePage API documentation:

"If the profile is not the default profile, the caller must ensure that the profile stays alive for as long as the page does."

The fix appears to be to explicitly delete the MailWebEnginePage in the MailWebEngineView destructor, so that the page is deleted before the profile (as a child of the MailWebEngineView) is deleted.

Test Plan

Built messagelib with this change, observed no crash when quitting KMail or closing a message viewer window. Without this change, the crash happens every time.

Build Akregator using the updated messagelib, observed correct operation and display of articles and pages.

Diff Detail

Repository
R94 PIM: Message Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
marten created this revision.Mar 6 2019, 9:59 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptMar 6 2019, 9:59 AM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
marten requested review of this revision.Mar 6 2019, 9:59 AM

Nice job, thanks for digging into this!

Maybe add a comment to the code why is this necessary so that it doesn't get accidentally removed in the future.

marten updated this revision to Diff 53282.Mar 6 2019, 2:35 PM

Something like this...?

Now that I think about it, wouldn't it be cleaner if the QWebEngineProfile was instantiated inside of MailWebEnginePage and the page would be set as a parent for the profile? That way the profile would be destroyed automatically by Qt, but after MailWebEnginePage destructor, so still (and always!) in the correct order...could you test it, please?

marten added a comment.Mar 6 2019, 9:50 PM

Sounds a good idea, will investigate.

marten updated this revision to Diff 53340.Mar 7 2019, 9:37 AM
marten retitled this revision from Explicitly delete MailWebEnginePage to avoid crash on KMail exit or viewer window close to Reassign ownership of QWebEngineProfile in MailWebEnginePage, to avoid crash on KMail exit or viewer window close.

Yes, doing that seems to work. It's not as elegant an implementation as it could be, because the QWebEngineProfile still needs to be created by MailWebEngineView and passed to the MailWebEnginePage which adopts it as a child. It's not possible to manage the profile entirely within MailWebEnginePage because a profile can only be set when a QWebEnginePage is created, and trying to do that in the constructor:

MailWebEnginePage::MailWebEnginePage(QObject *parent)
  : WebEngineViewer::WebEnginePage(new QWebEngineProfile(this), parent)
{
  initialise();
}

crashes because the profile attempts to call back into the QWebEnginePage which is not fully constructed yet.

anthonyfieroni added inline comments.
messageviewer/src/viewer/webengine/mailwebenginepage.cpp
40

It should be done in WebEngineViewer::WebEnginePage, to be applied to all pages not only to Mail.

messageviewer/src/viewer/webengine/mailwebengineview.cpp
83–87

It's not needed to change these lines, while you reparent inside.

marten updated this revision to Diff 53357.Mar 7 2019, 12:02 PM

There's always a better way to do it...

Handle the profile entirely within the MailWebEnginePage, since the MailWebEngineView doesn't do anything with it. Allows one MailWebEnginePage constructor, and the explicit destructor, to be eliminated.

dvratil accepted this revision.Mar 7 2019, 12:23 PM

Nice, good solution :)

This revision is now accepted and ready to land.Mar 7 2019, 12:23 PM
marten added a comment.Mar 7 2019, 1:15 PM

@dvratil: in light of @anthonyfieroni's comments, should this fix go into WebEnginePage (messagelib/webengineviewer/src/webenginepage.cpp) instead?

@anthonyfieroni: are there any other applications within KDE PIM or elsewhere using WebEnginePage, so that I can build and test with them? If this fix (latest diff 53357) were to be moved into WebEnginePage then it would remove the option for any user of that class to set their own QWebEngineProfile (because MailWebEngineView doesn't need to and will not).

As far as I can see WebEnginePage is also used by Akregator (subclassed as ArticleViewerWebEnginePage, so moving it one level up sounds like a good idea. None of them seems to provide a customized QWebEngineProfile, in both cases they just access the profile afterwords through profile() getter to adjust the settings.

marten updated this revision to Diff 53415.Mar 8 2019, 8:52 AM
marten retitled this revision from Reassign ownership of QWebEngineProfile in MailWebEnginePage, to avoid crash on KMail exit or viewer window close to Move management of QWebEngineProfile to WebEnginePage, to avoid crash on KMail exit or viewer window close.
marten edited the test plan for this revision. (Show Details)

Setup and destruction of the QWebEngineProfile moved to WebEnginePage, so MailWebEngineView need know nothing about the profile and MailWebEnginePage only needs to access it via profile().

Akregator currently constructs a WebEnginePage using its own profile, which however is always the default profile. This works, because all web views with Akregator use the same browser settings, but changing Akregator and eliminating the 2-argument WebEnginePage constructor would mean that releases of Akregator and messagelib would have to be synchronised exactly. For the moment I've just marked the 2-argument constructor as deprecated, and Akregator can be updated at a later date.

dvratil accepted this revision.Mar 11 2019, 5:59 PM

Looks good. Could you also create a patch for Akregator to use the non-deprecated ctor?

This revision was automatically updated to reflect the committed changes.

Will let the Akregator change soak for a bit, to make sure there are no problems, and then submit it.