Changeset 249198 in webkit

Timestamp:

Aug 28, 2019, 8:15:51 AM (7 years ago)

Author:

rniwa@webkit.org

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/ElementRareData.h (modified) (1 diff)
• dom/Node.cpp (modified) (1 diff)
• dom/Node.h (modified) (2 diffs)
• dom/NodeRareData.cpp (modified) (1 diff)
• dom/NodeRareData.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/ElementRareData.h

@@ -191 +191 @@
 
 inline ElementRareData::ElementRareData()
-    : m_tabIndex(0)
+    : NodeRareData(Type::Element)
+    , m_tabIndex(0)
     , m_childIndex(0)
     , m_tabIndexWasSetExplicitly(false)

trunk/Source/WebCore/dom/Node.cpp

@@ -396 +396 @@
 {
     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);
 }
 

trunk/Source/WebCore/dom/Node.h

@@ -665 +665 @@
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
 
+    struct NodeRareDataDeleter {
+        void operator()(NodeRareData*) const;
+    };
+
     uint32_t m_refCountAndParentBit { s_refCountIncrement };
     mutable uint32_t m_nodeFlags;
@@ -673 +677 @@
     Node* m_next { nullptr };
     CompactPointerTuple<RenderObject*, uint8_t> m_rendererWithStyleFlags;
-    std::unique_ptr<NodeRareData> m_rareData;
+    std::unique_ptr<NodeRareData, NodeRareDataDeleter> m_rareData;
 };
 

trunk/Source/WebCore/dom/NodeRareData.cpp

@@ -37 +37 @@
 
 struct SameSizeAsNodeRareData {
-    unsigned m_frameCount;
-    void* m_pointer[3];
+    unsigned m_frameCountAndIsElementRareDataFlag;
+    void* m_pointer[2];
 };
 

trunk/Source/WebCore/dom/NodeRareData.h

@@ -271 +271 @@
 #endif
 
-    NodeRareData()
-    { }
-
-    virtual ~NodeRareData()
-    { }
+    enum class Type { Element, Node };
+
+    NodeRareData(Type type = Type::Node)
+        : m_connectedFrameCount(0)
+        , m_isElementRareData(type == Type::Element)
+    {
+    }
+
+    bool isElementRareData() { return m_isElementRareData; }
 
     void clearNodeLists() { m_nodeLists = nullptr; }
@@ -321 +325 @@
 
 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;