Crash in Style::commitRelations

https://bugs.webkit.org/show_bug.cgi?id=272543 Reviewed by Antti Koivisto. A partial revert of 272448.795@safari-7618-branch. Use the EventTargetFlag that's available instead of making m_previous CompactPointerTuple. * Source/WebCore/cssjit/SelectorCompiler.cpp: (WebCore::SelectorCompiler::SelectorCodeGenerator::generateWalkToPreviousAdjacentElement): * Source/WebCore/dom/Element.cpp: (WebCore::Element::~Element): (WebCore::Element::identifier const): * Source/WebCore/dom/EventTarget.h: * Source/WebCore/dom/Node.cpp: (WebCore::Node::~Node): * Source/WebCore/dom/Node.h: (WebCore::Node::previousSibling const): (WebCore::Node::protectedPreviousSibling const): (WebCore::Node::previousSiblingMemoryOffset): (WebCore::Node::setPreviousSibling): (WebCore::Node::previousSiblingPointerMask): Deleted. (WebCore::Node::hasElementStateFlag const): Deleted. (WebCore::Node::clearElementStateFlag const): Deleted. (WebCore::Node::setElementStateFlag const): Deleted. * Source/WebCore/domjit/JSNodeDOMJIT.cpp: (WebCore::createCallDOMGetterForOffsetAccess): (WebCore::compileNodePreviousSiblingAttribute): Canonical link: https://commits.webkit.org/272448.907@safari-7618-branch
WebKit · Apr 12, 2024 · e75016c · e75016c
1 parent cb2f032
commit e75016c
Show file tree

Hide file tree

Showing 6 changed files with 13 additions and 42 deletions.
diff --git a/Source/WebCore/cssjit/SelectorCompiler.cpp b/Source/WebCore/cssjit/SelectorCompiler.cpp
@@ -2673,9 +2673,6 @@ inline void SelectorCodeGenerator::generateWalkToPreviousAdjacentElement(Assembl
 {
     Assembler::Label loopStart = m_assembler.label();
     m_assembler.loadPtr(Assembler::Address(workRegister, Node::previousSiblingMemoryOffset()), workRegister);
-    LocalRegister previousNodePointerMask(m_registerAllocator);
-    m_assembler.move(Assembler::TrustedImmPtr(Node::previousSiblingPointerMask()), previousNodePointerMask);
-    m_assembler.andPtr(previousNodePointerMask, workRegister);
     failureCases.append(m_assembler.branchTestPtr(Assembler::Zero, workRegister));
     DOMJIT::branchTestIsElementFlagOnNode(m_assembler, Assembler::Zero, workRegister).linkTo(loopStart, &m_assembler);
 }

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -258,7 +258,7 @@ Element::~Element()
     ASSERT(!beforePseudoElement());
     ASSERT(!afterPseudoElement());
 
-    if (UNLIKELY(hasElementStateFlag(ElementStateFlag::HasElementIdentifier)))
+    if (UNLIKELY(hasEventTargetFlag(EventTargetFlag::HasElementIdentifier)))
         elementIdentifiersMap().remove(*this);
     else
         ASSERT(!elementIdentifiersMap().contains(*this));
@@ -5462,7 +5462,7 @@ Vector<RefPtr<WebAnimation>> Element::getAnimations(std::optional<GetAnimationsO
 ElementIdentifier Element::identifier() const
 {
     return elementIdentifiersMap().ensure(const_cast<Element&>(*this), [&] {
-        setElementStateFlag(ElementStateFlag::HasElementIdentifier);
+        const_cast<Element&>(*this).setEventTargetFlag(EventTargetFlag::HasElementIdentifier);
         return ElementIdentifier::generate();
     }).iterator->value;
 }

diff --git a/Source/WebCore/dom/EventTarget.h b/Source/WebCore/dom/EventTarget.h
@@ -202,9 +202,9 @@ class EventTarget : public ScriptWrappable, public CanMakeWeakPtrWithBitField<Ev
         HasFormAssociatedCustomElementInterface = 1 << 11,
         HasShadowRootContainingSlots = 1 << 12,
         IsInTopLayer = 1 << 13,
+        HasElementIdentifier = 1 << 14,
         // SVGElement bits
-        HasPendingResources = 1 << 14,
-        // 1 Free bits
+        HasPendingResources = 1 << 15,
     };
 
     EventTargetData& ensureEventTargetData()

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -117,7 +117,7 @@ struct SameSizeAsNode : EventTarget {
     uint32_t nodeFlags;
     void* parentNode;
     void* treeScope;
-    uint8_t previous[8];
+    void* previous;
     void* next;
     uint8_t rendererWithStyleFlags[8];
     uint8_t rareDataWithBitfields[8];
@@ -430,7 +430,7 @@ Node::~Node()
 
     ASSERT(!renderer());
     ASSERT(!parentNode());
-    ASSERT(!m_previous.pointer());
+    ASSERT(!m_previous);
     ASSERT(!m_next);
 
     {

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
@@ -145,15 +145,10 @@ class Node : public EventTarget {
     inline RefPtr<ContainerNode> protectedParentNode() const; // Defined in ContainerNode.h.
     static ptrdiff_t parentNodeMemoryOffset() { return OBJECT_OFFSETOF(Node, m_parentNode); }
     inline Element* parentElement() const;
-    Node* previousSibling() const { return m_previous.pointer(); }
-    RefPtr<Node> protectedPreviousSibling() const { return m_previous.pointer(); }
+    Node* previousSibling() const { return m_previous; }
+    RefPtr<Node> protectedPreviousSibling() const { return m_previous; }
 
     static ptrdiff_t previousSiblingMemoryOffset() { return OBJECT_OFFSETOF(Node, m_previous); }
-#if CPU(ADDRESS64)
-    static uintptr_t previousSiblingPointerMask() { return CompactPointerTuple<Node*, uint16_t>::pointerMask; }
-#else
-    static uintptr_t previousSiblingPointerMask() { return -1; }
-#endif
     Node* nextSibling() const { return m_next; }
     RefPtr<Node> protectedNextSibling() const { return m_next; }
     static ptrdiff_t nextSiblingMemoryOffset() { return OBJECT_OFFSETOF(Node, m_next); }
@@ -335,7 +330,7 @@ class Node : public EventTarget {
     WEBCORE_EXPORT Element* enclosingLinkEventParentOrSelf();
 
     // These low-level calls give the caller responsibility for maintaining the integrity of the tree.
-    void setPreviousSibling(Node* previous) { m_previous.setPointer(previous); }
+    void setPreviousSibling(Node* previous) { m_previous = previous; }
     void setNextSibling(Node* next) { m_next = next; }
 
     virtual bool canContainRangeEndPoint() const { return false; }
@@ -632,11 +627,6 @@ class Node : public EventTarget {
         IsInCustomElementReactionQueue = 1 << 15,
     };
 
-    enum class ElementStateFlag : uint16_t {
-        HasElementIdentifier = 1 << 0,
-        // 15-bits free.
-    };
-
     enum class TabIndexState : uint8_t {
         NotSet = 0,
         Zero = 1,
@@ -663,10 +653,6 @@ class Node : public EventTarget {
     void setStateFlag(StateFlag flag, bool value = true) const { m_stateFlags.set(flag, value); }
     void clearStateFlag(StateFlag flag) const { setStateFlag(flag, false); }
 
-    bool hasElementStateFlag(ElementStateFlag flag) const { return OptionSet<ElementStateFlag>::fromRaw(m_previous.type()).contains(flag); }
-    inline void setElementStateFlag(ElementStateFlag, bool value = true) const;
-    void clearElementStateFlag(ElementStateFlag flag) const { setElementStateFlag(flag, false); }
-
     RareDataBitFields rareDataBitfields() const { return bitwise_cast<RareDataBitFields>(m_rareDataWithBitfields.type()); }
     void setRareDataBitfields(RareDataBitFields bitfields) { m_rareDataWithBitfields.setType(bitwise_cast<uint16_t>(bitfields)); }
 
@@ -782,7 +768,7 @@ class Node : public EventTarget {
 
     ContainerNode* m_parentNode { nullptr };
     TreeScope* m_treeScope { nullptr };
-    CompactPointerTuple<Node*, uint16_t> m_previous;
+    Node* m_previous { nullptr };
     Node* m_next { nullptr };
     CompactPointerTuple<RenderObject*, uint16_t> m_rendererWithStyleFlags;
     CompactUniquePtrTuple<NodeRareData, uint16_t> m_rareDataWithBitfields;
@@ -877,13 +863,6 @@ inline ContainerNode* Node::parentNodeGuaranteedHostFree() const
     return parentNode();
 }
 
-inline void Node::setElementStateFlag(ElementStateFlag flag, bool value) const
-{
-    auto set = OptionSet<ElementStateFlag>::fromRaw(m_previous.type());
-    set.set(flag, value);
-    const_cast<Node&>(*this).m_previous.setType(set.toRaw());
-}
-
 ALWAYS_INLINE void Node::setStyleFlag(NodeStyleFlag flag)
 {
     auto bitfields = styleBitfields();

diff --git a/Source/WebCore/domjit/JSNodeDOMJIT.cpp b/Source/WebCore/domjit/JSNodeDOMJIT.cpp
@@ -48,10 +48,10 @@ Ref<JSC::Snippet> checkSubClassSnippetForJSNode()
 enum class IsContainerGuardRequirement { Required, NotRequired };
 
 template<typename WrappedNode, typename ToWrapperOperation>
-static Ref<JSC::DOMJIT::CallDOMGetterSnippet> createCallDOMGetterForOffsetAccess(ptrdiff_t offset, ToWrapperOperation operation, IsContainerGuardRequirement isContainerGuardRequirement, std::optional<uintptr_t> mask = std::nullopt)
+static Ref<JSC::DOMJIT::CallDOMGetterSnippet> createCallDOMGetterForOffsetAccess(ptrdiff_t offset, ToWrapperOperation operation, IsContainerGuardRequirement isContainerGuardRequirement)
 {
     Ref<JSC::DOMJIT::CallDOMGetterSnippet> snippet = JSC::DOMJIT::CallDOMGetterSnippet::create();
-    snippet->numGPScratchRegisters = mask ? 2 : 1;
+    snippet->numGPScratchRegisters = 1;
     snippet->setGenerator([=](CCallHelpers& jit, JSC::SnippetParams& params) {
         JSValueRegs result = params[0].jsValueRegs();
         GPRReg node = params[1].gpr();
@@ -68,11 +68,6 @@ static Ref<JSC::DOMJIT::CallDOMGetterSnippet> createCallDOMGetterForOffsetAccess
             nullCases.append(jit.branchTest16(CCallHelpers::Zero, CCallHelpers::Address(scratch, Node::typeFlagsMemoryOffset()), CCallHelpers::TrustedImm32(Node::flagIsContainer())));
 
         jit.loadPtr(CCallHelpers::Address(scratch, offset), scratch);
-        if (mask) {
-            GPRReg pointerMask = params.gpScratch(1);
-            jit.move(CCallHelpers::TrustedImmPtr(*mask), pointerMask);
-            jit.andPtr(pointerMask, scratch);
-        }
         nullCases.append(jit.branchTestPtr(CCallHelpers::Zero, scratch));
 
         DOMJIT::toWrapper<WrappedNode>(jit, params, scratch, globalObject, result, operation, globalObjectValue);
@@ -109,7 +104,7 @@ Ref<JSC::DOMJIT::CallDOMGetterSnippet> compileNodeNextSiblingAttribute()
 
 Ref<JSC::DOMJIT::CallDOMGetterSnippet> compileNodePreviousSiblingAttribute()
 {
-    auto snippet = createCallDOMGetterForOffsetAccess<Node>(Node::previousSiblingMemoryOffset(), DOMJIT::operationToJSNode, IsContainerGuardRequirement::NotRequired, Node::previousSiblingPointerMask());
+    auto snippet = createCallDOMGetterForOffsetAccess<Node>(Node::previousSiblingMemoryOffset(), DOMJIT::operationToJSNode, IsContainerGuardRequirement::NotRequired);
     snippet->effect = JSC::DOMJIT::Effect::forDef(DOMJIT::AbstractHeapRepository::Node_previousSibling);
     return snippet;
 }