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: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tomas Popela
2014-01-23 03:36:38 PST
Just to note that this is affecting Fedora Rawhide, I've been seeing it for a while. Still there with WebkitGTK 2.4.7 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) 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.
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 on attachment 246524 [details]
proposed patch
This looks good conceptually, but please use a Ref protector instead of calling ref/deref explicitly!
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. (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. (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. Created attachment 246638 [details]
proposed patch ][
Uses RefPtr template now, also done a littel cleanup in the function. No unit tests added, though.
Comment on attachment 246638 [details]
proposed patch ][
Perfect, thanks!
Comment on attachment 246638 [details] proposed patch ][ Clearing flags on attachment: 246638 Committed r180141: <http://trac.webkit.org/changeset/180141> All reviewed patches have been landed. Closing bug. This is report #780444 [1] in the Debian bug tracking system. [1] https://bugs.debian.org/780444 |