Bug 127474

Summary:

[GTK] Loading page into WebView shows g_closure_unref warning

Product:

WebKit

Reporter:

Tomas Popela <tpopela>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adamw, cgarcia, commit-queue, gustavo, jim, mcrha, mrobinson, paulepanter

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed patch	andersca: review-, andersca: commit-queue-
proposed patch ][	none

Tomas Popela

Reported 2014-01-23 03:36:38 PST

While loading email into WebView in Evolution with WK1 it's showing this warning. Everything loads and renders fine. I'm on http://trac.webkit.org/changeset/162517 (evolution:21890): GLib-GObject-CRITICAL **: g_closure_unref: assertion 'closure->ref_count > 0' failed Breakpoint 1, 0x0000003761450220 in g_logv () from /lib64/libglib-2.0.so.0 #0 0x0000003761450220 in g_logv () from /lib64/libglib-2.0.so.0 #1 0x000000376145063f in g_log () from /lib64/libglib-2.0.so.0 #2 0x00007ffff124e50e in WTF::derefGPtr<_GClosure> (ptr=0x38e5200) at ../../Source/WTF/wtf/gobject/GRefPtr.cpp:159 #3 0x00007ffff44360b0 in WTF::GRefPtr<_GClosure>::operator= (this=0x385f330, optr=0x0) at ../../Source/WTF/wtf/gobject/GRefPtr.h:142 #4 0x00007ffff4435e16 in WebCore::GObjectEventListener::gobjectDestroyed (this=0x385f300) at ../../Source/WebCore/bindings/gobject/GObjectEventListener.cpp:61 #5 0x00007ffff4435fca in WebCore::GObjectEventListener::gobjectDestroyedCallback (listener=0x385f300) at ../../Source/WebCore/bindings/gobject/GObjectEventListener.h:50 #6 0x000000376181409f in weak_refs_notify () from /lib64/libgobject-2.0.so.0 #7 0x0000003761814ee8 in g_object_unref () from /lib64/libgobject-2.0.so.0 #8 0x00007ffff443323c in WebKit::DOMObjectCache::clearByFrame (frame=0x322beb0) at ../../Source/WebCore/bindings/gobject/DOMObjectCache.cpp:109 #9 0x00007ffff2fc9c04 in WebKit::FrameLoaderClient::setMainFrameDocumentReady (this=0x3227a90, ready=false) at ../../Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:583 #10 0x00007ffff3990384 in WebCore::FrameLoader::closeOldDataSources (this=0x322bf48) at ../../Source/WebCore/loader/FrameLoader.cpp:2062 #11 0x00007ffff398f4f0 in WebCore::FrameLoader::commitProvisionalLoad (this=0x322bf48) at ../../Source/WebCore/loader/FrameLoader.cpp:1812 #12 0x00007ffff3970fa7 in WebCore::DocumentLoader::commitIfReady (this=0x3949520) at ../../Source/WebCore/loader/DocumentLoader.cpp:354 #13 0x00007ffff3972e96 in WebCore::DocumentLoader::commitLoad (this=0x3949520, data=0x35916f0 "<!DOCTYPE HTML>\n<html>\n<head>\n<meta name=\"generator\" content=\"Evolution Mail\"/>\n<title>Evolution Mail Display</title>\n</head>\n<body class=\"-e-web-view-background-color e-web-view-text-color\"> <style>"..., length=465) at ../../Source/WebCore/loader/DocumentLoader.cpp:765 #14 0x00007ffff397349f in WebCore::DocumentLoader::dataReceived (this=0x3949520, resource=0x0, data=0x35916f0 "<!DOCTYPE HTML>\n<html>\n<head>\n<meta name=\"generator\" content=\"Evolution Mail\"/>\n<title>Evolution Mail Display</title>\n</head>\n<body class=\"-e-web-view-background-color e-web-view-text-color\"> <style>"..., length=465) at ../../Source/WebCore/loader/DocumentLoader.cpp:892 #15 0x00007ffff3972dff in WebCore::DocumentLoader::continueAfterContentPolicy (this=0x3949520, policy=WebCore::PolicyUse) at ../../Source/WebCore/loader/DocumentLoader.cpp:752 #16 0x00007ffff397269f in WebCore::DocumentLoader::responseReceived (this=0x3949520, resource=0x0, response=...) at ../../Source/WebCore/loader/DocumentLoader.cpp:655 #17 0x00007ffff397169d in WebCore::DocumentLoader::handleSubstituteDataLoadNow (this=0x3949520) at ../../Source/WebCore/loader/DocumentLoader.cpp:475 #18 0x00007ffff397e9a4 in std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<WebCore::Timer<WebCore::DocumentLoader>*&, void> (this=0x393bee0, __object=0x3949520) at /usr/include/c++/4.8.2/functional:601 #19 0x00007ffff397e2d3 in std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::__call<void, , 0ul, 1ul>(std::tuple<>&&, std::_Index_tuple<0ul, 1ul>) ( this=0x393bee0, __args=<unknown type in /usr/local/lib/libwebkitgtk-3.0.so.0, CU 0x14f45aaa, DIE 0x1506fb7f>) at /usr/include/c++/4.8.2/functional:1296 #20 0x00007ffff397d3c2 in std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<, void>() (this=0x393bee0) at /usr/include/c++/4.8.2/functional:1355 #21 0x00007ffff397bb9f in std::_Function_handler<void (), std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)> >::_M_invoke(std::_Any_data const&) ( __functor=...) at /usr/include/c++/4.8.2/functional:2071 #22 0x00007ffff2fba9e6 in std::function<void ()>::operator()() const (this=0x3949d60) at /usr/include/c++/4.8.2/functional:2464 #23 0x00007ffff397f212 in WebCore::Timer<WebCore::DocumentLoader>::fired (this=0x3949d28) at ../../Source/WebCore/platform/Timer.h:130 #24 0x00007ffff31758d3 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x3207ac0) at ../../Source/WebCore/platform/ThreadTimers.cpp:132 #25 0x00007ffff3175781 in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:107 #26 0x00007ffff319b08e in WebCore::sharedTimerTimeoutCallback () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49 #27 0x0000003761449e43 in g_timeout_dispatch () from /lib64/libglib-2.0.so.0 #28 0x00000037614492a6 in g_main_context_dispatch () from /lib64/libglib-2.0.so.0 #29 0x0000003761449628 in g_main_context_iterate.isra.24 () from /lib64/libglib-2.0.so.0 #30 0x0000003761449a3a in g_main_loop_run () from /lib64/libglib-2.0.so.0 #31 0x000000324f9aa355 in gtk_main () from /lib64/libgtk-3.so.0 #32 0x0000000000404b5e in main (argc=1, argv=0x7fffffffd888) at main.c:680

Attachments
proposed patch (1.72 KB, patch) 2015-02-13 07:35 PST, Milan Crha	andersca: review- andersca: commit-queue-	Details Formatted Diff Diff
proposed patch ][ (1.71 KB, patch) 2015-02-16 00:35 PST, Milan Crha	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Adam Williamson

Comment 1 2014-04-30 09:47:52 PDT

Just to note that this is affecting Fedora Rawhide, I've been seeing it for a while.

Milan Crha

Comment 2 2015-01-07 03:28:55 PST

Still there with WebkitGTK 2.4.7

Milan Crha

Comment 3 2015-02-13 05:18:54 PST

Valgrind report for the warning from WebKitGTK+ 2.4.8: Invalid read of size 8 at 0x3A8020F529: g_closure_unref (gclosure.c:581) by 0xA7EBEF2: void WTF::derefGPtr<_GClosure>(_GClosure*) (GRefPtr.cpp:159) by 0x8663B0D: WTF::GRefPtr<_GClosure>::operator=(_GClosure*) (GRefPtr.h:142) by 0x8663872: WebCore::GObjectEventListener::gobjectDestroyed() (GObjectEventListener.cpp:61) by 0x8663A27: WebCore::GObjectEventListener::gobjectDestroyedCallback(WebCore::GObjectEventListener*, _GObject*) (GObjectEventListener.h:50) by 0x3A80213BDE: weak_refs_notify (gobject.c:2630) by 0x3A80214CBB: g_object_unref (gobject.c:3133) by 0x86619C6: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (DOMObjectCache.cpp:109) by 0x742980B: WebKit::FrameLoaderClient::setMainFrameDocumentReady(bool) (FrameLoaderClientGtk.cpp:583) by 0x7D29833: WebCore::FrameLoader::closeOldDataSources() (FrameLoader.cpp:2067) by 0x7D28B34: WebCore::FrameLoader::commitProvisionalLoad() (FrameLoader.cpp:1817) by 0x7D0A8BA: WebCore::DocumentLoader::commitIfReady() (DocumentLoader.cpp:354) by 0x7D0C4FC: WebCore::DocumentLoader::commitLoad(char const*, int) (DocumentLoader.cpp:765) by 0x7D0CA2C: WebCore::DocumentLoader::dataReceived(WebCore::CachedResource*, char const*, int) (DocumentLoader.cpp:892) by 0x7D0C440: WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) (DocumentLoader.cpp:752) by 0x7D0BD9F: WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) (DocumentLoader.cpp:655) by 0x7D0AEEC: WebCore::DocumentLoader::handleSubstituteDataLoadNow(WebCore::Timer<WebCore::DocumentLoader>*) (DocumentLoader.cpp:475) by 0x7D16E7F: void std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<WebCore::Timer<WebCore::DocumentLoader>*&, void>(WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*&) const (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14) by 0x7D16738: void std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::__call<void, , 0ul, 1ul>(std::tuple<>&&, std::_Index_tuple<0ul, 1ul>) (functional:1264) by 0x7D1590B: void std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<, void>() (functional:1323) by 0x7D145A6: std::_Function_handler<void (), std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)> >::_M_invoke(std::_Any_data const&) (functional:2039) by 0x741B7DB: std::function<void ()>::operator()() const (functional:2439) by 0x7D17623: WebCore::Timer<WebCore::DocumentLoader>::fired() (Timer.h:132) by 0x75A9892: WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:132) by 0x75A978A: WebCore::ThreadTimers::sharedTimerFired() (ThreadTimers.cpp:107) by 0x75D67BA: WebCore::sharedTimerTimeoutCallback(void*) (SharedTimerGtk.cpp:49) by 0x3A7F64A552: g_timeout_dispatch (gmain.c:4520) by 0x3A7F649AEA: g_main_dispatch (gmain.c:3111) by 0x3A7F649AEA: g_main_context_dispatch (gmain.c:3710) by 0x3A7F649E87: g_main_context_iterate.isra.29 (gmain.c:3781) by 0x3A7F64A1B1: g_main_loop_run (gmain.c:3975) by 0x319E9EBE84: gtk_main (gtkmain.c:1207) by 0x404B7F: main (main.c:629) Address 0x13bf5480 is 32 bytes inside a block of size 72 free'd at 0x4A07CE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x3A7F64F7FE: g_free (gmem.c:190) by 0xA7EBEF2: void WTF::derefGPtr<_GClosure>(_GClosure*) (GRefPtr.cpp:159) by 0x8663AAD: WTF::GRefPtr<_GClosure>::~GRefPtr() (GRefPtr.h:70) by 0x866374E: WebCore::GObjectEventListener::~GObjectEventListener() (GObjectEventListener.cpp:44) by 0x86637C7: WebCore::GObjectEventListener::~GObjectEventListener() (GObjectEventListener.cpp:49) by 0x7649707: WTF::RefCounted<WebCore::EventListener>::deref() (RefCounted.h:147) by 0x7649BA4: derefIfNotNull<WebCore::EventListener> (PassRefPtr.h:39) by 0x7649BA4: ~RefPtr (RefPtr.h:55) by 0x7649BA4: WebCore::RegisteredEventListener::~RegisteredEventListener() (RegisteredEventListener.h:32) by 0x78F199E: WebCore::removeListenerFromVector(WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>*, WebCore::EventListener*, bool, unsigned long&) (EventListenerMap.cpp:144) by 0x78F1A3F: WebCore::EventListenerMap::remove(WTF::AtomicString const&, WebCore::EventListener*, bool, unsigned long&) (EventListenerMap.cpp:153) by 0x78FA475: WebCore::EventTarget::removeEventListener(WTF::AtomicString const&, WebCore::EventListener*, bool) (EventTarget.cpp:88) by 0x791FC50: WebCore::tryRemoveEventListener(WebCore::Node*, WTF::AtomicString const&, WebCore::EventListener*, bool) (Node.cpp:1832) by 0x791FD36: WebCore::Node::removeEventListener(WTF::AtomicString const&, WebCore::EventListener*, bool) (Node.cpp:1868) by 0x8663851: WebCore::GObjectEventListener::gobjectDestroyed() (GObjectEventListener.cpp:60) by 0x8663A27: WebCore::GObjectEventListener::gobjectDestroyedCallback(WebCore::GObjectEventListener*, _GObject*) (GObjectEventListener.h:50) by 0x3A80213BDE: weak_refs_notify (gobject.c:2630) by 0x3A80214CBB: g_object_unref (gobject.c:3133) by 0x86619C6: WebKit::DOMObjectCache::clearByFrame(WebCore::Frame*) (DOMObjectCache.cpp:109) by 0x742980B: WebKit::FrameLoaderClient::setMainFrameDocumentReady(bool) (FrameLoaderClientGtk.cpp:583) by 0x7D29833: WebCore::FrameLoader::closeOldDataSources() (FrameLoader.cpp:2067) by 0x7D28B34: WebCore::FrameLoader::commitProvisionalLoad() (FrameLoader.cpp:1817) by 0x7D0A8BA: WebCore::DocumentLoader::commitIfReady() (DocumentLoader.cpp:354) by 0x7D0C4FC: WebCore::DocumentLoader::commitLoad(char const*, int) (DocumentLoader.cpp:765) by 0x7D0CA2C: WebCore::DocumentLoader::dataReceived(WebCore::CachedResource*, char const*, int) (DocumentLoader.cpp:892) by 0x7D0C440: WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) (DocumentLoader.cpp:752) by 0x7D0BD9F: WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) (DocumentLoader.cpp:655) by 0x7D0AEEC: WebCore::DocumentLoader::handleSubstituteDataLoadNow(WebCore::Timer<WebCore::DocumentLoader>*) (DocumentLoader.cpp:475) by 0x7D16E7F: void std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<WebCore::Timer<WebCore::DocumentLoader>*&, void>(WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*&) const (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14) by 0x7D16738: void std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::__call<void, , 0ul, 1ul>(std::tuple<>&&, std::_Index_tuple<0ul, 1ul>) (functional:1264) by 0x7D1590B: void std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)>::operator()<, void>() (functional:1323) by 0x7D145A6: std::_Function_handler<void (), std::_Bind<std::_Mem_fn<void (WebCore::DocumentLoader::*)(WebCore::Timer<WebCore::DocumentLoader>*)> (WebCore::DocumentLoader*, WebCore::Timer<WebCore::DocumentLoader>*)> >::_M_invoke(std::_Any_data const&) (functional:2039) by 0x741B7DB: std::function<void ()>::operator()() const (functional:2439) by 0x7D17623: WebCore::Timer<WebCore::DocumentLoader>::fired() (Timer.h:132) by 0x75A9892: WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:132) by 0x75A978A: WebCore::ThreadTimers::sharedTimerFired() (ThreadTimers.cpp:107) by 0x75D67BA: WebCore::sharedTimerTimeoutCallback(void*) (SharedTimerGtk.cpp:49) by 0x3A7F64A552: g_timeout_dispatch (gmain.c:4520) by 0x3A7F649AEA: g_main_dispatch (gmain.c:3111) by 0x3A7F649AEA: g_main_context_dispatch (gmain.c:3710) by 0x3A7F649E87: g_main_context_iterate.isra.29 (gmain.c:3781) by 0x3A7F64A1B1: g_main_loop_run (gmain.c:3975) by 0x319E9EBE84: gtk_main (gtkmain.c:1207) by 0x404B7F: main (main.c:629)

Milan Crha

Comment 4 2015-02-13 07:35:55 PST

Created attachment 246524 [details] proposed patch This was a use-after-free in case when the target had the last reference to the object, then the call to target->removeEventListener() caused the object's destruction, thus the assignment after the call, m_handler = 0;, was done on an already freed object. Adding a temporary reference and dereference it at the very end, as the last thing in the function, fixed the runtime warning and the invalid memory usage. Please include it in the webkit1 release too.

WebKit Commit Bot

Comment 5 2015-02-13 07:37:12 PST

Attachment 246524 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Anders Carlsson

Comment 6 2015-02-13 10:32:02 PST

Comment on attachment 246524 [details] proposed patch This looks good conceptually, but please use a Ref protector instead of calling ref/deref explicitly!

Carlos Garcia Campos

Comment 7 2015-02-13 10:42:06 PST

Comment on attachment 246524 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=246524&action=review Do we have a simple test case or any other way to reproduce this? If we could add at least a unit test it would be perfect. > Source/WebCore/bindings/gobject/GObjectEventListener.cpp:58 > + // Add one reference in case the 'target' holds the last reference, > + // which may cause, inside removeEventListener(), free of this object > + // and later use-after-free with the m_handler = 0; assignment. > + ref(); As Anders suggests, you could do something like RefPtr<GObjectEventListener> protect(this); instead of calling ref/deref.

Milan Crha

Comment 8 2015-02-15 23:13:47 PST

(In reply to comment #7) > Do we have a simple test case or any other way to reproduce this? If we > could add at least a unit test it would be perfect. I didn't try it, but I suppose this is related to GTK+ widgets/plugins, which Evolution uses, thus a dead end in case of WebKit2. > As Anders suggests, you could do something like RefPtr<GObjectEventListener> > protect(this); instead of calling ref/deref. This is a mater of taste. I do not like to rely on compiler optimizations of an auto_pointer-like classes and their freeing in the right time, thus calling ref/defer "directly" ensures that the call is done always in the right time, instead of in the time when the compiler decides it's the best time for it.

Carlos Garcia Campos

Comment 9 2015-02-15 23:29:31 PST

(In reply to comment #8) > (In reply to comment #7) > > Do we have a simple test case or any other way to reproduce this? If we > > could add at least a unit test it would be perfect. > > I didn't try it, but I suppose this is related to GTK+ widgets/plugins, > which Evolution uses, thus a dead end in case of WebKit2. > > > As Anders suggests, you could do something like RefPtr<GObjectEventListener> > > protect(this); instead of calling ref/deref. > > This is a mater of taste. I do not like to rely on compiler optimizations of > an auto_pointer-like classes and their freeing in the right time, thus > calling ref/defer "directly" ensures that the call is done always in the > right time, instead of in the time when the compiler decides it's the best > time for it. WebKit uses RefPtr everywhere and the implicit ref/deref is the preferred way.

Milan Crha

Comment 10 2015-02-16 00:35:54 PST

Created attachment 246638 [details] proposed patch ][ Uses RefPtr template now, also done a littel cleanup in the function. No unit tests added, though.

Carlos Garcia Campos

Comment 11 2015-02-16 01:39:27 PST

Comment on attachment 246638 [details] proposed patch ][ Perfect, thanks!

WebKit Commit Bot

Comment 12 2015-02-16 02:24:19 PST

Comment on attachment 246638 [details] proposed patch ][ Clearing flags on attachment: 246638 Committed r180141: <http://trac.webkit.org/changeset/180141>

WebKit Commit Bot

Comment 13 2015-02-16 02:24:24 PST

All reviewed patches have been landed. Closing bug.

Paul Menzel

Comment 14 2015-03-14 02:10:03 PDT

This is report #780444 [1] in the Debian bug tracking system. [1] https://bugs.debian.org/780444

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