From 9bc7f17b9e4cf346454c564103b7255302f8d4b8 Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 7 Mar 2018 11:57:54 +1100
Subject: [PATCH] Bug 1442433 - Make nsAtom::mString more const. r=froydnj

The patch also uses GetStringBuffer() in a couple of appropriate places.

MozReview-Commit-ID: JufCUgmO8JL

--HG--
extra : rebase_source : ecd3f17b5560b19622c86759d605fa122d70e48a
---
 xpcom/ds/nsAtom.h        |  7 ++---
 xpcom/ds/nsAtomTable.cpp | 56 +++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/xpcom/ds/nsAtom.h b/xpcom/ds/nsAtom.h
index 191af8dcd96a..a8ca3ae01b1d 100644
--- a/xpcom/ds/nsAtom.h
+++ b/xpcom/ds/nsAtom.h
@@ -51,11 +51,12 @@ public:
   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);
   }
 
@@ -98,7 +99,7 @@ protected:
   // 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
diff --git a/xpcom/ds/nsAtomTable.cpp b/xpcom/ds/nsAtomTable.cpp
index 33e0062f0a73..a174ea9c4720 100644
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -92,38 +92,47 @@ private:
   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.
@@ -143,7 +152,7 @@ nsAtom::~nsAtom()
 {
   if (!IsStaticAtom()) {
     MOZ_ASSERT(IsDynamicAtom() || IsHTML5Atom());
-    nsStringBuffer::FromData(mString)->Release();
+    GetStringBuffer()->Release();
   }
 }
 
@@ -157,7 +166,7 @@ nsAtom::ToString(nsAString& aString) const
     // which is what's important.
     aString.AssignLiteral(mString, mLength);
   } else {
-    nsStringBuffer::FromData(mString)->ToString(mLength, aString);
+    GetStringBuffer()->ToString(mLength, aString);
   }
 }
 
@@ -182,8 +191,7 @@ nsAtom::AddSizeOfIncludingThis(MallocSizeOf aMallocSizeOf, AtomsSizes& aSizes)
   } else {
     aSizes.mDynamicAtomObjects += thisSize;
     aSizes.mDynamicUnsharedBuffers +=
-      nsStringBuffer::FromData(mString)->SizeOfIncludingThisIfUnshared(
-        aMallocSizeOf);
+      GetStringBuffer()->SizeOfIncludingThisIfUnshared(aMallocSizeOf);
   }
 }