Bug 1499861 - Make HTMLOptionsCollection::mSelect into a strong reference. r=qDot
authorAndrew McCreight <continuation@gmail.com>
Wed, 07 Nov 2018 08:38:42 -0500
changeset 444901 d4f3e119ae841008c1be59e72ee0a058e3803cf3
parent 444849 9163e0c24c4ed02f5478b73bec19a86d198e7c8f
child 444902 d5879bff1a027d610a506e113d41eacce7cd8f6c
push id35006
push useraiakab@mozilla.com
push dateWed, 07 Nov 2018 21:51:52 +0000
treeherdermozilla-central@5836a6061476 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersqDot
bugs1499861
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1499861 - Make HTMLOptionsCollection::mSelect into a strong reference. r=qDot The cycle collector makes weak references like this obsolete.
dom/html/HTMLOptionsCollection.cpp
dom/html/HTMLOptionsCollection.h
dom/html/HTMLSelectElement.cpp
dom/html/HTMLSelectElement.h
--- a/dom/html/HTMLOptionsCollection.cpp
+++ b/dom/html/HTMLOptionsCollection.cpp
@@ -27,33 +27,18 @@
 #include "nsServiceManagerUtils.h"
 #include "nsStyleConsts.h"
 #include "jsfriendapi.h"
 
 namespace mozilla {
 namespace dom {
 
 HTMLOptionsCollection::HTMLOptionsCollection(HTMLSelectElement* aSelect)
-{
-  // Do not maintain a reference counted reference. When
-  // the select goes away, it will let us know.
-  mSelect = aSelect;
-}
-
-HTMLOptionsCollection::~HTMLOptionsCollection()
-{
-  DropReference();
-}
-
-void
-HTMLOptionsCollection::DropReference()
-{
-  // Drop our (non ref-counted) reference
-  mSelect = nullptr;
-}
+  : mSelect(aSelect)
+{}
 
 nsresult
 HTMLOptionsCollection::GetOptionIndex(Element* aOption,
                                       int32_t aStartIndex,
                                       bool aForward,
                                       int32_t* aIndex)
 {
   // NOTE: aIndex shouldn't be set if the returned value isn't NS_OK.
@@ -80,17 +65,19 @@ HTMLOptionsCollection::GetOptionIndex(El
       return NS_OK;
     }
   }
 
   return NS_ERROR_FAILURE;
 }
 
 
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(HTMLOptionsCollection, mElements)
+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(HTMLOptionsCollection,
+                                      mElements,
+                                      mSelect)
 
 // nsISupports
 
 // QueryInterface implementation for HTMLOptionsCollection
 NS_INTERFACE_TABLE_HEAD(HTMLOptionsCollection)
   NS_WRAPPERCACHE_INTERFACE_TABLE_ENTRY
   NS_INTERFACE_TABLE(HTMLOptionsCollection,
                      nsIHTMLCollection)
@@ -110,33 +97,24 @@ uint32_t
 HTMLOptionsCollection::Length()
 {
   return mElements.Length();
 }
 
 void
 HTMLOptionsCollection::SetLength(uint32_t aLength, ErrorResult& aError)
 {
-  if (!mSelect) {
-    aError.Throw(NS_ERROR_UNEXPECTED);
-    return;
-  }
-
   mSelect->SetLength(aLength, aError);
 }
 
 void
 HTMLOptionsCollection::IndexedSetter(uint32_t aIndex,
                                      HTMLOptionElement* aOption,
                                      ErrorResult& aError)
 {
-  if (!mSelect) {
-    return;
-  }
-
   // if the new option is null, just remove this option.  Note that it's safe
   // to pass a too-large aIndex in here.
   if (!aOption) {
     mSelect->Remove(aIndex);
 
     // We're done.
     return;
   }
@@ -170,33 +148,23 @@ HTMLOptionsCollection::IndexedSetter(uin
   }
 
   parent->ReplaceChild(*aOption, *refChild, aError);
 }
 
 int32_t
 HTMLOptionsCollection::GetSelectedIndex(ErrorResult& aError)
 {
-  if (!mSelect) {
-    aError.Throw(NS_ERROR_UNEXPECTED);
-    return 0;
-  }
-
   return mSelect->SelectedIndex();
 }
 
 void
 HTMLOptionsCollection::SetSelectedIndex(int32_t aSelectedIndex,
                                         ErrorResult& aError)
 {
-  if (!mSelect) {
-    aError.Throw(NS_ERROR_UNEXPECTED);
-    return;
-  }
-
   mSelect->SetSelectedIndex(aSelectedIndex, aError);
 }
 
 Element*
 HTMLOptionsCollection::GetElementAt(uint32_t aIndex)
 {
   return ItemAsOption(aIndex);
 }
@@ -265,28 +233,19 @@ HTMLOptionsCollection::GetSupportedNames
   }
 }
 
 void
 HTMLOptionsCollection::Add(const HTMLOptionOrOptGroupElement& aElement,
                            const Nullable<HTMLElementOrLong>& aBefore,
                            ErrorResult& aError)
 {
-  if (!mSelect) {
-    aError.Throw(NS_ERROR_NOT_INITIALIZED);
-    return;
-  }
-
   mSelect->Add(aElement, aBefore, aError);
 }
 
 void
 HTMLOptionsCollection::Remove(int32_t aIndex, ErrorResult& aError)
 {
-  if (!mSelect) {
-    aError.Throw(NS_ERROR_UNEXPECTED);
-    return;
-  }
   mSelect->Remove(aIndex);
 }
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/html/HTMLOptionsCollection.h
+++ b/dom/html/HTMLOptionsCollection.h
@@ -39,17 +39,17 @@ public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
 
   // nsWrapperCache
   using nsWrapperCache::GetWrapperPreserveColor;
   using nsWrapperCache::GetWrapper;
   using nsWrapperCache::PreserveWrapper;
   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 protected:
-  virtual ~HTMLOptionsCollection();
+  virtual ~HTMLOptionsCollection() = default;
 
   virtual JSObject* GetWrapperPreserveColorInternal() override
   {
     return nsWrapperCache::GetWrapperPreserveColor();
   }
   virtual void PreserveWrapperInternal(nsISupports* aScriptObjectHolder) override
   {
     nsWrapperCache::PreserveWrapper(aScriptObjectHolder);
@@ -106,21 +106,16 @@ public:
    * Append an option to end of array
    */
   void AppendOption(mozilla::dom::HTMLOptionElement* aOption)
   {
     mElements.AppendElement(aOption);
   }
 
   /**
-   * Drop the reference to the select.  Called during select destruction.
-   */
-  void DropReference();
-
-  /**
    * Finds the index of a given option element.
    * If the option isn't part of the collection, return NS_ERROR_FAILURE
    * without setting aIndex.
    *
    * @param aOption the option to get the index of
    * @param aStartIndex the index to start looking at
    * @param aForward TRUE to look forward, FALSE to look backward
    * @return the option index
@@ -151,15 +146,15 @@ public:
   virtual void GetSupportedNames(nsTArray<nsString>& aNames) override;
   void SetLength(uint32_t aLength, ErrorResult& aError);
 
 private:
   /** The list of options (holds strong references).  This is infallible, so
    * various members such as InsertOptionAt are also infallible. */
   nsTArray<RefPtr<mozilla::dom::HTMLOptionElement> > mElements;
   /** The select element that contains this array */
-  HTMLSelectElement* mSelect;
+  RefPtr<HTMLSelectElement> mSelect;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_HTMLOptionsCollection_h
--- a/dom/html/HTMLSelectElement.cpp
+++ b/dom/html/HTMLSelectElement.cpp
@@ -137,21 +137,16 @@ HTMLSelectElement::HTMLSelectElement(alr
   // otherwise it is
 
   // Set up our default state: enabled, optional, and valid.
   AddStatesSilently(NS_EVENT_STATE_ENABLED |
                     NS_EVENT_STATE_OPTIONAL |
                     NS_EVENT_STATE_VALID);
 }
 
-HTMLSelectElement::~HTMLSelectElement()
-{
-  mOptions->DropReference();
-}
-
 // ISupports
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(HTMLSelectElement)
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(HTMLSelectElement,
                                                   nsGenericHTMLFormElementWithState)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mValidity)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOptions)
--- a/dom/html/HTMLSelectElement.h
+++ b/dom/html/HTMLSelectElement.h
@@ -386,17 +386,17 @@ public:
 
   void GetPreviewValue(nsAString& aValue)
   {
     aValue = mPreviewValue;
   }
   void SetPreviewValue(const nsAString& aValue);
 
 protected:
-  virtual ~HTMLSelectElement();
+  virtual ~HTMLSelectElement() = default;
 
   friend class SafeOptionListMutation;
 
   // Helper Methods
   /**
    * Check whether the option specified by the index is selected
    * @param aIndex the index
    * @return whether the option at the index is selected