Attachment 347106 Details for Bug 188575 – Patch

[patch] Patch

bug-188575-20180814163430.patch (text/plain), 13.34 KB, created by Ali Juma on 2018-08-14 13:34:31 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ali Juma

Created: 2018-08-14 13:34:31 PDT

Size: 13.34 KB

patch

obsolete

>Subversion Revision: 234851
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index af596853ede77abeb689bf5840259ca530c16b9e..d4837d8bc3a9ad00c892e57f2c3363c79458d572 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-08-14  Ali Juma  <ajuma@chromium.org>
>+
>+        [IntersectionObserver] Do not hold a strong reference to the root element
>+        https://bugs.webkit.org/show_bug.cgi?id=188575
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make IntersectionObserver have only a raw pointer to its root element rather than
>+        a reference, so that an otherwise-unreachable root isn't kept alive. Add logic to
>+        to clear this pointer when the root element gets deleted.
>+
>+        Test: intersection-observer/root-element-deleted.html
>+
>+        * dom/Element.cpp:
>+        (WebCore::Element::~Element):
>+        (WebCore::Element::disconnectFromIntersectionObservers):
>+        (WebCore::Element::ensureIntersectionObserverData):
>+        (WebCore::Element::intersectionObserverData):
>+        * dom/Element.h:
>+        * dom/ElementRareData.cpp:
>+        * dom/ElementRareData.h:
>+        (WebCore::ElementRareData::intersectionObserverData):
>+        (WebCore::ElementRareData::setIntersectionObserverData):
>+        * page/IntersectionObserver.cpp:
>+        (WebCore::IntersectionObserver::create):
>+        (WebCore::IntersectionObserver::IntersectionObserver):
>+        (WebCore::IntersectionObserver::~IntersectionObserver):
>+        (WebCore::IntersectionObserver::rootDestroyed):
>+        * page/IntersectionObserver.h:
>+        (WebCore::IntersectionObserver::root const):
>+
> 2018-08-14  Ali Juma  <ajuma@chromium.org>
> 
>         Follow-up: [IntersectionObserver] Implement rootMargin parsing
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index 69e6f90c1693dd7749384c2c7e2db719c7b3d206..b47c8932587bb190f8222bad8bf583f59e90b770 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -187,6 +187,10 @@ Element::~Element()
>     ASSERT(!beforePseudoElement());
>     ASSERT(!afterPseudoElement());
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    disconnectFromIntersectionObservers();
>+#endif
>+
>     removeShadowRoot();
> 
>     if (hasSyntheticAttrChildNodes())
>@@ -3233,6 +3237,32 @@ void Element::requestPointerLock()
> }
> #endif
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+void Element::disconnectFromIntersectionObservers()
>+{
>+    auto* observerData = intersectionObserverData();
>+    if (!observerData)
>+        return;
>+
>+    for (auto* observer : observerData->observers)
>+        observer->rootDestroyed();
>+    observerData->observers.clear();
>+}
>+
>+IntersectionObserverData& Element::ensureIntersectionObserverData()
>+{
>+    auto& rareData = ensureElementRareData();
>+    if (!rareData.intersectionObserverData())
>+        rareData.setIntersectionObserverData(std::make_unique<IntersectionObserverData>());
>+    return *rareData.intersectionObserverData();
>+}
>+
>+IntersectionObserverData* Element::intersectionObserverData()
>+{
>+    return hasRareData() ? elementRareData()->intersectionObserverData() : nullptr;
>+}
>+#endif
>+
> SpellcheckAttributeState Element::spellcheckAttributeState() const
> {
>     const AtomicString& value = attributeWithoutSynchronization(HTMLNames::spellcheckAttr);
>diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
>index 8c3905b3d93348c9b9c740732b6da9abe4d66619..f94cce0afe288ad68f487a1493c05a3ed9cef4a2 100644
>--- a/Source/WebCore/dom/Element.h
>+++ b/Source/WebCore/dom/Element.h
>@@ -56,6 +56,10 @@ class RenderTreePosition;
> class WebAnimation;
> struct ElementStyle;
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+struct IntersectionObserverData;
>+#endif
>+
> enum SpellcheckAttributeState {
>     SpellcheckAttributeTrue,
>     SpellcheckAttributeFalse,
>@@ -567,6 +571,11 @@ public:
>     using ContainerNode::setAttributeEventListener;
>     void setAttributeEventListener(const AtomicString& eventType, const QualifiedName& attributeName, const AtomicString& value);
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    IntersectionObserverData& ensureIntersectionObserverData();
>+    IntersectionObserverData* intersectionObserverData();
>+#endif
>+
>     Element* findAnchorElementForLink(String& outAnchorName);
> 
>     ExceptionOr<Ref<WebAnimation>> animate(JSC::ExecState&, JSC::Strong<JSC::JSObject>&&, std::optional<Variant<double, KeyframeAnimationOptions>>&&);
>@@ -646,6 +655,10 @@ private:
>     void formatForDebugger(char* buffer, unsigned length) const override;
> #endif
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    void disconnectFromIntersectionObservers();
>+#endif
>+
>     // The cloneNode function is private so that non-virtual cloneElementWith/WithoutChildren are used instead.
>     Ref<Node> cloneNodeInternal(Document&, CloningOperation) override;
>     virtual Ref<Element> cloneElementWithoutAttributesAndChildren(Document&);
>diff --git a/Source/WebCore/dom/ElementRareData.cpp b/Source/WebCore/dom/ElementRareData.cpp
>index 478a8887e0051acabb620e375ab6022528195bce..6cfa8096d3f58407dd0a86cc73f8f7e0c360e579 100644
>--- a/Source/WebCore/dom/ElementRareData.cpp
>+++ b/Source/WebCore/dom/ElementRareData.cpp
>@@ -44,6 +44,10 @@ struct SameSizeAsElementRareData : NodeRareData {
>     LayoutSize sizeForResizing;
>     IntPoint savedLayerScrollPosition;
>     void* pointers[8];
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    void* intersectionObserverData;
>+#endif
>+
> };
> 
> static_assert(sizeof(ElementRareData) == sizeof(SameSizeAsElementRareData), "ElementRareData should stay small");
>diff --git a/Source/WebCore/dom/ElementRareData.h b/Source/WebCore/dom/ElementRareData.h
>index a0f301e874a4c0af143930adadde62c5ea8fbbff..e719b3d49daa70b6f21fbee91f1d12d5f6dcc491 100644
>--- a/Source/WebCore/dom/ElementRareData.h
>+++ b/Source/WebCore/dom/ElementRareData.h
>@@ -24,6 +24,7 @@
> #include "CustomElementReactionQueue.h"
> #include "DOMTokenList.h"
> #include "DatasetDOMStringMap.h"
>+#include "IntersectionObserver.h"
> #include "NamedNodeMap.h"
> #include "NodeRareData.h"
> #include "PseudoElement.h"
>@@ -116,6 +117,11 @@ public:
>     bool hasCSSAnimation() const { return m_hasCSSAnimation; }
>     void setHasCSSAnimation(bool value) { m_hasCSSAnimation = value; }
> 
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    IntersectionObserverData* intersectionObserverData() { return m_intersectionObserverData.get(); }
>+    void setIntersectionObserverData(std::unique_ptr<IntersectionObserverData>&& data) { m_intersectionObserverData = WTFMove(data); }
>+#endif
>+
> private:
>     int m_tabIndex;
>     unsigned short m_childIndex;
>@@ -149,6 +155,9 @@ private:
>     RefPtr<ShadowRoot> m_shadowRoot;
>     std::unique_ptr<CustomElementReactionQueue> m_customElementReactionQueue;
>     std::unique_ptr<NamedNodeMap> m_attributeMap;
>+#if ENABLE(INTERSECTION_OBSERVER)
>+    std::unique_ptr<IntersectionObserverData> m_intersectionObserverData;
>+#endif
> 
>     RefPtr<PseudoElement> m_beforePseudoElement;
>     RefPtr<PseudoElement> m_afterPseudoElement;
>diff --git a/Source/WebCore/page/IntersectionObserver.cpp b/Source/WebCore/page/IntersectionObserver.cpp
>index 83cb2b0a056938dce242470f6f9fe0019b639800..e3db209e0e095ec52ca1f2dcbf98b799f92a5077 100644
>--- a/Source/WebCore/page/IntersectionObserver.cpp
>+++ b/Source/WebCore/page/IntersectionObserver.cpp
>@@ -100,15 +100,32 @@ ExceptionOr<Ref<IntersectionObserver>> IntersectionObserver::create(Ref<Intersec
>             return Exception { RangeError, "Failed to construct 'IntersectionObserver': all thresholds must lie in the range [0.0, 1.0]." };
>     }
> 
>-    return adoptRef(*new IntersectionObserver(WTFMove(callback), WTFMove(init.root), rootMarginOrException.releaseReturnValue(), WTFMove(thresholds)));
>+    return adoptRef(*new IntersectionObserver(WTFMove(callback), init.root, rootMarginOrException.releaseReturnValue(), WTFMove(thresholds)));
> }
> 
>-IntersectionObserver::IntersectionObserver(Ref<IntersectionObserverCallback>&& callback, RefPtr<Element>&& root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds)
>-    : m_root(WTFMove(root))
>+IntersectionObserver::IntersectionObserver(Ref<IntersectionObserverCallback>&& callback, Element* root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds)
>+    : m_root(root)
>     , m_rootMargin(WTFMove(parsedRootMargin))
>     , m_thresholds(WTFMove(thresholds))
>     , m_callback(WTFMove(callback))
> {
>+    if (m_root) {
>+        auto& observerData = m_root->ensureIntersectionObserverData();
>+        observerData.observers.append(this);
>+    }
>+}
>+
>+IntersectionObserver::~IntersectionObserver()
>+{
>+    if (m_root) {
>+        auto& elementObservers = m_root->intersectionObserverData()->observers;
>+        for (size_t i = 0; i < elementObservers.size(); ++i) {
>+            if (elementObservers.at(i) == this) {
>+                elementObservers.remove(i);
>+                break;
>+            }
>+        }
>+    }
> }
> 
> String IntersectionObserver::rootMargin() const
>@@ -145,6 +162,12 @@ Vector<RefPtr<IntersectionObserverEntry>> IntersectionObserver::takeRecords()
>     return { };
> }
> 
>+void IntersectionObserver::rootDestroyed()
>+{
>+    ASSERT(m_root);
>+    disconnect();
>+    m_root = nullptr;
>+}
> 
> } // namespace WebCore
> 
>diff --git a/Source/WebCore/page/IntersectionObserver.h b/Source/WebCore/page/IntersectionObserver.h
>index b04cfb21162855b21b670450130895349a2e14e2..961d8c45217058235c8039460c1b3b78a8e8bc67 100644
>--- a/Source/WebCore/page/IntersectionObserver.h
>+++ b/Source/WebCore/page/IntersectionObserver.h
>@@ -38,17 +38,26 @@ namespace WebCore {
> 
> class Element;
> 
>+struct IntersectionObserverData {
>+    // IntersectionObservers for which the element that owns this IntersectionObserverData is the root.
>+    Vector<IntersectionObserver*> observers;
>+
>+    // FIXME: Also track IntersectionObservers for which the element that owns this IntersectionObserverData is the target.
>+};
>+
> class IntersectionObserver : public RefCounted<IntersectionObserver> {
> public:
>     struct Init {
>-        RefPtr<Element> root;
>+        Element* root { nullptr };
>         String rootMargin;
>         Variant<double, Vector<double>> threshold;
>     };
> 
>     static ExceptionOr<Ref<IntersectionObserver>> create(Ref<IntersectionObserverCallback>&&, Init&&);
>-    
>-    Element* root() const { return m_root.get(); }
>+
>+    ~IntersectionObserver();
>+
>+    Element* root() const { return m_root; }
>     String rootMargin() const;
>     const Vector<double>& thresholds() const { return m_thresholds; }
> 
>@@ -58,10 +67,12 @@ public:
> 
>     Vector<RefPtr<IntersectionObserverEntry>> takeRecords();
> 
>+    void rootDestroyed();
>+
> private:
>-    IntersectionObserver(Ref<IntersectionObserverCallback>&&, RefPtr<Element>&& root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds);
>+    IntersectionObserver(Ref<IntersectionObserverCallback>&&, Element* root, LengthBox&& parsedRootMargin, Vector<double>&& thresholds);
>     
>-    RefPtr<Element> m_root;
>+    Element* m_root;
>     LengthBox m_rootMargin;
>     Vector<double> m_thresholds;
>     Ref<IntersectionObserverCallback> m_callback;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ee74960aa013c9403aaef2f8ec800f1e20e920f0..7ddcfc3481ca48277b0a6039c0010d670a340664 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-08-14  Ali Juma  <ajuma@chromium.org>
>+
>+        [IntersectionObserver] Do not hold a strong reference to the root element
>+        https://bugs.webkit.org/show_bug.cgi?id=188575
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * intersection-observer/root-element-deleted-expected.txt: Added.
>+        * intersection-observer/root-element-deleted.html: Added.
>+
> 2018-08-14  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] Crash under AnimationTimeline::cancelOrRemoveDeclarativeAnimation()
>diff --git a/LayoutTests/intersection-observer/root-element-deleted-expected.txt b/LayoutTests/intersection-observer/root-element-deleted-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..7df05d6abca7a6ba1f759bd91f913e1f7b768bb8
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/root-element-deleted-expected.txt
>@@ -0,0 +1,3 @@
>+
>+PASS IntersectionObserver doesn't keep unreachable root alive 
>+
>diff --git a/LayoutTests/intersection-observer/root-element-deleted.html b/LayoutTests/intersection-observer/root-element-deleted.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..86d436b4b9309e48f30ae27c978bce00db8ef83e
>--- /dev/null
>+++ b/LayoutTests/intersection-observer/root-element-deleted.html
>@@ -0,0 +1,28 @@
>+<!DOCTYPE html>
>+<head>
>+<script src="../resources/testharness.js"></script>
>+<script src="../resources/testharnessreport.js"></script>
>+<body>
>+
>+<div id="root" style="position:absolute">
>+    <div id="target" style="width: 100px; height: 100px; background-color: green"></div>
>+</div>
>+
>+<script>
>+    async_test(function(t) {
>+        var root = document.getElementById('root');
>+        var observer = new IntersectionObserver(function() {}, { root: document.getElementById('root') });;
>+        var target = document.getElementById('target');
>+        assert_equals(observer.root, root, 'Observer starts out with non-null root');
>+        observer.observe(target);
>+        root.parentNode.removeChild(root);
>+        root = null;
>+        target = null;
>+        requestAnimationFrame(t.step_func_done(function() {
>+            GCController.collect();
>+            assert_equals(observer.root, null, 'Observer has null root after root element is destroyed');
>+        }));
>+    }, "IntersectionObserver doesn't keep unreachable root alive");
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 188575: 347106 | 347159