Bug 1385324 - Part 2: Rewrite HTMLFormControlsCollection::GetSortedControls() to use RefPtr instead of raw pointers; r=qdot
authorEhsan Akhgari <ehsan@mozilla.com>
Fri, 28 Jul 2017 11:38:26 -0400
changeset 371857 1ba4cfc4fa6289f4ba92886099c131ed85edd8fd
parent 371856 485ceaeda92239b569b49989845fdee594c3c0a9
child 371858 07320f178c144b1a4be67400e2168e04677edec0
push id47611
push userarchaeopteryx@coole-files.de
push dateSun, 30 Jul 2017 09:20:48 +0000
treeherderautoland@8b577b152383 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersqdot
bugs1385324
milestone56.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 1385324 - Part 2: Rewrite HTMLFormControlsCollection::GetSortedControls() to use RefPtr instead of raw pointers; r=qdot
dom/html/HTMLFormControlsCollection.cpp
dom/html/HTMLFormControlsCollection.h
dom/html/HTMLFormElement.cpp
dom/html/HTMLFormElement.h
--- a/dom/html/HTMLFormControlsCollection.cpp
+++ b/dom/html/HTMLFormControlsCollection.cpp
@@ -256,17 +256,17 @@ HTMLFormControlsCollection::RemoveElemen
     return NS_OK;
   }
 
   return mForm->RemoveElementFromTableInternal(mNameLookupTable, aChild, aName);
 }
 
 nsresult
 HTMLFormControlsCollection::GetSortedControls(
-  nsTArray<nsGenericHTMLFormElement*>& aControls) const
+  nsTArray<RefPtr<nsGenericHTMLFormElement>>& aControls) const
 {
 #ifdef DEBUG
   HTMLFormElement::AssertDocumentOrder(mElements, mForm);
   HTMLFormElement::AssertDocumentOrder(mNotInElements, mForm);
 #endif
 
   aControls.Clear();
 
--- a/dom/html/HTMLFormControlsCollection.h
+++ b/dom/html/HTMLFormControlsCollection.h
@@ -10,16 +10,18 @@
 #include "mozilla/dom/Element.h" // DOMProxyHandler::getOwnPropertyDescriptor
 #include "nsIHTMLCollection.h"
 #include "nsInterfaceHashtable.h"
 #include "nsTArray.h"
 #include "nsWrapperCache.h"
 
 class nsGenericHTMLFormElement;
 class nsIFormControl;
+template <class T>
+class RefPtr;
 
 namespace mozilla {
 namespace dom {
 class HTMLFormElement;
 class HTMLImageElement;
 class OwningRadioNodeListOrElement;
 template<typename> struct Nullable;
 
@@ -70,17 +72,17 @@ public:
    * Create a sorted list of form control elements. This list is sorted
    * in document order and contains the controls in the mElements and
    * mNotInElements list. This function does not add references to the
    * elements.
    *
    * @param aControls The list of sorted controls[out].
    * @return NS_OK or NS_ERROR_OUT_OF_MEMORY.
    */
-  nsresult GetSortedControls(nsTArray<nsGenericHTMLFormElement*>& aControls) const;
+  nsresult GetSortedControls(nsTArray<RefPtr<nsGenericHTMLFormElement>>& aControls) const;
 
   // nsWrapperCache
   using nsWrapperCache::GetWrapperPreserveColor;
   using nsWrapperCache::PreserveWrapper;
   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 protected:
   virtual ~HTMLFormControlsCollection();
   virtual JSObject* GetWrapperPreserveColorInternal() override
--- a/dom/html/HTMLFormElement.cpp
+++ b/dom/html/HTMLFormElement.cpp
@@ -1018,41 +1018,30 @@ HTMLFormElement::NotifySubmitObservers(n
 }
 
 
 nsresult
 HTMLFormElement::WalkFormElements(HTMLFormSubmission* aFormSubmission)
 {
   // This shouldn't be called recursively, so use a rather large value
   // for the preallocated buffer.
-  AutoTArray<nsGenericHTMLFormElement*, 100> sortedControls;
+  AutoTArray<RefPtr<nsGenericHTMLFormElement>, 100> sortedControls;
   nsresult rv = mControls->GetSortedControls(sortedControls);
   NS_ENSURE_SUCCESS(rv, rv);
 
   uint32_t len = sortedControls.Length();
 
-  // Hold a reference to the elements so they can't be deleted while
-  // calling SubmitNamesValues().
-  for (uint32_t i = 0; i < len; ++i) {
-    static_cast<nsGenericHTMLElement*>(sortedControls[i])->AddRef();
-  }
-
   //
   // Walk the list of nodes and call SubmitNamesValues() on the controls
   //
   for (uint32_t i = 0; i < len; ++i) {
     // Tell the control to submit its name/value pairs to the submission
     sortedControls[i]->SubmitNamesValues(aFormSubmission);
   }
 
-  // Release the references.
-  for (uint32_t i = 0; i < len; ++i) {
-    static_cast<nsGenericHTMLElement*>(sortedControls[i])->Release();
-  }
-
   return NS_OK;
 }
 
 // nsIForm
 
 NS_IMETHODIMP_(uint32_t)
 HTMLFormElement::GetElementCount() const
 {
@@ -1136,16 +1125,42 @@ HTMLFormElement::AssertDocumentOrder(
   if (!aControls.IsEmpty()) {
     for (uint32_t i = 0; i < aControls.Length() - 1; ++i) {
       NS_ASSERTION(CompareFormControlPosition(aControls[i], aControls[i + 1],
                                               aForm) < 0,
                    "Form controls not ordered correctly");
     }
   }
 }
+
+/**
+ * Copy of the above function, but with RefPtrs.
+ *
+ * @param aControls List of form controls to check.
+ * @param aForm Parent form of the controls.
+ */
+/* static */ void
+HTMLFormElement::AssertDocumentOrder(
+  const nsTArray<RefPtr<nsGenericHTMLFormElement>>& aControls, nsIContent* aForm)
+{
+  // TODO: remove the return statement with bug 598468.
+  // This is done to prevent asserts in some edge cases.
+  return;
+
+  // Only iterate if aControls is not empty, since otherwise
+  // |aControls.Length() - 1| will be a very large unsigned number... not what
+  // we want here.
+  if (!aControls.IsEmpty()) {
+    for (uint32_t i = 0; i < aControls.Length() - 1; ++i) {
+      NS_ASSERTION(CompareFormControlPosition(aControls[i], aControls[i + 1],
+                                              aForm) < 0,
+                   "Form controls not ordered correctly");
+    }
+  }
+}
 #endif
 
 void
 HTMLFormElement::PostPasswordEvent()
 {
   // Don't fire another add event if we have a pending add event.
   if (mFormPasswordEventDispatcher.get()) {
     return;
@@ -1867,29 +1882,23 @@ HTMLFormElement::ForgetCurrentSubmission
 
 bool
 HTMLFormElement::CheckFormValidity(nsIMutableArray* aInvalidElements) const
 {
   bool ret = true;
 
   // This shouldn't be called recursively, so use a rather large value
   // for the preallocated buffer.
-  AutoTArray<nsGenericHTMLFormElement*, 100> sortedControls;
+  AutoTArray<RefPtr<nsGenericHTMLFormElement>, 100> sortedControls;
   if (NS_FAILED(mControls->GetSortedControls(sortedControls))) {
     return false;
   }
 
   uint32_t len = sortedControls.Length();
 
-  // Hold a reference to the elements so they can't be deleted while calling
-  // the invalid events.
-  for (uint32_t i = 0; i < len; ++i) {
-    sortedControls[i]->AddRef();
-  }
-
   for (uint32_t i = 0; i < len; ++i) {
     nsCOMPtr<nsIConstraintValidation> cvElmt = do_QueryObject(sortedControls[i]);
     if (cvElmt && cvElmt->IsCandidateForConstraintValidation() &&
         !cvElmt->IsValid()) {
       ret = false;
       bool defaultAction = true;
       nsContentUtils::DispatchTrustedEvent(sortedControls[i]->OwnerDoc(),
                                            static_cast<nsIContent*>(sortedControls[i]),
@@ -1900,21 +1909,16 @@ HTMLFormElement::CheckFormValidity(nsIMu
       // requested them.
       if (defaultAction && aInvalidElements) {
         aInvalidElements->AppendElement(ToSupports(sortedControls[i]),
                                         false);
       }
     }
   }
 
-  // Release the references.
-  for (uint32_t i = 0; i < len; ++i) {
-    static_cast<nsGenericHTMLElement*>(sortedControls[i])->Release();
-  }
-
   return ret;
 }
 
 bool
 HTMLFormElement::CheckValidFormSubmission()
 {
   /**
    * Check for form validity: do not submit a form if there are unhandled
--- a/dom/html/HTMLFormElement.h
+++ b/dom/html/HTMLFormElement.h
@@ -405,16 +405,19 @@ public:
 
   static int32_t
   CompareFormControlPosition(Element* aElement1, Element* aElement2,
                              const nsIContent* aForm);
 #ifdef DEBUG
   static void
   AssertDocumentOrder(const nsTArray<nsGenericHTMLFormElement*>& aControls,
                       nsIContent* aForm);
+  static void
+  AssertDocumentOrder(const nsTArray<RefPtr<nsGenericHTMLFormElement>>& aControls,
+                      nsIContent* aForm);
 #endif
 
   js::ExpandoAndGeneration mExpandoAndGeneration;
 
 protected:
   virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 
   void PostPasswordEvent();