Bug 1449827 - Optimize static atom use in nsTreeSanitizer. r=hsivonen
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 29 Mar 2018 15:51:39 +1100
changeset 411449 db3a89f85df2edd667a9503cc148e4e0721b3482
parent 411448 4e54be07d824fe3c4a257d316ee85eee4f653f83
child 411450 4b1dc5e311b92a54befa20bc4af71668fca6048f
push id101654
push usernnethercote@mozilla.com
push dateTue, 03 Apr 2018 12:20:20 +0000
treeherdermozilla-inbound@db3a89f85df2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershsivonen
bugs1449827
milestone61.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 1449827 - Optimize static atom use in nsTreeSanitizer. r=hsivonen The various AtomsTables in nsTreeSanitizer only contain static atoms. Knowing this, we can optimize things: - They can contain raw nsStaticAtom pointers instead of refcounted nsAtom pointers. - When looking up, we can/must first check if the atom we are looking for is static. If not, we know it can't be in the table. This is done by the new Contains() method. This change also lets us add more `const` to various places that interact with the tables. MozReview-Commit-ID: EFxWN2GU78L
dom/base/nsTreeSanitizer.cpp
dom/base/nsTreeSanitizer.h
--- a/dom/base/nsTreeSanitizer.cpp
+++ b/dom/base/nsTreeSanitizer.cpp
@@ -30,17 +30,17 @@
 #include "nsQueryObject.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 //
 // Thanks to Mark Pilgrim and Sam Ruby for the initial whitelist
 //
-nsStaticAtom* const kElementsHTML[] = {
+const nsStaticAtom* const kElementsHTML[] = {
   nsGkAtoms::a,
   nsGkAtoms::abbr,
   nsGkAtoms::acronym,
   nsGkAtoms::address,
   nsGkAtoms::area,
   nsGkAtoms::article,
   nsGkAtoms::aside,
   nsGkAtoms::audio,
@@ -145,17 +145,17 @@ nsStaticAtom* const kElementsHTML[] = {
   nsGkAtoms::u,
   nsGkAtoms::ul,
   nsGkAtoms::var,
   nsGkAtoms::video,
   nsGkAtoms::wbr,
   nullptr
 };
 
-nsStaticAtom* const kAttributesHTML[] = {
+const nsStaticAtom* const kAttributesHTML[] = {
   nsGkAtoms::abbr,
   nsGkAtoms::accept,
   nsGkAtoms::acceptcharset,
   nsGkAtoms::accesskey,
   nsGkAtoms::action,
   nsGkAtoms::alt,
   nsGkAtoms::as,
   nsGkAtoms::autocomplete,
@@ -253,17 +253,17 @@ nsStaticAtom* const kAttributesHTML[] = 
   nsGkAtoms::type,
   nsGkAtoms::usemap,
   nsGkAtoms::value,
   nsGkAtoms::width,
   nsGkAtoms::wrap,
   nullptr
 };
 
-nsStaticAtom* const kPresAttributesHTML[] = {
+const nsStaticAtom* const kPresAttributesHTML[] = {
   nsGkAtoms::align,
   nsGkAtoms::background,
   nsGkAtoms::bgcolor,
   nsGkAtoms::border,
   nsGkAtoms::cellpadding,
   nsGkAtoms::cellspacing,
   nsGkAtoms::color,
   nsGkAtoms::compact,
@@ -272,27 +272,27 @@ nsStaticAtom* const kPresAttributesHTML[
   nsGkAtoms::noshade,
   nsGkAtoms::pointSize,
   nsGkAtoms::size,
   nsGkAtoms::valign,
   nsGkAtoms::vspace,
   nullptr
 };
 
-nsStaticAtom* const kURLAttributesHTML[] = {
+const nsStaticAtom* const kURLAttributesHTML[] = {
   nsGkAtoms::action,
   nsGkAtoms::href,
   nsGkAtoms::src,
   nsGkAtoms::longdesc,
   nsGkAtoms::cite,
   nsGkAtoms::background,
   nullptr
 };
 
-nsStaticAtom* const kElementsSVG[] = {
+const nsStaticAtom* const kElementsSVG[] = {
   nsGkAtoms::a, // a
   nsGkAtoms::circle, // circle
   nsGkAtoms::clipPath, // clipPath
   nsGkAtoms::colorProfile, // color-profile
   nsGkAtoms::cursor, // cursor
   nsGkAtoms::defs, // defs
   nsGkAtoms::desc, // desc
   nsGkAtoms::ellipse, // ellipse
@@ -362,17 +362,17 @@ nsStaticAtom* const kElementsSVG[] = {
   nsGkAtoms::tref, // tref
   nsGkAtoms::tspan, // tspan
   nsGkAtoms::use, // use
   nsGkAtoms::view, // view
   // vkern
   nullptr
 };
 
-nsStaticAtom* const kAttributesSVG[] = {
+const nsStaticAtom* const kAttributesSVG[] = {
   // accent-height
   nsGkAtoms::accumulate, // accumulate
   nsGkAtoms::additive, // additive
   nsGkAtoms::alignment_baseline, // alignment-baseline
   // alphabetic
   nsGkAtoms::amplitude, // amplitude
   // arabic-form
   // ascent
@@ -595,22 +595,22 @@ nsStaticAtom* const kAttributesSVG[] = {
   nsGkAtoms::y1, // y1
   nsGkAtoms::y2, // y2
   nsGkAtoms::yChannelSelector, // yChannelSelector
   nsGkAtoms::z, // z
   nsGkAtoms::zoomAndPan, // zoomAndPan
   nullptr
 };
 
-nsStaticAtom* const kURLAttributesSVG[] = {
+const nsStaticAtom* const kURLAttributesSVG[] = {
   nsGkAtoms::href,
   nullptr
 };
 
-nsStaticAtom* const kElementsMathML[] = {
+const nsStaticAtom* const kElementsMathML[] = {
    nsGkAtoms::abs_, // abs
    nsGkAtoms::_and, // and
    nsGkAtoms::annotation_, // annotation
    nsGkAtoms::annotation_xml_, // annotation-xml
    nsGkAtoms::apply_, // apply
    nsGkAtoms::approx_, // approx
    nsGkAtoms::arccos_, // arccos
    nsGkAtoms::arccosh_, // arccosh
@@ -799,17 +799,17 @@ nsStaticAtom* const kElementsMathML[] = 
    nsGkAtoms::uplimit_, // uplimit
    nsGkAtoms::variance_, // variance
    nsGkAtoms::vector_, // vector
    nsGkAtoms::vectorproduct_, // vectorproduct
    nsGkAtoms::xor_, // xor
   nullptr
 };
 
-nsStaticAtom* const kAttributesMathML[] = {
+const nsStaticAtom* const kAttributesMathML[] = {
    nsGkAtoms::accent_, // accent
    nsGkAtoms::accentunder_, // accentunder
    nsGkAtoms::actiontype_, // actiontype
    nsGkAtoms::align, // align
    nsGkAtoms::alignmentscope_, // alignmentscope
    nsGkAtoms::alt, // alt
    nsGkAtoms::altimg_, // altimg
    nsGkAtoms::altimg_height_, // altimg-height
@@ -917,17 +917,17 @@ nsStaticAtom* const kAttributesMathML[] 
    nsGkAtoms::symmetric_, // symmetric
    nsGkAtoms::type, // type
    nsGkAtoms::voffset_, // voffset
    nsGkAtoms::width, // width
    nsGkAtoms::xref_, // xref
   nullptr
 };
 
-nsStaticAtom* const kURLAttributesMathML[] = {
+const nsStaticAtom* const kURLAttributesMathML[] = {
   nsGkAtoms::href,
   nsGkAtoms::src,
   nsGkAtoms::cdgroup_,
   nsGkAtoms::altimg_,
   nsGkAtoms::definitionURL_,
   nullptr
 };
 
@@ -979,36 +979,36 @@ nsTreeSanitizer::MustFlatten(int32_t aNa
       return true;
     }
     if (mFullDocument && (nsGkAtoms::title == aLocal ||
                           nsGkAtoms::html == aLocal ||
                           nsGkAtoms::head == aLocal ||
                           nsGkAtoms::body == aLocal)) {
       return false;
     }
-    return !sElementsHTML->GetEntry(aLocal);
+    return !sElementsHTML->Contains(aLocal);
   }
   if (aNamespace == kNameSpaceID_SVG) {
     if (mCidEmbedsOnly || mDropMedia) {
       // Sanitizing CSS-based URL references inside SVG presentational
       // attributes is not supported, so flattening for cid: embed case.
       return true;
     }
-    return !sElementsSVG->GetEntry(aLocal);
+    return !sElementsSVG->Contains(aLocal);
   }
   if (aNamespace == kNameSpaceID_MathML) {
-    return !sElementsMathML->GetEntry(aLocal);
+    return !sElementsMathML->Contains(aLocal);
   }
   return true;
 }
 
 bool
-nsTreeSanitizer::IsURL(nsStaticAtom* const* aURLs, nsAtom* aLocalName)
+nsTreeSanitizer::IsURL(const nsStaticAtom* const* aURLs, nsAtom* aLocalName)
 {
-  nsStaticAtom* atom;
+  const nsStaticAtom* atom;
   while ((atom = *aURLs)) {
     if (atom == aLocalName) {
       return true;
     }
     ++aURLs;
   }
   return false;
 }
@@ -1150,17 +1150,17 @@ nsTreeSanitizer::SanitizeStyleSheet(cons
     LogMessage("Removed some rules and/or properties from stylesheet.", aDocument);
   }
   return didSanitize;
 }
 
 void
 nsTreeSanitizer::SanitizeAttributes(mozilla::dom::Element* aElement,
                                     AtomsTable* aAllowed,
-                                    nsStaticAtom* const* aURLs,
+                                    const nsStaticAtom* const* aURLs,
                                     bool aAllowXLink,
                                     bool aAllowStyle,
                                     bool aAllowDangerousSrc)
 {
   uint32_t ac = aElement->GetAttrCount();
 
   for (int32_t i = ac - 1; i >= 0; --i) {
     const nsAttrName* attrName = aElement->GetAttrNameAt(i);
@@ -1207,20 +1207,20 @@ nsTreeSanitizer::SanitizeAttributes(mozi
           continue;
         }
         // else fall through to see if there's another reason to drop this
         // attribute (in particular if the attribute is background="" on an
         // HTML element)
       }
       if (!mDropNonCSSPresentation &&
           (aAllowed == sAttributesHTML) && // element is HTML
-          sPresAttributesHTML->GetEntry(attrLocal)) {
+          sPresAttributesHTML->Contains(attrLocal)) {
         continue;
       }
-      if (aAllowed->GetEntry(attrLocal) &&
+      if (aAllowed->Contains(attrLocal) &&
           !((attrLocal == nsGkAtoms::rel &&
              aElement->IsHTMLElement(nsGkAtoms::link)) ||
             (!mFullDocument &&
              attrLocal == nsGkAtoms::name &&
              aElement->IsHTMLElement(nsGkAtoms::meta)))) {
         // name="" and rel="" are whitelisted, but treat them as blacklisted
         // for <meta name> (fragment case) and <link rel> (all cases) to avoid
         // document-wide metadata or styling overrides with non-conforming
--- a/dom/base/nsTreeSanitizer.h
+++ b/dom/base/nsTreeSanitizer.h
@@ -85,17 +85,30 @@ class MOZ_STACK_CLASS nsTreeSanitizer {
     /**
      * Whether we should notify to the console for anything that's stripped.
      */
     bool mLogRemovals;
 
     /**
      * We have various tables of static atoms for elements and attributes.
      */
-    typedef nsTHashtable<nsRefPtrHashKey<nsAtom>> AtomsTable;
+    class AtomsTable : public nsTHashtable<nsPtrHashKey<const nsStaticAtom>>
+    {
+    public:
+        explicit AtomsTable(uint32_t aLength)
+          : nsTHashtable<nsPtrHashKey<const nsStaticAtom>>(aLength)
+        {}
+
+        bool Contains(nsAtom* aAtom)
+        {
+            // Because this table only contains static atoms, if aAtom isn't
+            // static we can immediately fail.
+            return aAtom->IsStatic() && GetEntry(aAtom->AsStatic());
+        }
+    };
 
     void SanitizeChildren(nsINode* aRoot);
 
     /**
      * Queries if an element must be replaced with its children.
      * @param aNamespace the namespace of the element the question is about
      * @param aLocal the local name of the element the question is about
      * @return true if the element must be replaced with its children and
@@ -117,17 +130,17 @@ class MOZ_STACK_CLASS nsTreeSanitizer {
 
     /**
      * Checks if a given local name (for an attribute) is on the given list
      * of URL attribute names.
      * @param aURLs the list of URL attribute names
      * @param aLocalName the name to search on the list
      * @return true if aLocalName is on the aURLs list and false otherwise
      */
-    bool IsURL(nsStaticAtom* const* aURLs, nsAtom* aLocalName);
+    bool IsURL(const nsStaticAtom* const* aURLs, nsAtom* aLocalName);
 
     /**
      * Removes dangerous attributes from the element. If the style attribute
      * is allowed, its value is sanitized. The values of URL attributes are
      * sanitized, except src isn't sanitized when it is allowed to remain
      * potentially dangerous.
      *
      * @param aElement the element whose attributes should be sanitized
@@ -135,17 +148,17 @@ class MOZ_STACK_CLASS nsTreeSanitizer {
      * @param aURLs the local names of URL-valued attributes
      * @param aAllowXLink whether XLink attributes are allowed
      * @param aAllowStyle whether the style attribute is allowed
      * @param aAllowDangerousSrc whether to leave the value of the src
      *                           attribute unsanitized
      */
     void SanitizeAttributes(mozilla::dom::Element* aElement,
                             AtomsTable* aAllowed,
-                            nsStaticAtom* const* aURLs,
+                            const nsStaticAtom* const* aURLs,
                             bool aAllowXLink,
                             bool aAllowStyle,
                             bool aAllowDangerousSrc);
 
     /**
      * Remove the named URL attribute from the element if the URL fails a
      * security check.
      *