Stop using a raw pointer for Node::m_treeScope

https://bugs.webkit.org/show_bug.cgi?id=262239 rdar://116501041 Reviewed by Ryosuke Niwa. * Source/WTF/wtf/Ref.h: (WTF::checkedDowncast): * Source/WTF/wtf/RefPtr.h: (WTF::checkedDowncast): * Source/WTF/wtf/TypeCasts.h: (WTF::checkedDowncast): Introduce checkedDowncast<>(), which is similar to downcast<>() but the type check happens in release builds too for extra safety. * Source/WebCore/dom/Node.cpp: (WebCore::Node::Node): (WebCore::Node::~Node): * Source/WebCore/dom/Node.h: * Source/WebCore/dom/ShadowRoot.h: * Source/WebCore/dom/TreeScope.cpp: (WebCore::TreeScope::incrementPtrCount const): (WebCore::TreeScope::decrementPtrCount const): * Source/WebCore/dom/TreeScope.h: Canonical link: https://commits.webkit.org/269692@main
WebKit · Oct 24, 2023 · 2714943 · 2714943
1 parent c57ce1f
commit 2714943
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 3 deletions.
diff --git a/Source/WTF/wtf/Ref.h b/Source/WTF/wtf/Ref.h
@@ -291,6 +291,15 @@ inline bool is(const Ref<ArgType, PtrTraits>& source)
     return is<ExpectedType>(source.get());
 }
 
+template<typename Target, typename Source, typename PtrTraits>
+inline Ref<Target> checkedDowncast(Ref<Source, PtrTraits> source)
+{
+    static_assert(!std::is_same_v<Source, Target>, "Unnecessary cast to same type");
+    static_assert(std::is_base_of_v<Source, Target>, "Should be a downcast");
+    RELEASE_ASSERT(is<Target>(source));
+    return static_reference_cast<Target>(WTFMove(source));
+}
+
 template<typename Target, typename Source, typename PtrTraits>
 inline Ref<Target> downcast(Ref<Source, PtrTraits> source)
 {

diff --git a/Source/WTF/wtf/RefPtr.h b/Source/WTF/wtf/RefPtr.h
@@ -258,6 +258,15 @@ inline bool is(const RefPtr<ArgType, PtrTraits, RefDerefTraits>& source)
     return is<ExpectedType>(source.get());
 }
 
+template<typename Target, typename Source, typename PtrTraits, typename RefDerefTraits>
+inline RefPtr<Target> checkedDowncast(RefPtr<Source, PtrTraits, RefDerefTraits> source)
+{
+    static_assert(!std::is_same_v<Source, Target>, "Unnecessary cast to same type");
+    static_assert(std::is_base_of_v<Source, Target>, "Should be a downcast");
+    RELEASE_ASSERT(!source || is<Target>(*source));
+    return static_pointer_cast<Target>(WTFMove(source));
+}
+
 template<typename Target, typename Source, typename PtrTraits, typename RefDerefTraits>
 inline RefPtr<Target> downcast(RefPtr<Source, PtrTraits, RefDerefTraits> source)
 {

diff --git a/Source/WTF/wtf/TypeCasts.h b/Source/WTF/wtf/TypeCasts.h
@@ -71,6 +71,23 @@ using match_constness_t =
     typename std::conditional_t<std::is_const_v<Reference>, typename std::add_const_t<T>, typename std::remove_const_t<T>>;
 
 // Safe downcasting functions.
+template<typename Target, typename Source>
+inline match_constness_t<Source, Target>& checkedDowncast(Source& source)
+{
+    static_assert(!std::is_same_v<Source, Target>, "Unnecessary cast to same type");
+    static_assert(std::is_base_of_v<Source, Target>, "Should be a downcast");
+    RELEASE_ASSERT(is<Target>(source));
+    return static_cast<match_constness_t<Source, Target>&>(source);
+}
+template<typename Target, typename Source>
+inline match_constness_t<Source, Target>* checkedDowncast(Source* source)
+{
+    static_assert(!std::is_same_v<Source, Target>, "Unnecessary cast to same type");
+    static_assert(std::is_base_of_v<Source, Target>, "Should be a downcast");
+    RELEASE_ASSERT(!source || is<Target>(*source));
+    return static_cast<match_constness_t<Source, Target>*>(source);
+}
+
 template<typename Target, typename Source>
 inline match_constness_t<Source, Target>& downcast(Source& source)
 {
@@ -134,5 +151,6 @@ inline bool is(const std::unique_ptr<ArgType, Deleter>& source)
 
 using WTF::TypeCastTraits;
 using WTF::is;
+using WTF::checkedDowncast;
 using WTF::downcast;
 using WTF::dynamicDowncast;
diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -394,7 +394,7 @@ inline void NodeRareData::operator delete(NodeRareData* nodeRareData, std::destr
 Node::Node(Document& document, ConstructionType type)
     : EventTarget(ConstructNode)
     , m_nodeFlags(type)
-    , m_treeScope(&document)
+    , m_treeScope((isDocumentNode() || isShadowRoot()) ? nullptr : &document)
 {
     ASSERT(isMainThread());
 
@@ -432,7 +432,16 @@ Node::~Node()
     ASSERT(!m_previous);
     ASSERT(!m_next);
 
-    document().decrementReferencingNodeCount();
+    {
+        // Not refing document because it may be in the middle of destruction.
+        auto& document = this->document(); // Store document before clearing out m_treeScope.
+
+        // The call to decrementReferencingNodeCount() below may destroy the document so we need to clear our
+        // m_treeScope CheckedPtr beforehand.
+        m_treeScope = nullptr;
+
+        document.decrementReferencingNodeCount(); // This may destroy the document.
+    }
 
 #if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY) && (ASSERT_ENABLED || ENABLE(SECURITY_ASSERTIONS))
     for (auto& document : Document::allDocuments()) {

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
@@ -762,7 +762,7 @@ class Node : public EventTarget {
     mutable OptionSet<NodeFlag> m_nodeFlags;
 
     CheckedPtr<ContainerNode> m_parentNode;
-    TreeScope* m_treeScope { nullptr };
+    CheckedPtr<TreeScope> m_treeScope;
     CheckedPtr<Node> m_previous;
     CheckedPtr<Node> m_next;
     CompactPointerTuple<RenderObject*, uint16_t> m_rendererWithStyleFlags;

diff --git a/Source/WebCore/dom/ShadowRoot.h b/Source/WebCore/dom/ShadowRoot.h
@@ -65,6 +65,10 @@ class ShadowRoot final : public DocumentFragment, public TreeScope {
 
     virtual ~ShadowRoot();
 
+    // Resolve ambiguity for CanMakeCheckedPtr.
+    void incrementPtrCount() const { static_cast<const DocumentFragment*>(this)->incrementPtrCount(); }
+    void decrementPtrCount() const { static_cast<const DocumentFragment*>(this)->decrementPtrCount(); }
+
     using TreeScope::getElementById;
     using TreeScope::rootNode;
 

diff --git a/Source/WebCore/dom/TreeScope.cpp b/Source/WebCore/dom/TreeScope.cpp
@@ -89,6 +89,22 @@ TreeScope::TreeScope(Document& document)
 
 TreeScope::~TreeScope() = default;
 
+void TreeScope::incrementPtrCount() const
+{
+    if (auto* document = dynamicDowncast<Document>(m_rootNode))
+        document->incrementPtrCount();
+    else
+        checkedDowncast<ShadowRoot>(m_rootNode).incrementPtrCount();
+}
+
+void TreeScope::decrementPtrCount() const
+{
+    if (auto* document = dynamicDowncast<Document>(m_rootNode))
+        document->decrementPtrCount();
+    else
+        checkedDowncast<ShadowRoot>(m_rootNode).decrementPtrCount();
+}
+
 void TreeScope::destroyTreeScopeData()
 {
     m_elementsById = nullptr;

diff --git a/Source/WebCore/dom/TreeScope.h b/Source/WebCore/dom/TreeScope.h
@@ -68,6 +68,10 @@ class TreeScope {
     TreeScope* parentTreeScope() const { return m_parentTreeScope; }
     void setParentTreeScope(TreeScope&);
 
+    // For CheckedPtr / CheckedRef use.
+    void incrementPtrCount() const;
+    void decrementPtrCount() const;
+
     Element* focusedElementInScope();
     Element* pointerLockElement() const;