Bug 1376695 - Part 2: Avoid rebuilding the list returned by getElementsByName when not needed. r=ehsan
authorJessica Jong <jjong@mozilla.com>
Wed, 26 Jul 2017 02:34:00 -0400
changeset 371291 753fd959084178188123c48f52f7f4196a387108
parent 371290 7eb9728ad2a1b4af4cd7e7a78f676cb0f7333c89
child 371292 000f28217a30b8cf649573ad2f52a03a8b426ef7
push id32241
push usercbook@mozilla.com
push dateThu, 27 Jul 2017 08:57:54 +0000
treeherdermozilla-central@e5693cea1ec9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1376695
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 1376695 - Part 2: Avoid rebuilding the list returned by getElementsByName when not needed. r=ehsan The list returned by getElementsByName is cached, however, it's rebuilt frequently due to content mutations. We can avoid rebuilding the list when the attribute changed is not the name attribute. We can also speed up the name matching process by skipping the elements that do not have a name attribute. MozReview-Commit-ID: 9TUPaQonjHz
dom/base/nsContentList.cpp
dom/base/nsContentList.h
dom/base/nsContentListDeclarations.h
dom/base/nsContentUtils.h
dom/html/nsHTMLDocument.cpp
dom/html/nsHTMLDocument.h
dom/html/test/mochitest.ini
dom/html/test/test_getElementsByName_after_mutation.html
--- a/dom/base/nsContentList.cpp
+++ b/dom/base/nsContentList.cpp
@@ -244,17 +244,17 @@ NS_GetContentList(nsINode* aRootNode,
   }
 
   sRecentlyUsedContentLists[recentlyUsedCacheIndex] = list;
   return list.forget();
 }
 
 #ifdef DEBUG
 const nsCacheableFuncStringContentList::ContentListType
-  nsCacheableFuncStringNodeList::sType = nsCacheableFuncStringContentList::eNodeList;
+  nsCachableElementsByNameNodeList::sType = nsCacheableFuncStringContentList::eNodeList;
 const nsCacheableFuncStringContentList::ContentListType
   nsCacheableFuncStringHTMLCollection::sType = nsCacheableFuncStringContentList::eHTMLCollection;
 #endif
 
 // Hashtable for storing nsCacheableFuncStringContentList
 static PLDHashTable* gFuncStringContentListHashTable;
 
 struct FuncStringContentListHashEntry : public PLDHashEntryHdr
@@ -333,43 +333,31 @@ GetFuncStringContentList(nsINode* aRootN
     }
   }
 
   // Don't cache these lists globally
 
   return list.forget();
 }
 
+// Explicit instantiations to avoid link errors
+template
 already_AddRefed<nsContentList>
-NS_GetFuncStringNodeList(nsINode* aRootNode,
-                         nsContentListMatchFunc aFunc,
-                         nsContentListDestroyFunc aDestroyFunc,
-                         nsFuncStringContentListDataAllocator aDataAllocator,
-                         const nsAString& aString)
-{
-  return GetFuncStringContentList<nsCacheableFuncStringNodeList>(aRootNode,
-                                                                 aFunc,
-                                                                 aDestroyFunc,
-                                                                 aDataAllocator,
-                                                                 aString);
-}
-
+GetFuncStringContentList<nsCachableElementsByNameNodeList>(nsINode* aRootNode,
+                                                           nsContentListMatchFunc aFunc,
+                                                           nsContentListDestroyFunc aDestroyFunc,
+                                                           nsFuncStringContentListDataAllocator aDataAllocator,
+                                                           const nsAString& aString);
+template
 already_AddRefed<nsContentList>
-NS_GetFuncStringHTMLCollection(nsINode* aRootNode,
-                               nsContentListMatchFunc aFunc,
-                               nsContentListDestroyFunc aDestroyFunc,
-                               nsFuncStringContentListDataAllocator aDataAllocator,
-                               const nsAString& aString)
-{
-  return GetFuncStringContentList<nsCacheableFuncStringHTMLCollection>(aRootNode,
-                                                                       aFunc,
-                                                                       aDestroyFunc,
-                                                                       aDataAllocator,
-                                                                       aString);
-}
+GetFuncStringContentList<nsCacheableFuncStringHTMLCollection>(nsINode* aRootNode,
+                                                              nsContentListMatchFunc aFunc,
+                                                              nsContentListDestroyFunc aDestroyFunc,
+                                                              nsFuncStringContentListDataAllocator aDataAllocator,
+                                                              const nsAString& aString);
 
 //-----------------------------------------------------
 // nsContentList implementation
 
 nsContentList::nsContentList(nsINode* aRootNode,
                              int32_t aMatchNameSpaceId,
                              nsIAtom* aHTMLMatchAtom,
                              nsIAtom* aXMLMatchAtom,
@@ -1071,24 +1059,43 @@ nsContentList::AssertInSync()
     }
   }
 
   NS_ASSERTION(cnt == mElements.Length(), "Too few elements");
 }
 #endif
 
 //-----------------------------------------------------
-// nsCacheableFuncStringNodeList
+// nsCachableElementsByNameNodeList
 
 JSObject*
-nsCacheableFuncStringNodeList::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto)
+nsCachableElementsByNameNodeList::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto)
 {
   return NodeListBinding::Wrap(cx, this, aGivenProto);
 }
 
+void
+nsCachableElementsByNameNodeList::AttributeChanged(nsIDocument* aDocument,
+                                                   Element* aElement,
+                                                   int32_t aNameSpaceID,
+                                                   nsIAtom* aAttribute,
+                                                   int32_t aModType,
+                                                   const nsAttrValue* aOldValue)
+{
+  // No need to rebuild the list if the changed attribute is not the name
+  // attribute.
+  if (aAttribute != nsGkAtoms::name) {
+    return;
+  }
+
+  nsCacheableFuncStringContentList::AttributeChanged(aDocument, aElement,
+                                                     aNameSpaceID, aAttribute,
+                                                     aModType, aOldValue);
+}
+
 //-----------------------------------------------------
 // nsCacheableFuncStringHTMLCollection
 
 JSObject*
 nsCacheableFuncStringHTMLCollection::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto)
 {
   return HTMLCollectionBinding::Wrap(cx, this, aGivenProto);
 }
--- a/dom/base/nsContentList.h
+++ b/dom/base/nsContentList.h
@@ -541,33 +541,35 @@ protected:
   virtual void RemoveFromCaches() override {
     RemoveFromFuncStringHashtable();
   }
   void RemoveFromFuncStringHashtable();
 
   nsString mString;
 };
 
-class nsCacheableFuncStringNodeList
+class nsCachableElementsByNameNodeList
   : public nsCacheableFuncStringContentList
 {
 public:
-  nsCacheableFuncStringNodeList(nsINode* aRootNode,
-                                nsContentListMatchFunc aFunc,
-                                nsContentListDestroyFunc aDestroyFunc,
-                                nsFuncStringContentListDataAllocator aDataAllocator,
-                                const nsAString& aString)
+  nsCachableElementsByNameNodeList(nsINode* aRootNode,
+                                   nsContentListMatchFunc aFunc,
+                                   nsContentListDestroyFunc aDestroyFunc,
+                                   nsFuncStringContentListDataAllocator aDataAllocator,
+                                   const nsAString& aString)
     : nsCacheableFuncStringContentList(aRootNode, aFunc, aDestroyFunc,
                                        aDataAllocator, aString)
   {
 #ifdef DEBUG
     mType = eNodeList;
 #endif
   }
 
+  NS_DECL_NSIMUTATIONOBSERVER_ATTRIBUTECHANGED
+
   virtual JSObject* WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto) override;
 
 #ifdef DEBUG
   static const ContentListType sType;
 #endif
 };
 
 class nsCacheableFuncStringHTMLCollection
--- a/dom/base/nsContentListDeclarations.h
+++ b/dom/base/nsContentListDeclarations.h
@@ -52,22 +52,17 @@ typedef void* (*nsFuncStringContentListD
 // elements in HTML documents and aTagname against everything else.
 // For any other value of aMatchNameSpaceId, the list will match
 // aTagname against all elements.
 already_AddRefed<nsContentList>
 NS_GetContentList(nsINode* aRootNode,
                   int32_t aMatchNameSpaceId,
                   const nsAString& aTagname);
 
+template<class ListType>
 already_AddRefed<nsContentList>
-NS_GetFuncStringNodeList(nsINode* aRootNode,
+GetFuncStringContentList(nsINode* aRootNode,
                          nsContentListMatchFunc aFunc,
                          nsContentListDestroyFunc aDestroyFunc,
                          nsFuncStringContentListDataAllocator aDataAllocator,
                          const nsAString& aString);
-already_AddRefed<nsContentList>
-NS_GetFuncStringHTMLCollection(nsINode* aRootNode,
-                               nsContentListMatchFunc aFunc,
-                               nsContentListDestroyFunc aDestroyFunc,
-                               nsFuncStringContentListDataAllocator aDataAllocator,
-                               const nsAString& aString);
 
 #endif // nsContentListDeclarations_h
--- a/dom/base/nsContentUtils.h
+++ b/dom/base/nsContentUtils.h
@@ -50,16 +50,17 @@
 
 class imgICache;
 class imgIContainer;
 class imgINotificationObserver;
 class imgIRequest;
 class imgLoader;
 class imgRequestProxy;
 class nsAutoScriptBlockerSuppressNodeRemoved;
+class nsCacheableFuncStringHTMLCollection;
 class nsHtml5StringParser;
 class nsIChannel;
 class nsIConsoleService;
 class nsIContent;
 class nsIContentPolicy;
 class nsIContentSecurityPolicy;
 class nsIDocShellTreeItem;
 class nsIDocumentLoaderFactory;
@@ -2078,20 +2079,21 @@ public:
    * Utility method for getElementsByClassName.  aRootNode is the node (either
    * document or element), which getElementsByClassName was called on.
    */
   static already_AddRefed<nsContentList>
   GetElementsByClassName(nsINode* aRootNode, const nsAString& aClasses)
   {
     NS_PRECONDITION(aRootNode, "Must have root node");
 
-    return NS_GetFuncStringHTMLCollection(aRootNode, MatchClassNames,
-                                          DestroyClassNameArray,
-                                          AllocClassMatchingInfo,
-                                          aClasses);
+    return GetFuncStringContentList<nsCacheableFuncStringHTMLCollection>(aRootNode,
+                                                                         MatchClassNames,
+                                                                         DestroyClassNameArray,
+                                                                         AllocClassMatchingInfo,
+                                                                         aClasses);
   }
 
   /**
    * Returns a presshell for this document, if there is one. This will be
    * aDoc's direct presshell if there is one, otherwise we'll look at all
    * ancestor documents to try to find a presshell, so for example this can
    * still find a presshell for documents in display:none frames that have
    * no presentation. So you have to be careful how you use this presshell ---
--- a/dom/html/nsHTMLDocument.cpp
+++ b/dom/html/nsHTMLDocument.cpp
@@ -2033,16 +2033,21 @@ nsHTMLDocument::Writeln(JSContext* cx, c
   WriteCommon(cx, aText, true, rv);
 }
 
 bool
 nsHTMLDocument::MatchNameAttribute(Element* aElement, int32_t aNamespaceID,
                                    nsIAtom* aAtom, void* aData)
 {
   NS_PRECONDITION(aElement, "Must have element to work with!");
+
+  if (!aElement->HasName()) {
+    return false;
+  }
+
   nsString* elementName = static_cast<nsString*>(aData);
   return
     aElement->GetNameSpaceID() == kNameSpaceID_XHTML &&
     aElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
                           *elementName, eCaseMatters);
 }
 
 /* static */
--- a/dom/html/nsHTMLDocument.h
+++ b/dom/html/nsHTMLDocument.h
@@ -188,18 +188,21 @@ public:
   nsIHTMLCollection* Links();
   nsIHTMLCollection* Forms()
   {
     return nsHTMLDocument::GetForms();
   }
   nsIHTMLCollection* Scripts();
   already_AddRefed<nsContentList> GetElementsByName(const nsAString & aName)
   {
-    return NS_GetFuncStringNodeList(this, MatchNameAttribute, nullptr,
-                                    UseExistingNameString, aName);
+    return GetFuncStringContentList<nsCachableElementsByNameNodeList>(this,
+                                                                      MatchNameAttribute,
+                                                                      nullptr,
+                                                                      UseExistingNameString,
+                                                                      aName);
   }
   already_AddRefed<nsIDocument> Open(JSContext* cx,
                                      const nsAString& aType,
                                      const nsAString& aReplace,
                                      mozilla::ErrorResult& rv);
   already_AddRefed<nsPIDOMWindowOuter>
   Open(JSContext* cx,
        const nsAString& aURL,
--- a/dom/html/test/mochitest.ini
+++ b/dom/html/test/mochitest.ini
@@ -606,8 +606,9 @@ skip-if = os == "android" # up/down arro
 [test_bug1295719_event_sequence_for_number_keys.html]
 [test_bug1310865.html]
 [test_bug1315146.html]
 [test_fakepath.html]
 [test_script_module.html]
 support-files =
   file_script_module.html
   file_script_nomodule.html
+[test_getElementsByName_after_mutation.html]
new file mode 100644
--- /dev/null
+++ b/dom/html/test/test_getElementsByName_after_mutation.html
@@ -0,0 +1,51 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1376695
+-->
+<head>
+  <title>Test for Bug 1376695</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1376695">Mozilla Bug 1376695</a>
+<p id="display"></p>
+<div id="originalFoo" name="foo">
+<pre id="test">
+<script type="application/javascript">
+
+/** Test to ensure that the list returned by getElementsByName is updated after
+  * mutations.
+  **/
+
+var fooList = document.getElementsByName("foo");
+var originalDiv = document.getElementById("originalFoo");
+
+is(fooList.length, 1, "Should find one element with name 'foo' initially");
+is(fooList[0], originalDiv, "Element should be the original div");
+
+var newTree = document.createElement("p");
+var child1 = document.createElement("div");
+var child2 = document.createElement("div");
+child2.setAttribute("name", "foo");
+
+newTree.appendChild(child1);
+newTree.appendChild(child2);
+document.body.appendChild(newTree);
+
+is(fooList.length, 2,
+   "Should find two elements with name 'foo' after appending the new tree");
+is(fooList[1], child2, "Element should be the new appended div with name 'foo'");
+
+document.body.removeChild(newTree);
+
+is(fooList.length, 1,
+   "Should find one element with name 'foo' after removing the new tree");
+is(fooList[0], originalDiv,
+   "Element should be the original div after removing the new tree");
+
+</script>
+</pre>
+</body>
+</html>