Bug 1322717 - Disconnect/Unlink in the proper order to avoid crashes in mozilla::dom::DOMIntersectionObserver::UnlinkTarget. r=mrbkap
authorTobias Schneider <schneider@jancona.com>
Thu, 08 Dec 2016 18:54:07 -0800
changeset 325606 cfc91cb39848376e1a47a16006b0d84028232158
parent 325605 53cf106dd90cfcfd2b920571e789ffbeee55b651
child 325607 97ec33ac932242b2bfcadab32a98a743183ebcea
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersmrbkap
bugs1322717
milestone53.0a1
Bug 1322717 - Disconnect/Unlink in the proper order to avoid crashes in mozilla::dom::DOMIntersectionObserver::UnlinkTarget. r=mrbkap
dom/base/DOMIntersectionObserver.cpp
dom/base/DOMIntersectionObserver.h
dom/base/Element.cpp
dom/base/Element.h
dom/base/FragmentOrElement.h
dom/base/nsNodeUtils.cpp
dom/base/test/test_intersectionobservers.html
--- a/dom/base/DOMIntersectionObserver.cpp
+++ b/dom/base/DOMIntersectionObserver.cpp
@@ -42,16 +42,17 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(DOM
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMIntersectionObserver)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mOwner)
   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)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOwner)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCallback)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRoot)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mQueuedEntries)
@@ -180,19 +181,20 @@ DOMIntersectionObserver::UnlinkTarget(El
 }
 
 void
 DOMIntersectionObserver::Connect()
 {
   if (mConnected) {
     return;
   }
+
+  mConnected = true;
   nsIDocument* document = mOwner->GetExtantDoc();
   document->AddIntersectionObserver(this);
-  mConnected = true;
 }
 
 void
 DOMIntersectionObserver::Disconnect()
 {
   if (!mConnected) {
     return;
   }
--- a/dom/base/DOMIntersectionObserver.h
+++ b/dom/base/DOMIntersectionObserver.h
@@ -96,19 +96,17 @@ protected:
 
 #define NS_DOM_INTERSECTION_OBSERVER_IID \
 { 0x8570a575, 0xe303, 0x4d18, \
   { 0xb6, 0xb1, 0x4d, 0x2b, 0x49, 0xd8, 0xef, 0x94 } }
 
 class DOMIntersectionObserver final : public nsISupports,
                                       public nsWrapperCache
 {
-  virtual ~DOMIntersectionObserver() {
-    Disconnect();
-  }
+  virtual ~DOMIntersectionObserver() { }
 
 public:
   DOMIntersectionObserver(already_AddRefed<nsPIDOMWindowInner>&& aOwner,
                           mozilla::dom::IntersectionCallback& aCb)
   : mOwner(aOwner), mCallback(&aCb), mConnected(false)
   {
   }
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -3864,54 +3864,54 @@ Element::ClearDataset()
 {
   nsDOMSlots *slots = GetExistingDOMSlots();
 
   MOZ_ASSERT(slots && slots->mDataset,
              "Slots should exist and dataset should not be null.");
   slots->mDataset = nullptr;
 }
 
-nsTArray<Element::nsDOMSlots::IntersectionObserverRegistration>*
+nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, int32_t>*
 Element::RegisteredIntersectionObservers()
 {
   nsDOMSlots* slots = DOMSlots();
   return &slots->mRegisteredIntersectionObservers;
 }
 
 void
 Element::RegisterIntersectionObserver(DOMIntersectionObserver* aObserver)
 {
-  RegisteredIntersectionObservers()->AppendElement(
-    nsDOMSlots::IntersectionObserverRegistration { aObserver, -1 });
+  nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, int32_t>* observers =
+    RegisteredIntersectionObservers();
+  if (observers->Contains(aObserver)) {
+    return;
+  }
+  RegisteredIntersectionObservers()->Put(aObserver, -1);
 }
 
 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;
 }
 
 void
 Element::ClearServoData() {
 #ifdef MOZ_STYLO
   Servo_Element_ClearData(this);
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -1418,17 +1418,17 @@ protected:
    * the value of xlink:show, converted to a suitably equivalent named target
    * (e.g. _blank).
    */
   virtual void GetLinkTarget(nsAString& aTarget);
 
   nsDOMTokenList* GetTokenList(nsIAtom* aAtom,
                                const DOMTokenListSupportedTokenArray aSupportedTokens = nullptr);
 
-  nsTArray<nsDOMSlots::IntersectionObserverRegistration>* RegisteredIntersectionObservers();
+  nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, int32_t>* RegisteredIntersectionObservers();
 
 private:
   /**
    * Get this element's client area rect in app units.
    * @return the frame's client area
    */
   nsRect GetClientAreaRect();
 
--- a/dom/base/FragmentOrElement.h
+++ b/dom/base/FragmentOrElement.h
@@ -16,16 +16,17 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/MemoryReporting.h"
 #include "nsAttrAndChildArray.h"          // member
 #include "nsCycleCollectionParticipant.h" // NS_DECL_CYCLE_*
 #include "nsIContent.h"                   // base class
 #include "nsIWeakReference.h"             // base class
 #include "nsNodeUtils.h"                  // class member nsNodeUtils::CloneNodeImpl
 #include "nsIHTMLCollection.h"
+#include "nsDataHashtable.h"
 
 class ContentUnbinder;
 class nsContentList;
 class nsDOMAttributeMap;
 class nsDOMTokenList;
 class nsIControllers;
 class nsICSSDeclaration;
 class nsIDocument;
@@ -342,22 +343,17 @@ public:
     /**
      * Web components custom element data.
      */
     RefPtr<CustomElementData> mCustomElementData;
 
     /**
      * Registered Intersection Observers on the element.
      */
-    struct IntersectionObserverRegistration {
-      DOMIntersectionObserver* observer;
-      int32_t previousThreshold;
-    };
-
-    nsTArray<IntersectionObserverRegistration> mRegisteredIntersectionObservers;
+    nsDataHashtable<nsPtrHashKey<DOMIntersectionObserver>, int32_t> mRegisteredIntersectionObservers;
   };
 
 protected:
   void GetMarkup(bool aIncludeSelf, nsAString& aMarkup);
   void SetInnerHTMLInternal(const nsAString& aInnerHTML, ErrorResult& aError);
 
   // Override from nsINode
   virtual nsINode::nsSlots* CreateSlots() override;
--- a/dom/base/nsNodeUtils.cpp
+++ b/dom/base/nsNodeUtils.cpp
@@ -296,18 +296,19 @@ nsNodeUtils::LastRelease(nsINode* aNode)
                                          nsIMutationObserver,
                                          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);
+      for (auto iter = domSlots->mRegisteredIntersectionObservers.Iter(); !iter.Done(); iter.Next()) {
+        DOMIntersectionObserver* observer = iter.Key();
+        observer->UnlinkTarget(*elem);
       }
     }
 
     delete slots;
     aNode->mSlots = nullptr;
   }
 
   // Kill properties first since that may run external code, so we want to
--- a/dom/base/test/test_intersectionobservers.html
+++ b/dom/base/test/test_intersectionobservers.html
@@ -886,16 +886,29 @@ limitations under the License.
           expect(e.data).to.be.ok();
           win.close();
           done();
         };
 
         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 () {
       
       it('should not trigger if target and root are not in the same document',
           function(done) {
 
         var spy = sinon.spy();