Bug 239353 – [GTK] Warning in WebKitDOMDocumentGtk.cpp with GCC 12

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

I think GCC maybe found a real bug here:

In file included from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-16.cpp:5:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp: In function ‘WebKitDOMNodeIterator* webkit_dom_document_create_node_iterator(WebKitDOMDocument*, WebKitDOMNode*, gulong, WebKitDOMNodeFilter*, gboolean, GError**)’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1107:23: warning: pointer may be used after ‘static void WebCore::NodeIterator::operator delete(void*)’ [-Wuse-after-free]
 1107 |     return WebKit::kit(gobjectResult.get());
      |            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/12/memory:76,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/StdLibExtras.h:30,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/FastMalloc.h:26,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebKit/config.h:42,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDOMTokenList.cpp:20,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-16.cpp:1:
In member function ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = WebCore::NodeIterator]’,
    inlined from ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = WebCore::NodeIterator]’ at /usr/include/c++/12/bits/unique_ptr.h:79:7,
    inlined from ‘void WTF::RefCounted<T, Deleter>::deref() const [with T = WebCore::NodeIterator; Deleter = std::default_delete<WebCore::NodeIterator>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounted.h:190:22,
    inlined from ‘WTF::Ref<T, <template-parameter-1-2> >::~Ref() [with T = WebCore::NodeIterator; Traits = WTF::RawPtrTraits<WebCore::NodeIterator>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/Ref.h:61:23,
    inlined from ‘WebKitDOMNodeIterator* webkit_dom_document_create_node_iterator(WebKitDOMDocument*, WebKitDOMNode*, gulong, WebKitDOMNodeFilter*, gboolean, GError**)’ at /home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1106:87:
/usr/include/c++/12/bits/unique_ptr.h:85:9: note: call to ‘static void WebCore::NodeIterator::operator delete(void*)’ here
   85 |         delete __ptr;
      |         ^~~~~~~~~~~~
/home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp: In function ‘WebKitDOMTreeWalker* webkit_dom_document_create_tree_walker(WebKitDOMDocument*, WebKitDOMNode*, gulong, WebKitDOMNodeFilter*, gboolean, GError**)’:
/home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1121:23: warning: pointer may be used after ‘static void WebCore::TreeWalker::operator delete(void*)’ [-Wuse-after-free]
 1121 |     return WebKit::kit(gobjectResult.get());
      |            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
In member function ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = WebCore::TreeWalker]’,
    inlined from ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = WebCore::TreeWalker]’ at /usr/include/c++/12/bits/unique_ptr.h:79:7,
    inlined from ‘void WTF::RefCounted<T, Deleter>::deref() const [with T = WebCore::TreeWalker; Deleter = std::default_delete<WebCore::TreeWalker>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounted.h:190:22,
    inlined from ‘WTF::Ref<T, <template-parameter-1-2> >::~Ref() [with T = WebCore::TreeWalker; Traits = WTF::RawPtrTraits<WebCore::TreeWalker>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/Ref.h:61:23,
    inlined from ‘WebKitDOMTreeWalker* webkit_dom_document_create_tree_walker(WebKitDOMDocument*, WebKitDOMNode*, gulong, WebKitDOMNodeFilter*, gboolean, GError**)’ at /home/mcatanzaro/Projects/WebKit/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1120:83:
/usr/include/c++/12/bits/unique_ptr.h:85:9: note: call to ‘static void WebCore::TreeWalker::operator delete(void*)’ here
   85 |         delete __ptr;
      |         ^~~~~~~~~~~~

Unless I'm mistaken, the problem is here:

    RefPtr<WebCore::TreeWalker> gobjectResult = WTF::getPtr(item->createTreeWalker(*convertedRoot, whatToShow, WTF::getPtr(convertedFilter), expandEntityReferences));
    return WebKit::kit(gobjectResult.get());

Behind so many confusing layers like WTF::getPtr and WebKit::kit, it's a little unclear what's happening with the refcount, but it sure looks like have only one ref and return a dangling pointer. Using leakRef() makes the warnings go away:

diff --git a/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp b/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp
index a9cdac9c0911..95f6e5932abe 100644
--- a/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp
+++ b/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp
@@ -1104,7 +1104,7 @@ WebKitDOMNodeIterator* webkit_dom_document_create_node_iterator(WebKitDOMDocumen
     WebCore::Node* convertedRoot = WebKit::core(root);
     RefPtr<WebCore::NodeFilter> convertedFilter = WebKit::core(item, filter);
     RefPtr<WebCore::NodeIterator> gobjectResult = WTF::getPtr(item->createNodeIterator(*convertedRoot, whatToShow, WTF::getPtr(convertedFilter), expandEntityReferences));
-    return WebKit::kit(gobjectResult.get());
+    return WebKit::kit(gobjectResult.leakRef());
 }
 
 WebKitDOMTreeWalker* webkit_dom_document_create_tree_walker(WebKitDOMDocument* self, WebKitDOMNode* root, gulong whatToShow, WebKitDOMNodeFilter* filter, gboolean expandEntityReferences, GError** error)
@@ -1118,7 +1118,7 @@ WebKitDOMTreeWalker* webkit_dom_document_create_tree_walker(WebKitDOMDocument* s
     WebCore::Node* convertedRoot = WebKit::core(root);
     RefPtr<WebCore::NodeFilter> convertedFilter = WebKit::core(item, filter);
     RefPtr<WebCore::TreeWalker> gobjectResult = WTF::getPtr(item->createTreeWalker(*convertedRoot, whatToShow, WTF::getPtr(convertedFilter), expandEntityReferences));
-    return WebKit::kit(gobjectResult.get());
+    return WebKit::kit(gobjectResult.leakRef());
 }
 
 WebKitDOMCSSStyleDeclaration* webkit_dom_document_get_override_style(WebKitDOMDocument*, WebKitDOMElement*, const gchar*)


Problem is, this was previously generated code, so it's a pattern that is very common throughout these bindings. I wonder why GCC is only flagging these two particular places....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Behind so many confusing layers like WTF::getPtr and WebKit::kit, it's a
> little unclear what's happening with the refcount, but it sure looks like
> have only one ref and return a dangling pointer.

What we're returning is actually a GObject from the DOMObjectCache, not the WebCore object. So the general pattern looks fine.

[Michael Catanzaro]

I don't see anything wrong with the code. Maybe there could be a bug in DOMObjectCache, but if so, we're not going to find it from this useless warning. I'm just going to suppress it and move on. Spent too much effort already squinting at these deprecated functions.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/621

[EWS]

Committed r294232 (250590@main): <https://commits.webkit.org/250590@main>

Reviewed commits have been landed. Closing PR #621 and removing active labels.