Stabilize and align Intersection Observers

- Fixes several crashes
- Aligns the feature with the W3C WD spec

Tag #249

9 files changed, 111 insertions, 64 deletions

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<nsPIDOMWindowInner>&& 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<Element::nsDOMSlots::IntersectionObserverRegistration>*
+nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, 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<nsPtrHashKey<DOMIntersectionObserver>, 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<nsDOMSlots::IntersectionObserverRegistration>* observers =
+  nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, 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<nsDOMSlots::IntersectionObserverRegistration>* observers =
+  nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, 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<nsDOMSlots::IntersectionObserverRegistration>* RegisteredIntersectionObservers();
+  nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, 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<IntersectionObserverRegistration> mRegisteredIntersectionObservers;
+    nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, 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<RefPtr<DOMIntersectionObserver>> 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<FragmentOrElement::nsDOMSlots*>(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 () {