Bug 1442433 - Make nsAtom::mString more const. r=froydnj
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 07 Mar 2018 11:57:54 +1100
changeset 461909 06e62ae8e4cc238cd75c69fa55997d28562ccf76
parent 461908 786c4905fe76d92f6f45cdce39ea4eb1228791b9
child 461910 ca86b5f0a30a71d0e57e829176bd1eb611d8c2a1
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1442433
milestone60.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 1442433 - Make nsAtom::mString more const. r=froydnj The patch also uses GetStringBuffer() in a couple of appropriate places. MozReview-Commit-ID: JufCUgmO8JL
xpcom/ds/nsAtom.h
xpcom/ds/nsAtomTable.cpp
--- a/xpcom/ds/nsAtom.h
+++ b/xpcom/ds/nsAtom.h
@@ -46,21 +46,22 @@ public:
 
   char16ptr_t GetUTF16String() const { return mString; }
 
   uint32_t GetLength() const { return mLength; }
 
   void ToString(nsAString& aString) const;
   void ToUTF8String(nsACString& aString) const;
 
-  // This is only valid for dynamic atoms.
+  // This is not valid for static atoms. The caller must *not* mutate the
+  // string buffer, otherwise all hell will break loose.
   nsStringBuffer* GetStringBuffer() const
   {
     // See the comment on |mString|'s declaration.
-    MOZ_ASSERT(IsDynamicAtom());
+    MOZ_ASSERT(IsDynamicAtom() || IsHTML5Atom());
     return nsStringBuffer::FromData(mString);
   }
 
   // A hashcode that is better distributed than the actual atom pointer, for
   // use in situations that need a well-distributed hashcode. It's called hash()
   // rather than Hash() so we can use mozilla::BloomFilter<N, nsAtom>, because
   // BloomFilter requires elements to implement a function called hash().
   //
@@ -93,17 +94,17 @@ protected:
 
   const uint32_t mLength:30;
   const uint32_t mKind:2; // nsAtom::AtomKind
   const uint32_t mHash;
   // WARNING! For static atoms, this is a pointer to a static char buffer. For
   // non-static atoms it points to the chars in an nsStringBuffer. This means
   // that nsStringBuffer::FromData(mString) calls are only valid for non-static
   // atoms.
-  char16_t* mString;
+  char16_t* const mString;
 };
 
 // A trivial subclass of nsAtom that can be used for known static atoms. The
 // main advantage of this class is that it doesn't require refcounting, so you
 // can use |nsStaticAtom*| in contrast with |RefPtr<nsAtom>|.
 //
 // This class would be |final| if it wasn't for nsICSSAnonBoxPseudo and
 // nsICSSPseudoElement, which are trivial subclasses used to ensure only
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -87,48 +87,57 @@ private:
   nsDynamicAtom(const nsAString& aString, uint32_t aHash)
     : nsAtom(AtomKind::DynamicAtom, aString, aHash)
     , mRefCnt(1)
   {}
 
   mozilla::ThreadSafeAutoRefCnt mRefCnt;
 };
 
+static char16_t*
+FromStringBuffer(const nsAString& aString)
+{
+  char16_t* str;
+  size_t length = aString.Length();
+  RefPtr<nsStringBuffer> buf = nsStringBuffer::FromString(aString);
+  if (buf) {
+    str = static_cast<char16_t*>(buf->Data());
+  } else {
+    const size_t size = (length + 1) * sizeof(char16_t);
+    buf = nsStringBuffer::Alloc(size);
+    if (MOZ_UNLIKELY(!buf)) {
+      NS_ABORT_OOM(size); // OOM because atom allocations should be small.
+    }
+    str = static_cast<char16_t*>(buf->Data());
+    CopyUnicodeTo(aString, 0, str, length);
+    str[length] = char16_t(0);
+  }
+
+  MOZ_ASSERT(buf && buf->StorageSize() >= (length + 1) * sizeof(char16_t),
+             "enough storage");
+
+  // Take ownership of the string buffer.
+  mozilla::Unused << buf.forget();
+
+  return str;
+}
+
 // This constructor is for dynamic atoms and HTML5 atoms.
 nsAtom::nsAtom(AtomKind aKind, const nsAString& aString, uint32_t aHash)
   : mLength(aString.Length())
   , mKind(static_cast<uint32_t>(aKind))
   , mHash(aHash)
+  , mString(FromStringBuffer(aString))
 {
   MOZ_ASSERT(aKind == AtomKind::DynamicAtom || aKind == AtomKind::HTML5Atom);
-  RefPtr<nsStringBuffer> buf = nsStringBuffer::FromString(aString);
-  if (buf) {
-    mString = static_cast<char16_t*>(buf->Data());
-  } else {
-    const size_t size = (mLength + 1) * sizeof(char16_t);
-    buf = nsStringBuffer::Alloc(size);
-    if (MOZ_UNLIKELY(!buf)) {
-      // We OOM because atom allocations should be small and it's hard to
-      // handle them more gracefully in a constructor.
-      NS_ABORT_OOM(size);
-    }
-    mString = static_cast<char16_t*>(buf->Data());
-    CopyUnicodeTo(aString, 0, mString, mLength);
-    mString[mLength] = char16_t(0);
-  }
 
   MOZ_ASSERT_IF(!IsHTML5Atom(), mHash == HashString(mString, mLength));
 
   MOZ_ASSERT(mString[mLength] == char16_t(0), "null terminated");
-  MOZ_ASSERT(buf && buf->StorageSize() >= (mLength + 1) * sizeof(char16_t),
-             "enough storage");
   MOZ_ASSERT(Equals(aString), "correct data");
-
-  // Take ownership of buffer
-  mozilla::Unused << buf.forget();
 }
 
 // This constructor is for static atoms.
 nsAtom::nsAtom(const char16_t* aString, uint32_t aLength, uint32_t aHash)
   : mLength(aLength)
   , mKind(static_cast<uint32_t>(AtomKind::StaticAtom))
   , mHash(aHash)
   , mString(const_cast<char16_t*>(aString))
@@ -138,31 +147,31 @@ nsAtom::nsAtom(const char16_t* aString, 
   MOZ_ASSERT(mString[mLength] == char16_t(0), "null terminated");
   MOZ_ASSERT(NS_strlen(mString) == mLength, "correct storage");
 }
 
 nsAtom::~nsAtom()
 {
   if (!IsStaticAtom()) {
     MOZ_ASSERT(IsDynamicAtom() || IsHTML5Atom());
-    nsStringBuffer::FromData(mString)->Release();
+    GetStringBuffer()->Release();
   }
 }
 
 void
 nsAtom::ToString(nsAString& aString) const
 {
   // See the comment on |mString|'s declaration.
   if (IsStaticAtom()) {
     // AssignLiteral() lets us assign without copying. This isn't a string
     // literal, but it's a static atom and thus has an unbounded lifetime,
     // which is what's important.
     aString.AssignLiteral(mString, mLength);
   } else {
-    nsStringBuffer::FromData(mString)->ToString(mLength, aString);
+    GetStringBuffer()->ToString(mLength, aString);
   }
 }
 
 void
 nsAtom::ToUTF8String(nsACString& aBuf) const
 {
   MOZ_ASSERT(!IsHTML5Atom(), "Called ToUTF8String() on an HTML5 atom");
   CopyUTF16toUTF8(nsDependentString(mString, mLength), aBuf);
@@ -177,18 +186,17 @@ nsAtom::AddSizeOfIncludingThis(MallocSiz
   size_t thisSize = aMallocSizeOf(this);
   if (IsStaticAtom()) {
     // String buffers pointed to by static atoms are in static memory, and so
     // are not measured here.
     aSizes.mStaticAtomObjects += thisSize;
   } else {
     aSizes.mDynamicAtomObjects += thisSize;
     aSizes.mDynamicUnsharedBuffers +=
-      nsStringBuffer::FromData(mString)->SizeOfIncludingThisIfUnshared(
-        aMallocSizeOf);
+      GetStringBuffer()->SizeOfIncludingThisIfUnshared(aMallocSizeOf);
   }
 }
 
 //----------------------------------------------------------------------
 
 struct AtomTableKey
 {
   AtomTableKey(const char16_t* aUTF16String, uint32_t aLength,