From 8b71cda1956ab640ad76bea36e7971476aa78bcf Mon Sep 17 00:00:00 2001 From: wolfbeast Date: Wed, 27 Jun 2018 16:00:53 +0200 Subject: Stabilize and align Intersection Observers - Fixes several crashes - Aligns the feature with the W3C WD spec Tag #249 --- dom/base/DOMIntersectionObserver.cpp | 37 +++++++++++++---- dom/base/DOMIntersectionObserver.h | 4 +- dom/base/Element.cpp | 45 ++++++++++++-------- dom/base/Element.h | 2 +- dom/base/FragmentOrElement.cpp | 8 ++++ dom/base/FragmentOrElement.h | 8 +--- dom/base/nsDocument.cpp | 3 +- dom/base/nsNodeUtils.cpp | 9 ---- dom/base/test/test_intersectionobservers.html | 59 ++++++++++++++++++--------- 9 files changed, 111 insertions(+), 64 deletions(-) (limited to 'dom/base') diff --git a/dom/base/DOMIntersectionObserver.cpp b/dom/base/DOMIntersectionObserver.cpp index 169c3fe7a..e39abf1a6 100644 --- a/dom/base/DOMIntersectionObserver.cpp +++ b/dom/base/DOMIntersectionObserver.cpp @@ -47,6 +47,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMIntersectionObserver) NS_IMPL_CYCLE_COLLECTION_UNLINK(mCallback) NS_IMPL_CYCLE_COLLECTION_UNLINK(mRoot) NS_IMPL_CYCLE_COLLECTION_UNLINK(mQueuedEntries) + tmp->Disconnect(); NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMIntersectionObserver) @@ -184,9 +185,10 @@ DOMIntersectionObserver::Connect() if (mConnected) { return; } + mConnected = true; + nsIDocument* document = mOwner->GetExtantDoc(); document->AddIntersectionObserver(this); - mConnected = true; } void @@ -202,7 +204,9 @@ DOMIntersectionObserver::Disconnect() mObservationTargets.Clear(); if (mOwner) { nsIDocument* document = mOwner->GetExtantDoc(); - document->RemoveIntersectionObserver(this); + if (document) { + document->RemoveIntersectionObserver(this); + } } mConnected = false; } @@ -248,6 +252,12 @@ EdgeInclusiveIntersection(const nsRect& aRect, const nsRect& aOtherRect) return Some(nsRect(left, top, right - left, bottom - top)); } +enum class BrowsingContextInfo { + SimilarOriginBrowsingContext, + DifferentOriginBrowsingContext, + UnknownBrowsingContext +}; + void DOMIntersectionObserver::Update(nsIDocument* aDocument, DOMHighResTimeStamp time) { @@ -359,11 +369,22 @@ DOMIntersectionObserver::Update(nsIDocument* aDocument, DOMHighResTimeStamp time } } - nsRect rootIntersectionRect = rootRect; - bool isInSimilarOriginBrowsingContext = rootFrame && targetFrame && - CheckSimilarOrigin(root, target); + nsRect rootIntersectionRect; + BrowsingContextInfo isInSimilarOriginBrowsingContext = + BrowsingContextInfo::UnknownBrowsingContext; + + if (rootFrame && targetFrame) { + rootIntersectionRect = rootRect; + } + + if (root && target) { + isInSimilarOriginBrowsingContext = CheckSimilarOrigin(root, target) ? + BrowsingContextInfo::SimilarOriginBrowsingContext : + BrowsingContextInfo::DifferentOriginBrowsingContext; + } - if (isInSimilarOriginBrowsingContext) { + if (isInSimilarOriginBrowsingContext == + BrowsingContextInfo::SimilarOriginBrowsingContext) { rootIntersectionRect.Inflate(rootMargin); } @@ -413,7 +434,9 @@ DOMIntersectionObserver::Update(nsIDocument* aDocument, DOMHighResTimeStamp time if (target->UpdateIntersectionObservation(this, threshold)) { QueueIntersectionObserverEntry( target, time, - isInSimilarOriginBrowsingContext ? Some(rootIntersectionRect) : Nothing(), + isInSimilarOriginBrowsingContext == + BrowsingContextInfo::DifferentOriginBrowsingContext ? + Nothing() : Some(rootIntersectionRect), targetRect, intersectionRect, intersectionRatio ); } diff --git a/dom/base/DOMIntersectionObserver.h b/dom/base/DOMIntersectionObserver.h index 3eb10ad38..8144fc5c5 100644 --- a/dom/base/DOMIntersectionObserver.h +++ b/dom/base/DOMIntersectionObserver.h @@ -101,9 +101,7 @@ protected: class DOMIntersectionObserver final : public nsISupports, public nsWrapperCache { - virtual ~DOMIntersectionObserver() { - Disconnect(); - } + virtual ~DOMIntersectionObserver() { } public: DOMIntersectionObserver(already_AddRefed&& aOwner, diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp index 092755590..52d06b0f8 100644 --- a/dom/base/Element.cpp +++ b/dom/base/Element.cpp @@ -3912,44 +3912,55 @@ Element::ClearDataset() slots->mDataset = nullptr; } -nsTArray* +nsDataHashtable, int32_t>* Element::RegisteredIntersectionObservers() { nsDOMSlots* slots = DOMSlots(); return &slots->mRegisteredIntersectionObservers; } +enum nsPreviousIntersectionThreshold { + eUninitialized = -2, + eNonIntersecting = -1 +}; + void Element::RegisterIntersectionObserver(DOMIntersectionObserver* aObserver) { - RegisteredIntersectionObservers()->AppendElement( - nsDOMSlots::IntersectionObserverRegistration { aObserver, -1 }); + nsDataHashtable, int32_t>* observers = + RegisteredIntersectionObservers(); + if (observers->Contains(aObserver)) { + return; + } + + // Value can be: + // -2: Makes sure next calculated threshold always differs, leading to a + // notification task being scheduled. + // -1: Non-intersecting. + // >= 0: Intersecting, valid index of aObserver->mThresholds. + RegisteredIntersectionObservers()->Put(aObserver, eUninitialized); } void Element::UnregisterIntersectionObserver(DOMIntersectionObserver* aObserver) { - nsTArray* observers = + nsDataHashtable, int32_t>* observers = RegisteredIntersectionObservers(); - for (uint32_t i = 0; i < observers->Length(); ++i) { - nsDOMSlots::IntersectionObserverRegistration reg = observers->ElementAt(i); - if (reg.observer == aObserver) { - observers->RemoveElementAt(i); - break; - } - } + observers->Remove(aObserver); } bool Element::UpdateIntersectionObservation(DOMIntersectionObserver* aObserver, int32_t aThreshold) { - nsTArray* observers = + nsDataHashtable, int32_t>* observers = RegisteredIntersectionObservers(); - for (auto& reg : *observers) { - if (reg.observer == aObserver && reg.previousThreshold != aThreshold) { - reg.previousThreshold = aThreshold; - return true; - } + if (!observers->Contains(aObserver)) { + return false; + } + int32_t previousThreshold = observers->Get(aObserver); + if (previousThreshold != aThreshold) { + observers->Put(aObserver, aThreshold); + return true; } return false; } diff --git a/dom/base/Element.h b/dom/base/Element.h index cf1d197e2..049984d1b 100644 --- a/dom/base/Element.h +++ b/dom/base/Element.h @@ -1380,7 +1380,7 @@ protected: nsDOMTokenList* GetTokenList(nsIAtom* aAtom, const DOMTokenListSupportedTokenArray aSupportedTokens = nullptr); - nsTArray* RegisteredIntersectionObservers(); + nsDataHashtable, int32_t>* RegisteredIntersectionObservers(); private: /** diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp index b22a0d4ff..fde983e7c 100644 --- a/dom/base/FragmentOrElement.cpp +++ b/dom/base/FragmentOrElement.cpp @@ -608,6 +608,7 @@ FragmentOrElement::nsDOMSlots::Unlink(bool aIsXUL) mLabelsList = nullptr; mCustomElementData = nullptr; mClassList = nullptr; + mRegisteredIntersectionObservers.Clear(); } size_t @@ -1359,6 +1360,13 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement) { nsDOMSlots *slots = tmp->GetExistingDOMSlots(); if (slots) { + if (tmp->IsElement()) { + Element* elem = tmp->AsElement(); + for (auto iter = slots->mRegisteredIntersectionObservers.Iter(); !iter.Done(); iter.Next()) { + DOMIntersectionObserver* observer = iter.Key(); + observer->UnlinkTarget(*elem); + } + } slots->Unlink(tmp->IsXULElement()); } } diff --git a/dom/base/FragmentOrElement.h b/dom/base/FragmentOrElement.h index 1cd8033bb..7c74e9cd4 100644 --- a/dom/base/FragmentOrElement.h +++ b/dom/base/FragmentOrElement.h @@ -21,6 +21,7 @@ #include "nsIWeakReference.h" // base class #include "nsNodeUtils.h" // class member nsNodeUtils::CloneNodeImpl #include "nsIHTMLCollection.h" +#include "nsDataHashtable.h" class ContentUnbinder; class nsContentList; @@ -353,12 +354,7 @@ public: /** * Registered Intersection Observers on the element. */ - struct IntersectionObserverRegistration { - DOMIntersectionObserver* observer; - int32_t previousThreshold; - }; - - nsTArray mRegisteredIntersectionObservers; + nsDataHashtable, int32_t> mRegisteredIntersectionObservers; }; protected: diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp index fd3b52948..8acfd901a 100644 --- a/dom/base/nsDocument.cpp +++ b/dom/base/nsDocument.cpp @@ -12507,7 +12507,8 @@ nsDocument::ScheduleIntersectionObserverNotification() void nsDocument::NotifyIntersectionObservers() { - for (const auto& observer : mIntersectionObservers) { + nsTArray> observers(mIntersectionObservers); + for (const auto& observer : observers) { observer->Notify(); } } diff --git a/dom/base/nsNodeUtils.cpp b/dom/base/nsNodeUtils.cpp index 69a9414fe..ecea95dc1 100644 --- a/dom/base/nsNodeUtils.cpp +++ b/dom/base/nsNodeUtils.cpp @@ -297,15 +297,6 @@ nsNodeUtils::LastRelease(nsINode* aNode) NodeWillBeDestroyed, (aNode)); } - if (aNode->IsElement()) { - Element* elem = aNode->AsElement(); - FragmentOrElement::nsDOMSlots* domSlots = - static_cast(slots); - for (auto& reg : domSlots->mRegisteredIntersectionObservers) { - reg.observer->UnlinkTarget(*elem); - } - } - delete slots; aNode->mSlots = nullptr; } diff --git a/dom/base/test/test_intersectionobservers.html b/dom/base/test/test_intersectionobservers.html index e7875e3af..10e3d7712 100644 --- a/dom/base/test/test_intersectionobservers.html +++ b/dom/base/test/test_intersectionobservers.html @@ -325,7 +325,7 @@ limitations under the License. }); - it('does not trigger if target does not intersect when observing begins', + it('does trigger if target does not intersect when observing begins', function(done) { var spy = sinon.spy(); @@ -334,7 +334,7 @@ limitations under the License. targetEl2.style.top = '-40px'; io.observe(targetEl2); callDelayed(function() { - expect(spy.callCount).to.be(0); + expect(spy.callCount).to.be(1); done(); }, ASYNC_TIMEOUT); }); @@ -528,7 +528,7 @@ limitations under the License. spy.waitForNotification(function() { expect(spy.callCount).to.be(1); var records = sortRecords(spy.lastCall.args[0]); - expect(records.length).to.be(2); + expect(records.length).to.be(3); expect(records[0].target).to.be(targetEl1); expect(records[0].intersectionRatio).to.be(0.25); expect(records[1].target).to.be(targetEl2); @@ -636,10 +636,10 @@ limitations under the License. expect(records.length).to.be(3); expect(records[0].target).to.be(targetEl1); expect(records[0].intersectionRatio).to.be(0.5); - expect(records[1].target).to.be(targetEl3); - expect(records[1].intersectionRatio).to.be(0.5); - expect(records[2].target).to.be(targetEl4); + expect(records[2].target).to.be(targetEl3); expect(records[2].intersectionRatio).to.be(0.5); + expect(records[3].target).to.be(targetEl4); + expect(records[3].intersectionRatio).to.be(0.5); io.disconnect(); done(); }, {root: rootEl, rootMargin: '-10px 10%'}); @@ -652,11 +652,11 @@ limitations under the License. function(done) { io = new IntersectionObserver(function(records) { records = sortRecords(records); - expect(records.length).to.be(2); + expect(records.length).to.be(4); expect(records[0].target).to.be(targetEl1); expect(records[0].intersectionRatio).to.be(0.5); - expect(records[1].target).to.be(targetEl4); - expect(records[1].intersectionRatio).to.be(0.5); + expect(records[3].target).to.be(targetEl4); + expect(records[3].intersectionRatio).to.be(0.5); io.disconnect(); done(); }, {root: rootEl, rootMargin: '-5% -2.5% 0px'}); @@ -669,13 +669,13 @@ limitations under the License. function(done) { io = new IntersectionObserver(function(records) { records = sortRecords(records); - expect(records.length).to.be(3); + expect(records.length).to.be(4); expect(records[0].target).to.be(targetEl1); expect(records[0].intersectionRatio).to.be(0.5); expect(records[1].target).to.be(targetEl2); expect(records[1].intersectionRatio).to.be(0.5); - expect(records[2].target).to.be(targetEl4); - expect(records[2].intersectionRatio).to.be(0.25); + expect(records[3].target).to.be(targetEl4); + expect(records[3].intersectionRatio).to.be(0.25); io.disconnect(); done(); }, {root: rootEl, rootMargin: '5% -2.5% -10px -190px'}); @@ -705,9 +705,9 @@ limitations under the License. spy.waitForNotification(function() { expect(spy.callCount).to.be(1); var records = sortRecords(spy.lastCall.args[0]); - expect(records.length).to.be(1); - expect(records[0].intersectionRatio).to.be(0); - expect(records[0].target).to.be(targetEl2); + expect(records.length).to.be(2); + expect(records[1].intersectionRatio).to.be(0); + expect(records[1].target).to.be(targetEl2); done(); }, ASYNC_TIMEOUT); }, @@ -797,14 +797,14 @@ limitations under the License. function(done) { document.getElementById('fixtures').appendChild(rootEl); callDelayed(function() { - expect(spy.callCount).to.be(0); + expect(spy.callCount).to.be(1); done(); }, ASYNC_TIMEOUT); }, function(done) { parentEl.insertBefore(targetEl1, targetEl2); spy.waitForNotification(function() { - expect(spy.callCount).to.be(1); + expect(spy.callCount).to.be(2); var records = sortRecords(spy.lastCall.args[0]); expect(records.length).to.be(1); expect(records[0].intersectionRatio).to.be(1); @@ -815,7 +815,7 @@ limitations under the License. function(done) { grandParentEl.parentNode.removeChild(grandParentEl); spy.waitForNotification(function() { - expect(spy.callCount).to.be(2); + expect(spy.callCount).to.be(3); var records = sortRecords(spy.lastCall.args[0]); expect(records.length).to.be(1); expect(records[0].intersectionRatio).to.be(0); @@ -826,7 +826,7 @@ limitations under the License. function(done) { rootEl.appendChild(targetEl1); spy.waitForNotification(function() { - expect(spy.callCount).to.be(3); + expect(spy.callCount).to.be(4); var records = sortRecords(spy.lastCall.args[0]); expect(records.length).to.be(1); expect(records[0].intersectionRatio).to.be(1); @@ -837,7 +837,7 @@ limitations under the License. function(done) { rootEl.parentNode.removeChild(rootEl); spy.waitForNotification(function() { - expect(spy.callCount).to.be(4); + expect(spy.callCount).to.be(5); var records = sortRecords(spy.lastCall.args[0]); expect(records.length).to.be(1); expect(records[0].intersectionRatio).to.be(0); @@ -867,8 +867,14 @@ limitations under the License. targetEl1.style.top = '220px'; targetEl1.style.left = '220px'; + + var callCount = 0; io = new IntersectionObserver(function(records) { + callCount++; + if (callCount <= 1) { + return; + } expect(records.length).to.be(1); expect(records[0].intersectionRatio).to.be(1); done(); @@ -891,6 +897,19 @@ limitations under the License. var win = window.open("intersectionobserver_window.html"); }); + it('triggers only once if observed multiple times (and does not crash when collected)', function(done) { + var spy = sinon.spy(); + io = new IntersectionObserver(spy, {root: rootEl}); + io.observe(targetEl1); + io.observe(targetEl1); + io.observe(targetEl1); + + callDelayed(function () { + expect(spy.callCount).to.be(1); + done(); + }, ASYNC_TIMEOUT); + }); + }); describe('observe subframe', function () { -- cgit v1.2.3