Bug 144508 – [GTK] API tests crashing on debug builds due to extra unref

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Mario Sanchez Prada]

At the moment, the TestWebKitWebContext test suite is crashing on the GTK+ Debug but due to a problem caused by the URISchemeTest test suite:
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/4430/steps/API%20tests/logs/stdio

I reproduced the very same error locally, and this is the backtrace I got:

ASSERTION FAILED: m_table
../../Source/WTF/wtf/HashTable.h(210) : void WTF::HashTableConstIterator<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::checkValidity() const [with Key = WebCore::FrameDestructionObserver*; Value = WebCore::FrameDestructionObserver*; Extractor = WTF::IdentityExtractor; HashFunctions = WTF::PtrHash<WebCore::FrameDestructionObserver*>; Traits = WTF::HashTraits<WebCore::FrameDestructionObserver*>; KeyTraits = WTF::HashTraits<WebCore::FrameDestructionObserver*>]
1   0x7f64062f50b9 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f64062f50b9]
2   0x7f640c548dfb /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNK3WTF22HashTableConstIteratorIPN7WebCore24FrameDestructionObserverES3_NS_17IdentityExtractorENS_7PtrHashIS3_EENS_10HashTraitsIS3_EES8_E13checkValidityEv+0x3d) [0x7f640c548dfb]
3   0x7f640c54829c /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF22HashTableConstIteratorIPN7WebCore24FrameDestructionObserverES3_NS_17IdentityExtractorENS_7PtrHashIS3_EENS_10HashTraitsIS3_EES8_EppEv+0x18) [0x7f640c54829c]
4   0x7f640c546ea8 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF29HashTableConstIteratorAdapterINS_9HashTableIPN7WebCore24FrameDestructionObserverES4_NS_17IdentityExtractorENS_7PtrHashIS4_EENS_10HashTraitsIS4_EES9_EES4_EppEv+0x18) [0x7f640c546ea8]
5   0x7f640c5448e8 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore5Frame14willDetachPageEv+0xc4) [0x7f640c5448e8]
6   0x7f640c400392 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore11FrameLoader16detachFromParentEv+0x142) [0x7f640c400392]
7   0x7f640b88662e /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage5closeEv+0x3c2) [0x7f640b88662e]
8   0x7f640ba44994 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC22callMemberFunctionImplIN6WebKit7WebPageEMS2_FvvESt5tupleIJEEJEEEvPT_T0_OT1_St14index_sequenceIJXspT2_EEE+0x65) [0x7f640ba44994]
9   0x7f640ba42a7c /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18callMemberFunctionIN6WebKit7WebPageEMS2_FvvESt5tupleIIEESt19make_index_sequenceILm0EEEEvOT1_PT_T0_+0x41) [0x7f640ba42a7c]
10  0x7f640ba3ed5a /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC13handleMessageIN8Messages7WebPage5CloseEN6WebKit7WebPageEMS5_FvvEEEvRNS_14MessageDecoderEPT0_T1_+0x8f) [0x7f640ba3ed5a]
11  0x7f640ba39194 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage24didReceiveWebPageMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x1eba) [0x7f640ba39194]
12  0x7f640b89100f /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x185) [0x7f640b89100f]
13  0x7f640b5252f8 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_14MessageDecoderE+0x120) [0x7f640b5252f8]
14  0x7f640b750870 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit10WebProcess17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x4c) [0x7f640b750870]
15  0x7f640b5130f0 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageERNS_14MessageDecoderE+0x3a) [0x7f640b5130f0]
16  0x7f640b5131bc /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageESt10unique_ptrINS_14MessageDecoderESt14default_deleteIS2_EE+0xca) [0x7f640b5131bc]
17  0x7f640b51337f /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection18dispatchOneMessageEv+0xcd) [0x7f640b51337f]
18  0x7f640b512f36 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x4252f36) [0x7f640b512f36]
19  0x7f640b5144fe /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x42544fe) [0x7f640b5144fe]
20  0x7f640b52dac4 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNKSt8functionIFvvEEclEv+0x32) [0x7f640b52dac4]
21  0x7f640d4b6be9 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF7RunLoop11performWorkEv+0xdb) [0x7f640d4b6be9]
22  0x7f640d4ba712 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x61fa712) [0x7f640d4ba712]
23  0x7f640d4bac9a /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x61fac9a) [0x7f640d4bac9a]
24  0x7f640b52dac4 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNKSt8functionIFvvEEclEv+0x32) [0x7f640b52dac4]
25  0x7f640634106f /home/mario/work/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF15GMainLoopSource12voidCallbackEv+0x6d) [0x7f640634106f]
26  0x7f6406341773 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF15GMainLoopSource18voidSourceCallbackEPS0_+0x23) [0x7f6406341773]
27  0x7f6401ee458d /home/mario/work/WebKit/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x13d) [0x7f6401ee458d]
28  0x7f6401ee4928 /home/mario/work/WebKit/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(+0x48928) [0x7f6401ee4928]
29  0x7f6401ee4c42 /home/mario/work/WebKit/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7f6401ee4c42]
30  0x7f640d4ba48c /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF7RunLoop3runEv+0x42) [0x7f640d4ba48c]
31  0x7f640b9b4b06 /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit16ChildProcessMainINS_10WebProcessENS_14WebProcessMainEEEiiPPc+0x82) [0x7f640b9b4b06]

[Mario Sanchez Prada]

I debugged this extensively today, and found out that the problem seems to be that the hashtable holding the destruction observers for the Frame is being prematurely cleared due to a problem initiated when WebFrameLoaderClient::dispatchDidFinishDocumentLoad() is run, at the time it tries to notify the InjectedBundle:

    void WebFrameLoaderClient::dispatchDidFinishDocumentLoad()
    {
        [...]
    
        // Notify the bundle client.
        webPage->injectedBundleLoaderClient().didFinishDocumentLoadForFrame(webPage, m_frame, userData);
    
        [...]
    }
 
This bit will end up getting the following callback in Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp executed:

    static void documentLoadedCallback(WebKitWebPage* webPage, WebKitWebExtension* extension)
    {
        // FIXME: Too much code just to send a message, we need convenient custom API for this.
        WebKitDOMDocument* document = webkit_web_page_get_dom_document(webPage);
        GRefPtr<WebKitDOMDOMWindow> window = adoptGRef(webkit_dom_document_get_default_view(document));
        if (WebKitDOMWebKitNamespace* webkit = webkit_dom_dom_window_get_webkit_namespace(window.get())) {
            WebKitDOMUserMessageHandlersNamespace* messageHandlers = webkit_dom_webkit_namespace_get_message_handlers(webkit);
            if (WebKitDOMUserMessageHandler* handler = webkit_dom_user_message_handlers_namespace_get_handler(messageHandlers, "dom"))
                webkit_dom_user_message_handler_post_message(handler, "DocumentLoaded");
        }
    
        webkit_dom_dom_window_webkit_message_handlers_post_message(window.get(), "dom-convenience", "DocumentLoaded");
    
        gpointer data = g_object_get_data(G_OBJECT(extension), "dbus-connection");
        if (data)
            emitDocumentLoaded(G_DBUS_CONNECTION(data));
        else
            delayedSignalsQueue.append(DelayedSignal(DocumentLoadedSignal));
    }


...and this seems to be the problem: before r180214 [1], 'window' was just a raw pointer obtained as this:

    GRefPtr<WebKitDOMDOMWindow> window = adoptGRef(webkit_dom_document_get_default_view(document));


However, since r180214 it's now put in a GRefPtr<WebKitDOMDOMWindow>, meaning that its refcount will get decreased as soon as we live the scope of this function. And because we are using adoptGRef() here, that means that 'window' won't increase the refcount at all when getting the value from webkit_dom_document_get_default_view(), effectively being 1 at the time this callback finishes, causing the destruction of this 'window' object.

And this would be probably ok if 'window' was not cached inside the DOMCache, but the truth is that it is cached (automatically on construction the first time time it's requested via webkit_dom_document_get_default_view()). So, we can't simply steal the reference from the cache and unref it freely afterwards or we will get the crash we are seeing.

So, I believe we either drop the adoptGRef() usage and prevent the refcount of 'window' from getting to zero or, even simpler, we stop using GRefPtr<> and use a raw pointer as it used to be the case before r180214, which seems also to be the case everywhere else but here (which looks like a mistake):

    $ git grep webkit_dom_document_get_default_view
    Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow* domWindow = webkit_dom_document_get_default_view(document);
    Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow* domWindow = webkit_dom_document_get_default_view(document);
    Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow* domWindow = webkit_dom_document_get_default_view(document);
    Tests/WebKit2Gtk/WebExtensionTest.cpp:    GRefPtr<WebKitDOMDOMWindow> window = adoptGRef(webkit_dom_document_get_default_view(document));

Doing either way will get the crash gone.

[1] http://trac.webkit.org/changeset/180214

[Mario Sanchez Prada]

I just realized, both by checking the log in the buildbot and running locally the tests myself, that this crash is not exclusive to TestWebKitWebContext, but to all the following test suites:

  * TestWebKitWebContext
  * TestWebKitFaviconDatabase
  * TestWebKitUserContentManager
  * TestLoaderClient
  * TestResources

Renaming the bug accordingly...

[Mario Sanchez Prada]

Created attachment 252196 [details]
Patch proposal

The following patch moves back to using a raw pointer here, which I think it's ok because I can't see anyway the point of stealing the only reference for an object that is supposed to be cached :). Please review, thanks!

[Michael Catanzaro]

Thanks for debugging this!

I'm concerned about this patch because webkit_dom_document_get_default_view() is documented to be transfer full, so it really does need to be unreffed. I would expect the cache to ref it before returning it.

[Michael Catanzaro]

*** Bug 141736 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

(In reply to comment #4)
> Thanks for debugging this!
> 
> I'm concerned about this patch because
> webkit_dom_document_get_default_view() is documented to be transfer full, so
> it really does need to be unreffed. I would expect the cache to ref it
> before returning it.

Actually, I'm concerned about the very same thing, but it's late here and forgot to mention it (I did mention it on IRC to Martin, though) before attaching the patch, which I wanted to do anyway because I'm unsure I will be able to devote much time to WK next week.

Anyway, it might even be a bug in the documentation because everywhere I see this function used it's actually treating it like transfer-none, but of course I'm not 100% sure about it either. What I can say, though, is that I've checked a similar function that is documented to be transfer none (webkit_dom_document_get_document_element()) and to me they do look very similar:

    WebKitDOMElement* webkit_dom_document_get_document_element(WebKitDOMDocument* self)
    {
        WebCore::JSMainThreadNullState state;
        g_return_val_if_fail(WEBKIT_DOM_IS_DOCUMENT(self), 0);
        WebCore::Document* item = WebKit::core(self);
        RefPtr<WebCore::Element> gobjectResult = WTF::getPtr(item->documentElement());
        return WebKit::kit(gobjectResult.get());
    }

    WebKitDOMDOMWindow* webkit_dom_document_get_default_view(WebKitDOMDocument* self)
    {
        WebCore::JSMainThreadNullState state;
        g_return_val_if_fail(WEBKIT_DOM_IS_DOCUMENT(self), 0);
        WebCore::Document* item = WebKit::core(self);
        RefPtr<WebCore::DOMWindow> gobjectResult = WTF::getPtr(item->defaultView());
        return WebKit::kit(gobjectResult.get());
    }


And if you check WebKitDOMDOMWindow.cpp, you'll see that indeed no extra reference is being added to the value returned, which is simply returned from the cache (if there) or created for the first time and returned as is (will be added to the cache from the GObject constructor), in a similar fashion to what is done in, for instance, WebKitDOMDOMSelection.

So, all in all, I think you raised a very good point, because either there's a bug in the doc that needs to be fixed, or this patch I'm proposing would be wrong and there's some extra ref that needs to be added somewhere, probably in WebKit::kit() inside WebKitDOMDOMWindow.cpp, but I'd rather wait until next week, so that we give others a chance to comment too.

[Carlos Garcia Campos]

(In reply to comment #1)
> I debugged this extensively today, and found out that the problem seems to
> be that the hashtable holding the destruction observers for the Frame is
> being prematurely cleared due to a problem initiated when
> WebFrameLoaderClient::dispatchDidFinishDocumentLoad() is run, at the time it
> tries to notify the InjectedBundle:
> 
>     void WebFrameLoaderClient::dispatchDidFinishDocumentLoad()
>     {
>         [...]
>     
>         // Notify the bundle client.
>        
> webPage->injectedBundleLoaderClient().didFinishDocumentLoadForFrame(webPage,
> m_frame, userData);
>     
>         [...]
>     }
>  
> This bit will end up getting the following callback in
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp executed:
> 
>     static void documentLoadedCallback(WebKitWebPage* webPage,
> WebKitWebExtension* extension)
>     {
>         // FIXME: Too much code just to send a message, we need convenient
> custom API for this.
>         WebKitDOMDocument* document =
> webkit_web_page_get_dom_document(webPage);
>         GRefPtr<WebKitDOMDOMWindow> window =
> adoptGRef(webkit_dom_document_get_default_view(document));
>         if (WebKitDOMWebKitNamespace* webkit =
> webkit_dom_dom_window_get_webkit_namespace(window.get())) {
>             WebKitDOMUserMessageHandlersNamespace* messageHandlers =
> webkit_dom_webkit_namespace_get_message_handlers(webkit);
>             if (WebKitDOMUserMessageHandler* handler =
> webkit_dom_user_message_handlers_namespace_get_handler(messageHandlers,
> "dom"))
>                 webkit_dom_user_message_handler_post_message(handler,
> "DocumentLoaded");
>         }
>     
>        
> webkit_dom_dom_window_webkit_message_handlers_post_message(window.get(),
> "dom-convenience", "DocumentLoaded");
>     
>         gpointer data = g_object_get_data(G_OBJECT(extension),
> "dbus-connection");
>         if (data)
>             emitDocumentLoaded(G_DBUS_CONNECTION(data));
>         else
>             delayedSignalsQueue.append(DelayedSignal(DocumentLoadedSignal));
>     }
> 
> 
> ...and this seems to be the problem: before r180214 [1], 'window' was just a
> raw pointer obtained as this:
> 
>     GRefPtr<WebKitDOMDOMWindow> window =
> adoptGRef(webkit_dom_document_get_default_view(document));
> 
> 
> However, since r180214 it's now put in a GRefPtr<WebKitDOMDOMWindow>,
> meaning that its refcount will get decreased as soon as we live the scope of
> this function. And because we are using adoptGRef() here, that means that
> 'window' won't increase the refcount at all when getting the value from
> webkit_dom_document_get_default_view(), effectively being 1 at the time this
> callback finishes, causing the destruction of this 'window' object.

This is the expected behaviour, the dom wrapper is destroyed, but that shouldn't affect the core object, since all wrapped objects are refed/unrefed.

> And this would be probably ok if 'window' was not cached inside the
> DOMCache, but the truth is that it is cached (automatically on construction
> the first time time it's requested via
> webkit_dom_document_get_default_view()). So, we can't simply steal the
> reference from the cache and unref it freely afterwards or we will get the
> crash we are seeing.

We should be able. The problem might be that the core object is destroyed as well, and that shouldn't happen.

> So, I believe we either drop the adoptGRef() usage and prevent the refcount
> of 'window' from getting to zero or, even simpler, we stop using GRefPtr<>
> and use a raw pointer as it used to be the case before r180214, which seems
> also to be the case everywhere else but here (which looks like a mistake):
> 
>     $ git grep webkit_dom_document_get_default_view
>     Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow*
> domWindow = webkit_dom_document_get_default_view(document);
>     Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow*
> domWindow = webkit_dom_document_get_default_view(document);
>     Tests/WebKit2Gtk/DOMDOMWindowTest.cpp:        WebKitDOMDOMWindow*
> domWindow = webkit_dom_document_get_default_view(document);
>     Tests/WebKit2Gtk/WebExtensionTest.cpp:    GRefPtr<WebKitDOMDOMWindow>
> window = adoptGRef(webkit_dom_document_get_default_view(document));

DOMDOMWindowTest.cpp doesn't actually exist, I think it's a wip from quique that I ended up pushing by mistake at some point. 

> Doing either way will get the crash gone.

I think that simply hides the actual problem by leaking the WebKitDOMDOMWindow object. Thanks a lot for the analysis, it'll definitely help to catch the real problem. I'm going to look at it.

> [1] http://trac.webkit.org/changeset/180214

[Carlos Garcia Campos]

Comment on attachment 252196 [details]
Patch proposal

This is indeed leaking the DOMWindow and hiding the actual bug

[Carlos Garcia Campos]

Created attachment 252230 [details]
Patch

This fixes the crashes for me.

[Martin Robinson]

Comment on attachment 252230 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        created, the DOM object cache was notified about the previous
> +        DOMWindow being destroyed before objects for the new DOMWindow are
> +        added to the cache. However, that's not always the case and we
> +        only create a DOMWindowObserver for the first DOMWindow. We need
> +        to keep a pointer to the DOMWindow being observed to clear() the

Does this mean the cache is leaking in some cases?

[Carlos Garcia Campos]

(In reply to comment #10)
> Comment on attachment 252230 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252230&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        created, the DOM object cache was notified about the previous
> > +        DOMWindow being destroyed before objects for the new DOMWindow are
> > +        added to the cache. However, that's not always the case and we
> > +        only create a DOMWindowObserver for the first DOMWindow. We need
> > +        to keep a pointer to the DOMWindow being observed to clear() the
> 
> Does this mean the cache is leaking in some cases?

Not exactly, but objects are released a bit later, when the first DOMwindow object is destroyed, or when the page is detached.

[Mario Sanchez Prada]

Comment on attachment 252230 [details]
Patch

I'm so happy you found the actual bug and wrote the right patch! Now I might even find time to finish my work on bug 144262, which is what I actually intented to work on when I got sucked by this other problem :).

Patch looks great to me, btw. Thanks,

[Carlos Garcia Campos]

Committed r183729: <http://trac.webkit.org/changeset/183729>