Bug 1504503 - [abrt] Crash under e_dom_resize_document_content_to_preview_width()
Summary: [abrt] Crash under e_dom_resize_document_content_to_preview_width()
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: evolution
Version: 27
Hardware: x86_64
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Milan Crha
QA Contact: Fedora Extras Quality Assurance
URL: https://retrace.fedoraproject.org/faf...
Whiteboard: abrt_hash:4192cea1ee840bd56c3f11b8f12...
: 1509237 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-10-20 03:57 UTC by Davide Repetto
Modified: 2017-11-06 10:35 UTC (History)
12 users (show)

Fixed In Version: evolution-3.26.2
Clone Of:
Environment:
Last Closed: 2017-10-23 17:09:46 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
File: backtrace (86.07 KB, text/plain)
2017-10-20 03:57 UTC, Davide Repetto
no flags Details
File: cgroup (289 bytes, text/plain)
2017-10-20 03:57 UTC, Davide Repetto
no flags Details
File: core_backtrace (40.37 KB, text/plain)
2017-10-20 03:57 UTC, Davide Repetto
no flags Details
File: cpuinfo (1.04 KB, text/plain)
2017-10-20 03:57 UTC, Davide Repetto
no flags Details
File: dso_list (16.70 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: environ (2.77 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: exploitable (93 bytes, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: limits (1.29 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: maps (81.51 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: open_fds (1.25 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: proc_pid_status (1.30 KB, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details
File: var_log_messages (114 bytes, text/plain)
2017-10-20 03:58 UTC, Davide Repetto
no flags Details

Description Davide Repetto 2017-10-20 03:57:43 UTC
Version-Release number of selected component:
webkitgtk4-2.18.0-2.fc27

Additional info:
reporter:       libreport-2.9.2
backtrace_rating: 4
cmdline:        /usr/libexec/webkit2gtk-4.0/WebKitWebProcess 63
crash_function: webkit_dom_document_get_document_element
executable:     /usr/libexec/webkit2gtk-4.0/WebKitWebProcess
journald_cursor: s=f88bbcb6f6d34c15a475041d9a40241f;i=1e3807;b=72e0797557c24d7d975985de73db5a58;m=b08163b0;t=55bf186c8f748;x=d3f92255f84ff165
kernel:         4.13.8-300.fc27.x86_64
rootdir:        /
runlevel:       N 5
type:           CCpp
uid:            1000

Truncated backtrace:
Thread no. 1 (10 frames)
 #0 webkit_dom_document_get_document_element at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocument.cpp:1435
 #1 e_dom_resize_document_content_to_preview_width at /usr/src/debug/evolution-3.26.1-1.fc27.x86_64/src/web-extensions/e-dom-utils.c:990
 #2 ffi_call_unix64 at ../src/x86/unix64.S:76
 #3 ffi_call at ../src/x86/ffi64.c:525
 #4 g_cclosure_marshal_generic at gclosure.c:1490
 #6 WebKit::GObjectEventListener::handleEvent at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/GObjectEventListener.cpp:77
 #7 WebCore::EventTarget::fireEventListeners at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebCore/dom/EventTarget.cpp:277
 #9 WebCore::DOMWindow::dispatchEvent at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebCore/page/DOMWindow.cpp:1980
 #10 WebCore::Document::dispatchWindowEvent at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebCore/dom/Document.cpp:4202
 #11 WebCore::FrameView::sendResizeEventIfNeeded at /usr/src/debug/webkitgtk4-2.18.0-2.fc27.x86_64/Source/WebCore/page/FrameView.cpp:3652

Comment 1 Davide Repetto 2017-10-20 03:57:51 UTC
Created attachment 1341138 [details]
File: backtrace

Comment 2 Davide Repetto 2017-10-20 03:57:53 UTC
Created attachment 1341139 [details]
File: cgroup

Comment 3 Davide Repetto 2017-10-20 03:57:56 UTC
Created attachment 1341140 [details]
File: core_backtrace

Comment 4 Davide Repetto 2017-10-20 03:57:57 UTC
Created attachment 1341141 [details]
File: cpuinfo

Comment 5 Davide Repetto 2017-10-20 03:58:00 UTC
Created attachment 1341142 [details]
File: dso_list

Comment 6 Davide Repetto 2017-10-20 03:58:01 UTC
Created attachment 1341143 [details]
File: environ

Comment 7 Davide Repetto 2017-10-20 03:58:03 UTC
Created attachment 1341144 [details]
File: exploitable

Comment 8 Davide Repetto 2017-10-20 03:58:05 UTC
Created attachment 1341145 [details]
File: limits

Comment 9 Davide Repetto 2017-10-20 03:58:08 UTC
Created attachment 1341146 [details]
File: maps

Comment 10 Davide Repetto 2017-10-20 03:58:11 UTC
Created attachment 1341147 [details]
File: open_fds

Comment 11 Davide Repetto 2017-10-20 03:58:12 UTC
Created attachment 1341148 [details]
File: proc_pid_status

Comment 12 Davide Repetto 2017-10-20 03:58:14 UTC
Created attachment 1341149 [details]
File: var_log_messages

Comment 13 Milan Crha 2017-10-20 07:00:59 UTC
Thanks for a bug report. Do you know when this crash happened, please? I suppose when you've been (quickly?) moving between messages or something like that?

Even the backtrace skipped that part, it most likely happened within dom_window_resize_cb(), which takes as an argument a WebKitDOMDocument instance which had been relevant in time of the corresponding webkit_dom_event_target_add_event_listener() call.

Tomas, I do not know how this works, you connected the "resize" signal on a WebKitDOMDOMWindow. When the document of the window changes (like when it's freed), is the window also freed, or it is reused for another instance of the document and the previous listeners are not removed until the next call to e_dom_utils_e_mail_display_bind_dom()? Might it be possible to get the document from the elements passed to dom_window_resize_cb(), instead of providing it as a user data? That should make it pretty safe, always using up-to-date document instance, not even needing to re-add the signal listener.

I fixed a similar crash at [1], but it doesn't seem to be enough or the new WebKitGTK+ changed behaviour (being it the later, I consider updating all active Fedoras with the latest WebKitGTK+ harmful, not harmless, but that's only my personal opinion).

[1] https://bugzilla.gnome.org/show_bug.cgi?id=739955#c22

Comment 14 Tomas Popela 2017-10-20 07:23:04 UTC
(In reply to Milan Crha from comment #13)
> Tomas, I do not know how this works, you connected the "resize" signal on a
> WebKitDOMDOMWindow. When the document of the window changes (like when it's
> freed), is the window also freed, or it is reused for another instance of
> the document

I don't think that the window is freed when the document is destroyed. But it is freed when we load a new URI to the web view (i.e. when user jumps to the other message).

> and the previous listeners are not removed until the next call
> to e_dom_utils_e_mail_display_bind_dom()?

Yes, could be, maybe it would make sense to connect to beforeunload event[0] and remove the listeners there.

> Might it be possible to get the
> document from the elements passed to dom_window_resize_cb(), instead of
> providing it as a user data? That should make it pretty safe, always using
> up-to-date document instance, not even needing to re-add the signal listener.

Yes, that's possible to get the document from the element, by calling webkit_dom_node_get_owner_document().

> I fixed a similar crash at [1], but it doesn't seem to be enough or the new
> WebKitGTK+ changed behaviour (being it the later, I consider updating all
> active Fedoras with the latest WebKitGTK+ harmful, not harmless, but that's
> only my personal opinion).

And Fedora users are lucky that it's only your opinion. Their security is more important.

[0] - https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload

Comment 15 Milan Crha 2017-10-20 08:39:38 UTC
(In reply to Tomas Popela from comment #14)
> ...

Okay, thanks for all the pointers. I may try it, though I'm currently unsure how to verify it'll really fix the issue. It theoretically should, though I'd like to be sure too. Nonetheless, even if I'd not reproduce it, I'll do the change to try to avoid the crash.

> And Fedora users are lucky that it's only your opinion. Their security is
> more important.

Sure, security fixes are important, I'm not questioning that. I only see a significant difference in backporting security fixes into basically frozen distribution (not frozen for security fixes) and providing completely new package version to such frozen distributions, which can cause regressions and misbehaviour to the package users. Having secure product which cannot be used by others as it could be before the update kind of lacks sense. And it often happens that the (6 months) development cycle fixes many things, but also breaks other things (at least for me, the horizontal scroll bar broke, but I cannot tell you when, even I have older fedoras installed, because they all have the same webkitgtk4 now). Anyway, I do not want to argue here about such things, it's not the right place for it. Of course, feel free to respond, if you want; we have just different opinion on this subject.

Comment 16 Tomas Popela 2017-10-20 09:08:09 UTC
(In reply to Milan Crha from comment #15)
> Sure, security fixes are important, I'm not questioning that. I only see a
> significant difference in backporting security fixes into basically frozen
> distribution (not frozen for security fixes) and providing completely new
> package version to such frozen distributions, which can cause regressions
> and misbehaviour to the package users. Having secure product which cannot be
> used by others as it could be before the update kind of lacks sense. And it
> often happens that the (6 months) development cycle fixes many things, but
> also breaks other things (at least for me, the horizontal scroll bar broke,
> but I cannot tell you when, even I have older fedoras installed, because
> they all have the same webkitgtk4 now).

We took the path of risking some regressions (that we be solved promptly by the whole WebKitGTK+ team), but for having the most secure WebKitGTK+ up to date. Also you can use previous versions of WebKitGTK+ from koji.

Comment 17 Milan Crha 2017-10-20 09:46:34 UTC
(In reply to Tomas Popela from comment #16)
> We took the path of risking some regressions (that we be solved promptly by
> the whole WebKitGTK+ team), but for having the most secure WebKitGTK+ up to
> date.

Yup, that makes perfect sense. Thanks.

Comment 18 Davide Repetto 2017-10-20 09:51:31 UTC
(In reply to Milan Crha from comment #13)
> Thanks for a bug report. Do you know when this crash happened, please? I
> suppose when you've been (quickly?) moving between messages or something
> like that?

Yes. It was most likely that. Moving between messages and deleting them.
In fact I was going through the SPAM...

Comment 19 Michael Catanzaro 2017-10-20 15:48:20 UTC
Careful, onbeforeunload is a bit tricky with WebKitGTK+... it only runs when you call webkit_web_view_try_close(), which most applications probably don't do at all.

Comment 20 Milan Crha 2017-10-23 17:09:46 UTC
I fixed it upstream with the below change. Instead of using passed-in WebKitDOMDocument valid from time of the event subscribing, get the current document from the object which sent the signal.

Created commit 929dbca69c in evo master (3.27.2+) [1]
Created commit 6fd3d59c28 in evo gnome-3-26 (3.26.2+)

[1] https://git.gnome.org/browse/evolution/commit/?id=929dbca69c

Comment 21 Milan Crha 2017-11-06 10:35:48 UTC
*** Bug 1509237 has been marked as a duplicate of this bug. ***


Note You need to log in before you can comment on or make changes to this bug.