Bug 239353 - [GTK] Warning in WebKitDOMDocumentGtk.cpp with GCC 12
Summary: [GTK] Warning in WebKitDOMDocumentGtk.cpp with GCC 12
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-14 14:02 PDT by Michael Catanzaro
Modified: 2022-05-16 06:56 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-04-14 14:02:08 PDT
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....
Comment 1 Michael Catanzaro 2022-05-13 14:39:38 PDT
(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.
Comment 2 Michael Catanzaro 2022-05-13 15:01:13 PDT
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.
Comment 3 Michael Catanzaro 2022-05-13 15:10:34 PDT
Pull request: https://github.com/WebKit/WebKit/pull/621
Comment 4 EWS 2022-05-16 06:56:18 PDT
Committed r294232 (250590@main): <https://commits.webkit.org/250590@main>

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