REGRESSION (r248807): Objects stored in ElementRareData are leaked
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Aug 2019 15:15:51 +0000 (15:15 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Aug 2019 15:15:51 +0000 (15:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200954

Reviewed by Antti Koivisto.

Use a custom deleter in std::unique_ptr to call the correct destructor instead of making
NodeRareData's destructor virtual. Added NodeRareData::isElementRareData to differentiate
ElementRareData and NodeRareData by borrowing 1 bit from the frame count.

No new tests since there should be no behavioral change.

* dom/ElementRareData.h:
(WebCore::ElementRareData::ElementRareData):
* dom/Node.cpp:
(WebCore::Node::materializeRareData): Call the constructors of unique_ptr directly since
make_unique does not take a custom deleter. We can't add the support to makeUnique either
without making it three arguments since we need to cast ElementRareData to NodeRareData
in addition to specifying a custom deleter (normal casting wouldn't work due to
the presence of a custom deleter).
(WebCore::Node::NodeRareDataDeleter::operator() const): Added.
* dom/Node.h:
(WebCore::Node::NodeRareDataDeleter): Added.
* dom/NodeRareData.cpp:
* dom/NodeRareData.h:
(WebCore::NodeRareData::NodeRareData): Makes newly added Type.
(WebCore::NodeRareData::isElementRareData): Added.
(WebCore::NodeRareData::~NodeRareData): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249198 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/ElementRareData.h
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h
Source/WebCore/dom/NodeRareData.cpp
Source/WebCore/dom/NodeRareData.h

index 02284f4..1ad90e2 100644 (file)
@@ -1,3 +1,33 @@
+2019-08-28  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r248807): Objects stored in ElementRareData are leaked
+        https://bugs.webkit.org/show_bug.cgi?id=200954
+
+        Reviewed by Antti Koivisto.
+
+        Use a custom deleter in std::unique_ptr to call the correct destructor instead of making
+        NodeRareData's destructor virtual. Added NodeRareData::isElementRareData to differentiate
+        ElementRareData and NodeRareData by borrowing 1 bit from the frame count.
+
+        No new tests since there should be no behavioral change.
+
+        * dom/ElementRareData.h:
+        (WebCore::ElementRareData::ElementRareData):
+        * dom/Node.cpp:
+        (WebCore::Node::materializeRareData): Call the constructors of unique_ptr directly since
+        make_unique does not take a custom deleter. We can't add the support to makeUnique either
+        without making it three arguments since we need to cast ElementRareData to NodeRareData
+        in addition to specifying a custom deleter (normal casting wouldn't work due to
+        the presence of a custom deleter).
+        (WebCore::Node::NodeRareDataDeleter::operator() const): Added.
+        * dom/Node.h:
+        (WebCore::Node::NodeRareDataDeleter): Added.
+        * dom/NodeRareData.cpp:
+        * dom/NodeRareData.h:
+        (WebCore::NodeRareData::NodeRareData): Makes newly added Type.
+        (WebCore::NodeRareData::isElementRareData): Added.
+        (WebCore::NodeRareData::~NodeRareData): Deleted.
+
 2019-08-28  Claudio Saavedra  <csaavedra@igalia.com>
 
         [SOUP] Shut compilation warning
index ac8e3b2..ce8b48e 100644 (file)
@@ -190,7 +190,8 @@ private:
 };
 
 inline ElementRareData::ElementRareData()
-    : m_tabIndex(0)
+    : NodeRareData(Type::Element)
+    , m_tabIndex(0)
     , m_childIndex(0)
     , m_tabIndexWasSetExplicitly(false)
 #if ENABLE(FULLSCREEN_API)
index 0accf2e..9801b17 100644 (file)
@@ -395,9 +395,17 @@ void Node::willBeDeletedFrom(Document& document)
 void Node::materializeRareData()
 {
     if (is<Element>(*this))
-        m_rareData = makeUnique<ElementRareData>();
+        m_rareData = std::unique_ptr<NodeRareData, NodeRareDataDeleter>(new ElementRareData);
     else
-        m_rareData = makeUnique<NodeRareData>();
+        m_rareData = std::unique_ptr<NodeRareData, NodeRareDataDeleter>(new NodeRareData);
+}
+
+inline void Node::NodeRareDataDeleter::operator()(NodeRareData* rareData) const
+{
+    if (rareData->isElementRareData())
+        delete static_cast<ElementRareData*>(rareData);
+    else
+        delete static_cast<NodeRareData*>(rareData);
 }
 
 void Node::clearRareData()
index 8d64854..8cc214a 100644 (file)
@@ -664,6 +664,10 @@ private:
     static void moveTreeToNewScope(Node&, TreeScope& oldScope, TreeScope& newScope);
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
 
+    struct NodeRareDataDeleter {
+        void operator()(NodeRareData*) const;
+    };
+
     uint32_t m_refCountAndParentBit { s_refCountIncrement };
     mutable uint32_t m_nodeFlags;
 
@@ -672,7 +676,7 @@ private:
     Node* m_previous { nullptr };
     Node* m_next { nullptr };
     CompactPointerTuple<RenderObject*, uint8_t> m_rendererWithStyleFlags;
-    std::unique_ptr<NodeRareData> m_rareData;
+    std::unique_ptr<NodeRareData, NodeRareDataDeleter> m_rareData;
 };
 
 #ifndef NDEBUG
index 918a665..ea906d6 100644 (file)
@@ -36,8 +36,8 @@
 namespace WebCore {
 
 struct SameSizeAsNodeRareData {
-    unsigned m_frameCount;
-    void* m_pointer[3];
+    unsigned m_frameCountAndIsElementRareDataFlag;
+    void* m_pointer[2];
 };
 
 COMPILE_ASSERT(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData), NodeRareDataShouldStaySmall);
index f3132e6..bff975d 100644 (file)
@@ -270,11 +270,15 @@ public:
     };
 #endif
 
-    NodeRareData()
-    { }
+    enum class Type { Element, Node };
 
-    virtual ~NodeRareData()
-    { }
+    NodeRareData(Type type = Type::Node)
+        : m_connectedFrameCount(0)
+        , m_isElementRareData(type == Type::Element)
+    {
+    }
+
+    bool isElementRareData() { return m_isElementRareData; }
 
     void clearNodeLists() { m_nodeLists = nullptr; }
     NodeListsNodeData* nodeLists() const { return m_nodeLists.get(); }
@@ -320,7 +324,8 @@ public:
 #endif
 
 private:
-    unsigned m_connectedFrameCount { 0 }; // Must fit Page::maxNumberOfFrames.
+    unsigned m_connectedFrameCount : 31; // Must fit Page::maxNumberOfFrames.
+    unsigned m_isElementRareData : 1;
 
     std::unique_ptr<NodeListsNodeData> m_nodeLists;
     std::unique_ptr<NodeMutationObserverData> m_mutationObserverData;