Bug 118788 – [GTK] WebKitDOM objects leaking

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tomas Popela]

On recent trunk I noticed that we are leaking some WebKitDOM objects like Range or Node. 

tpopela  ~/dev/webkitgtk3/webkitgtk-2.0.3/Programs/unittests  >  13:09  [f19]  
$ ./testdomnode 
/webkit/domnode/test_hierarchy_navigation: OK
/webkit/domnode/test_insertion: OK

tpopela  ~/dev/WebKit/WebKitBuild/Debug/Programs/unittests  >  13:06  [master]  
$ ./testdomnode 
/webkit/domnode/test_hierarchy_navigation: OK
/webkit/domnode/test_insertion: OK
LEAK: 5 WebCoreNode

It is even bigger, when testing WebKit based composer for Evolution (4 lines email written in composer):

tpopela  ~/dev/evolution/build/test/bin  >  13:04   
$ ./evolution 
LEAK: 20 CachedResource
LEAK: 7191 Range
LEAK: 4155 WebCoreNode

[Martin Robinson]

DOM nodes are cached and released after a short timeout. Can you verify that after spinning the main loop for a while this doesn't happen automatically?

[Tomas Popela]

No, it does not help.

[Sergio Villar Senin]

(In reply to comment #0)

> tpopela  ~/dev/evolution/build/test/bin  >  13:04   
> $ ./evolution 
> LEAK: 20 CachedResource
> LEAK: 7191 Range
> LEAK: 4155 WebCoreNode

Don't know exactly about DOM objects, but note that the "leaks" in CachedResources are not really leaks. WebKit does not free cached resources on program exit on purpose, because the program is actually finishing so they'll be eventually released anyway and we're speeding up the exit process.

[Zan Dobersek]

(In reply to comment #3)
> (In reply to comment #0)
> 
> > tpopela  ~/dev/evolution/build/test/bin  >  13:04   
> > $ ./evolution 
> > LEAK: 20 CachedResource
> > LEAK: 7191 Range
> > LEAK: 4155 WebCoreNode
> 
> Don't know exactly about DOM objects, but note that the "leaks" in CachedResources are not really leaks. WebKit does not free cached resources on program exit on purpose, because the program is actually finishing so they'll be eventually released anyway and we're speeding up the exit process.

This number might be lower after r152340.
http://trac.webkit.org/changeset/152340

[Milan Crha]

Created attachment 246517 [details]
wk-leaks.c

Test application (first line shows how to compile & run it), which makes it easier to reproduce the leaks. Similar code (update_styles()) is used in Evolution and called to quite few times on various occasions.

The thing is that objects like WebKitDOMCSSRuleList (returned from webkit_dom_css_style_sheet_get_css_rules()) and WebKitDOMCSSRule (returned from webkit_dom_css_rule_list_item()) are not freed on frame reload, but a new objects are returned anyway.

The reason is that DOMObjectCache::clearByFrame() is called with a valid frame, but these objkects (and many other DOM objects) are stored in the cache with NULL data->frame.

My opinion: It won't work to always remove objects which has data->frame == NULL in the cache on any frame cleanup, because that would influence (invalidate) GObject-s from other web views (if there are more instances).

[Carlos Garcia Campos]

(In reply to comment #5)
> Created attachment 246517 [details]
> wk-leaks.c
> 
> Test application (first line shows how to compile & run it), which makes it
> easier to reproduce the leaks. Similar code (update_styles()) is used in
> Evolution and called to quite few times on various occasions.
> 
> The thing is that objects like WebKitDOMCSSRuleList (returned from
> webkit_dom_css_style_sheet_get_css_rules()) and WebKitDOMCSSRule (returned
> from webkit_dom_css_rule_list_item()) are not freed on frame reload, but a
> new objects are returned anyway.
> 
> The reason is that DOMObjectCache::clearByFrame() is called with a valid
> frame, but these objkects (and many other DOM objects) are stored in the
> cache with NULL data->frame.
> 
> My opinion: It won't work to always remove objects which has data->frame ==
> NULL in the cache on any frame cleanup, because that would influence
> (invalidate) GObject-s from other web views (if there are more instances).

I think the actual bug are the gtk-doc tags used, because not all GObject DOM bindings are supposed to be returned as transfer-none. See the original bug #40302 and the commits message:

"Manual management (calling g_object_unref on them) is also allowed, and is required for objects that are not in the DOM tree (eg NodeLists)."

I forgot about those when I added the transfer annotations to the gtk-doc :-(

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 246517 [details]
> > wk-leaks.c
> > 
> > Test application (first line shows how to compile & run it), which makes it
> > easier to reproduce the leaks. Similar code (update_styles()) is used in
> > Evolution and called to quite few times on various occasions.
> > 
> > The thing is that objects like WebKitDOMCSSRuleList (returned from
> > webkit_dom_css_style_sheet_get_css_rules()) and WebKitDOMCSSRule (returned
> > from webkit_dom_css_rule_list_item()) are not freed on frame reload, but a
> > new objects are returned anyway.
> > 
> > The reason is that DOMObjectCache::clearByFrame() is called with a valid
> > frame, but these objkects (and many other DOM objects) are stored in the
> > cache with NULL data->frame.
> > 
> > My opinion: It won't work to always remove objects which has data->frame ==
> > NULL in the cache on any frame cleanup, because that would influence
> > (invalidate) GObject-s from other web views (if there are more instances).
> 
> I think the actual bug are the gtk-doc tags used, because not all GObject
> DOM bindings are supposed to be returned as transfer-none. See the original
> bug #40302 and the commits message:
> 
> "Manual management (calling g_object_unref on them) is also allowed, and is
> required for objects that are not in the DOM tree (eg NodeLists)."
> 
> I forgot about those when I added the transfer annotations to the gtk-doc :-(

But there's still a problem. A lot of DOM objects are initially created and then added to a container, so they are not initially in document and we don't track their frame. I wonder if we should simply remove the inDocument check and always track the frame.

[Carlos Garcia Campos]

I have reworked the DOMObjectCache in bug #141558, removing the inDocument condition and it seems to work for all the WebKit2 unit tests.

[Milan Crha]

Created attachment 246821 [details]
wk-leaks.c ][

Updated test program. It also checks when the returned pointers are freed and which are eventually leaked and prints that on console. The memory-related lines start with "\t#" in the debug output.

[Milan Crha]

Created attachment 246823 [details]
wk-leaks.c ]I[

I'm sorry, one mo update, to not leak the object in the code as such, at the end of add_css_rule_into_style_sheet(). Tomas pointed it out on IRC.

[Milan Crha]

Few more observations found (by Tomas). When using
   webkit_dom_event_target_add_event_listener()
on a webkit GObject, then this object cannot be unreffed, because that will cause the listener die as well. That's pretty unfortunate. I know the idea is to have the object alive as long as the associated frame is alive, and keep all the memory management fully up to the WebKit, but that doesn't work in Evolution, where we have one WebView for a whole lifetime of the application.

I know there is an option to destroy & create the WebView whenever a new message is selected or such, but it has UI impact, which I'd like to avoid (UI flickering is not good).

Thus, apart of fixing the DOMObjecCache, could you add also a function for webkit_web_view to cleanup all objects from the DOMObjectCache associated for that WebView (its frame)? If would be like if the WebView's frame was destroyed, except it will not. That will mean than any GObject-s created for that view will be invalidated. It'll help to keep the memory usage low without constant recreation of the widgets.

[Carlos Garcia Campos]

(In reply to comment #11)
> Few more observations found (by Tomas). When using
>    webkit_dom_event_target_add_event_listener()
> on a webkit GObject, then this object cannot be unreffed, because that will
> cause the listener die as well. That's pretty unfortunate.

The thing is that the event listener object doesn't exist in DOM bindings, it's an internal object that is created on demand, and destroyed when the target object is destroyed, or when the event listener is removed.

> I know the idea
> is to have the object alive as long as the associated frame is alive, and
> keep all the memory management fully up to the WebKit, but that doesn't work
> in Evolution, where we have one WebView for a whole lifetime of the
> application.
> 
> I know there is an option to destroy & create the WebView whenever a new
> message is selected or such, but it has UI impact, which I'd like to avoid
> (UI flickering is not good).

Yes, I agree that's not an option, it doesn't make sense to destroiy and create web views for this. I think the problem is that we have always made the DOM objects depend on its frame, when what we should probably be monitoring is the document instead of (or in addition to) the frame.

> Thus, apart of fixing the DOMObjecCache, could you add also a function for
> webkit_web_view to cleanup all objects from the DOMObjectCache associated
> for that WebView (its frame)? If would be like if the WebView's frame was
> destroyed, except it will not. That will mean than any GObject-s created for
> that view will be invalidated. It'll help to keep the memory usage low
> without constant recreation of the widgets.

We are not going to add new API to WebKit1, but I think it can be fixed internally without any new API, by clearing the objects when the document is detached from the frame or when the document is destroyed or the render tree is destroyed, I don't know exactly how to do it yet, but it could be something in that direction.

[Carlos Garcia Campos]

Created attachment 248428 [details]
Patch

I think this patch solves the problems Milan is having. It also clears the DOM objects when the document is detached from the frame.

[Carlos Garcia Campos]

Created attachment 248431 [details]
Updated patch

Made a mistake in the unit test, it should be fine now

[Milan Crha]

(In reply to comment #13)
> I think this patch solves the problems Milan is having. It also clears the
> DOM objects when the document is detached from the frame.

This is not for WebKit1, or the 2.4.8 misses some changeset, because there is no DOMObjectCacheFrameObserver. My problem is currently with WebKit1, thus I cannot test this change to know whether it'll really fix my issue or not.

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #13)
> > I think this patch solves the problems Milan is having. It also clears the
> > DOM objects when the document is detached from the frame.
> 
> This is not for WebKit1, or the 2.4.8 misses some changeset, because there
> is no DOMObjectCacheFrameObserver. My problem is currently with WebKit1,
> thus I cannot test this change to know whether it'll really fix my issue or
> not.

Yes, I know, I'll backport this and the other patches to 2.4 branch as soon as I find the time.

[Milan Crha]

(In reply to comment #16)
> Yes, I know, I'll backport this and the other patches to 2.4 branch as soon
> as I find the time.

Thanks. Once you have it done, and give me the pointers to relevant changes, I'll be able to test it in the "real life" of the Evolution use case.

[Carlos Garcia Campos]

I've realized the patch doesn't always work. It only works the first time the document is detached from the frame, but when a new document is created we no longer track its DOM Window. We need to use a different class for the DOM Window observer.

[Carlos Garcia Campos]

Created attachment 248646 [details]
New patch

Instead of making DOMObjectCacheFrameObserver inherit from DOMWindowProperty, use a helper class DOMWindowObserver used as a member of DOMObjectCacheFrameObserver and created/destroyed on demand to track the document when it's detached form the frame, but also when a new document is created. I've updated the unit test to run 3 times, because when run twice, the first time objects were correctly cleared when the document was detached from frame, but the second time it was the frame destruction what caused the objects to be cleared.

[Sergio Villar Senin]

Comment on attachment 248646 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248646&action=review

Looks good

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDOMNode.cpp:73
> +    }

I think it's worth also to try to load some external resource and reload it multiple times as the code paths are different that starting new loads.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebProcessTest.cpp:67
> +    // Test /WebKitDOMNode/dom-cache is an exception, because it's called twice, so

trice right?

[Carlos Garcia Campos]

(In reply to comment #20)
> Comment on attachment 248646 [details]
> New patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248646&action=review
> 
> Looks good

Thanks.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDOMNode.cpp:73
> > +    }
> 
> I think it's worth also to try to load some external resource and reload it
> multiple times as the code paths are different that starting new loads.

Not easy in this case, but even if the code path is not exactly the same, the global object is detached from frame on reload as well, so there's no difference.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebProcessTest.cpp:67
> > +    // Test /WebKitDOMNode/dom-cache is an exception, because it's called twice, so
> 
> trice right?

Right.

[Carlos Garcia Campos]

Committed r181631: <http://trac.webkit.org/changeset/181631>

[Carlos Garcia Campos]

Milan, I've just backported this to 2.4 branch, see http://trac.webkit.org/changeset/182479. With a slightly modified version of your test case (manually unreffing the transfer full objects) I get no leaks:

# dump_leaked_pointers: leaked 0 pointers

Please, try it out to confirm it works as expected for you in evolution.

[Carlos Garcia Campos]

Created attachment 250277 [details]
Fix test case

This is the patch required for wk-leaks.c, FWIW.

[Milan Crha]

Thanks. I've got webkitgtk3 2.4.8 as patched in Fedora [1] and applied on top of it your changeset. When I run cvurrent git master of evolution (at git commit 6668154), then it crashes with this backtrace: 

#0  0x00007ffff1018668 in g_type_check_instance_is_fundamentally_a (type_instance=0x5337f20, fundamental_type=80) at gtype.c:4029
#1  0x00007ffff0ffebc4 in g_object_unref (_object=0x5337f20) at gobject.c:3067
#2  0x00007ffff4ad3de8 in WebKit::DOMObjectCacheFrameObserver::DOMWindowObserver::willDetachGlobalObjectFromFrame() () from /build/local/lib/libwebkitgtk-3.0.so.0
#3  0x00007ffff42e1db2 in WebCore::DOMWindow::willDetachDocumentFromFrame() () from /build/local/lib/libwebkitgtk-3.0.so.0
#4  0x00007ffff3ef307e in WebCore::Document::prepareForDestruction() () from /build/local/lib/libwebkitgtk-3.0.so.0
#5  0x00007ffff43021cd in WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) () from /build/local/lib/libwebkitgtk-3.0.so.0
#6  0x00007ffff4304fdb in WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, WebCore::IntRect const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) ()
   from /build/local/lib/libwebkitgtk-3.0.so.0
#7  0x00007ffff3bccf81 in WebKit::FrameLoaderClient::transitionToCommittedForNewPage() () from /build/local/lib/libwebkitgtk-3.0.so.0
#8  0x00007ffff4261359 in WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) () from /build/local/lib/libwebkitgtk-3.0.so.0
#9  0x00007ffff4262ee7 in WebCore::FrameLoader::commitProvisionalLoad() () from /build/local/lib/libwebkitgtk-3.0.so.0
#10 0x00007ffff424a75a in WebCore::DocumentLoader::commitLoad(char const*, int) () from /build/local/lib/libwebkitgtk-3.0.so.0
#11 0x00007ffff424b810 in WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) () from /build/local/lib/libwebkitgtk-3.0.so.0
#12 0x00007ffff424ea07 in WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) () from /build/local/lib/libwebkitgtk-3.0.so.0
#13 0x00007ffff4247ed7 in WebCore::DocumentLoader::handleSubstituteDataLoadNow(WebCore::Timer<WebCore::DocumentLoader>*) () from /build/local/lib/libwebkitgtk-3.0.so.0
#14 0x00007ffff3cd8a09 in WebCore::ThreadTimers::sharedTimerFiredInternal() () from /build/local/lib/libwebkitgtk-3.0.so.0
#15 0x00007ffff3cf8682 in WebCore::sharedTimerTimeoutCallback(void*) () from /build/local/lib/libwebkitgtk-3.0.so.0
#16 0x00007ffff0cd5451 in g_timeout_dispatch (source=0x575bce0, callback=0x7ffff3cf8670 <WebCore::sharedTimerTimeoutCallback(void*)>, user_data=0x0) at gmain.c:4545
#17 0x00007ffff0cd3656 in g_main_dispatch (context=0x674460) at gmain.c:3122
#18 0x00007ffff0cd4491 in g_main_context_dispatch (context=0x674460) at gmain.c:3737
#19 0x00007ffff0cd4676 in g_main_context_iterate (context=0x674460, block=1, dispatch=1, self=0x6746d0) at gmain.c:3808
#20 0x00007ffff0cd4a9d in g_main_loop_run (loop=0x4b087f0) at gmain.c:4002
#21 0x00007ffff252cd4c in gtk_main () at gtkmain.c:1219
#22 0x0000000000404f14 in main (argc=1, argv=0x7fffffffdd88) at main.c:638

It also prints runtime warnings right after start at the very same place. We made some leak-hunting fixes on WebKit objects, as it used to be safe to unref (almost) any webkit-returned object and it took care of the DOMObjectCache properly, but it doesn't seem to be related to this crash, because when I involve valgrind, I get this:

==15478== Invalid read of size 4
==15478==    at 0x86C5DE8: WebKit::DOMObjectCacheFrameObserver::DOMWindowObserver::willDetachGlobalObjectFromFrame() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7ED3DB1: WebCore::DOMWindow::willDetachDocumentFromFrame() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7AE507D: WebCore::Document::prepareForDestruction() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7EF41CC: WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7EF6FDA: WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, WebCore::IntRect const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x77BEF80: WebKit::FrameLoaderClient::transitionToCommittedForNewPage() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E53358: WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E54EE6: WebCore::FrameLoader::commitProvisionalLoad() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E3C759: WebCore::DocumentLoader::commitLoad(char const*, int) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E3D80F: WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E40A06: WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E39ED6: WebCore::DocumentLoader::handleSubstituteDataLoadNow(WebCore::Timer<WebCore::DocumentLoader>*) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==  Address 0x150c63f8 is 8 bytes inside a block of size 16 free'd
==15478==    at 0x4A08103: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==15478==    by 0x86C4538: WebKit::DOMObjectCache::forget(void*) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x875CD24: webkit_dom_node_finalize(_GObject*) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0xB9EFE87: g_object_unref (gobject.c:3170)
==15478==    by 0x86C5DE7: WebKit::DOMObjectCacheFrameObserver::DOMWindowObserver::willDetachGlobalObjectFromFrame() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7ED3DB1: WebCore::DOMWindow::willDetachDocumentFromFrame() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7AE507D: WebCore::Document::prepareForDestruction() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7EF41CC: WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7EF6FDA: WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, WebCore::IntRect const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x77BEF80: WebKit::FrameLoaderClient::transitionToCommittedForNewPage() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E53358: WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)
==15478==    by 0x7E54EE6: WebCore::FrameLoader::commitProvisionalLoad() (in /build/local/lib/libwebkitgtk-3.0.so.0.22.14)

From that I guess I need more than just that single changeset to get "working webkit1" here. Is there a way I can get a pre-release tarball or anything like that, with all the applied fixes, please?

[1] http://pkgs.fedoraproject.org/cgit/webkitgtk3.git/tree/

[Milan Crha]

(In reply to comment #24)
> This is the patch required for wk-leaks.c, FWIW.

The patched wk-leaks.c works fine, it's only the evolution, which crashes and shows the critical warnings as stated in the previous comment.

[Michael Catanzaro]

(In reply to comment #25)
> Thanks. I've got webkitgtk3 2.4.8 as patched in Fedora [1] and applied on
> top of it your changeset. When I run cvurrent git master of evolution (at
> git commit 6668154), then it crashes with this backtrace: 

See bug #143521