Bug 127474 - [GTK] Loading page into WebView shows g_closure_unref warning
Summary: [GTK] Loading page into WebView shows g_closure_unref warning
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-23 03:36 PST by Tomas Popela
Modified: 2015-03-14 02:10 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 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
Comment 1 Adam Williamson 2014-04-30 09:47:52 PDT
Just to note that this is affecting Fedora Rawhide, I've been seeing it for a while.
Comment 2 Milan Crha 2015-01-07 03:28:55 PST
Still there with WebkitGTK 2.4.7
Comment 3 Milan Crha 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)
Comment 4 Milan Crha 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.
Comment 5 WebKit Commit Bot 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.
Comment 6 Anders Carlsson 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!
Comment 7 Carlos Garcia Campos 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.
Comment 8 Milan Crha 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.
Comment 9 Carlos Garcia Campos 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.
Comment 10 Milan Crha 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.
Comment 11 Carlos Garcia Campos 2015-02-16 01:39:27 PST
Comment on attachment 246638 [details]
proposed patch ][

Perfect, thanks!
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2015-02-16 02:24:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Paul Menzel 2015-03-14 02:10:03 PDT
This is report #780444 [1] in the Debian bug tracking system.

[1] https://bugs.debian.org/780444