Bug 1407858 part 2. Make DOMString's data model clearer and update various documentation. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 22 Dec 2017 13:02:51 -0500
changeset 449116 282695389a27575824079980f2b66cfd2d38e186
parent 449115 a229bf23450588c4548333d0e4184920b5411c80
child 449117 45fe585acac17620a94c163f53d0008d258f3927
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1407858
milestone59.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 1407858 part 2. Make DOMString's data model clearer and update various documentation. r=smaug MozReview-Commit-ID: AaTeI1e7Qnk
dom/base/Element.h
dom/bindings/DOMString.h
js/xpconnect/src/xpcpublic.h
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -965,34 +965,32 @@ private:
 
 protected:
   inline bool GetAttr(int32_t aNameSpaceID, nsAtom* aName,
                       DOMString& aResult) const
   {
     NS_ASSERTION(nullptr != aName, "must have attribute name");
     NS_ASSERTION(aNameSpaceID != kNameSpaceID_Unknown,
                  "must have a real namespace ID!");
-    MOZ_ASSERT(aResult.HasStringBuffer() && aResult.StringBufferLength() == 0,
-               "Should have empty string coming in");
+    MOZ_ASSERT(aResult.IsEmpty(), "Should have empty string coming in");
     const nsAttrValue* val = mAttrsAndChildren.GetAttr(aName, aNameSpaceID);
     if (val) {
       val->ToString(aResult);
       return true;
     }
     // else DOMString comes pre-emptied.
     return false;
   }
 
 public:
   bool HasAttrs() const { return mAttrsAndChildren.HasAttrs(); }
 
   inline bool GetAttr(const nsAString& aName, DOMString& aResult) const
   {
-    MOZ_ASSERT(aResult.HasStringBuffer() && aResult.StringBufferLength() == 0,
-               "Should have empty string coming in");
+    MOZ_ASSERT(aResult.IsEmpty(), "Should have empty string coming in");
     const nsAttrValue* val = mAttrsAndChildren.GetAttr(aName);
     if (val) {
       val->ToString(aResult);
       return true;
     }
     // else DOMString comes pre-emptied.
     return false;
   }
--- a/dom/bindings/DOMString.h
+++ b/dom/bindings/DOMString.h
@@ -17,51 +17,53 @@
 
 namespace mozilla {
 namespace dom {
 
 /**
  * A class for representing string return values.  This can be either passed to
  * callees that have an nsString or nsAString out param or passed to a callee
  * that actually knows about this class and can work with it.  Such a callee may
- * call SetKnownLiveStringBuffer or SetStringBuffer or SetKnownLiveString or
- * SetKnownLiveAtom on this object.  It's only OK to call
+ * call these setters:
+ *
+ *   SetKnownLiveStringBuffer
+ *   SetStringBuffer
+ *   SetKnownLiveString
+ *   SetKnownLiveAtom
+ *   SetNull
+ *
+ * to assign a value to the DOMString without instantiating an actual nsString
+ * in the process, or use AsAString() to instantiate an nsString and work with
+ * it.  These options are mutually exclusive!  Don't do more than one of them.
+ *
+ * It's only OK to call
  * SetKnownLiveStringBuffer/SetKnownLiveString/SetKnownLiveAtom if the caller of
  * the method in question plans to keep holding a strong ref to the stringbuffer
  * involved, whether it's a raw nsStringBuffer, or stored inside the string or
  * atom being passed.  In the string/atom cases that means the caller must own
  * the string or atom, and not mutate it (in the string case) for the lifetime
  * of the DOMString.
  *
- * The proper way to store a value in this class is to either to do nothing
- * (which leaves this as an empty string), to call
- * SetKnownLiveStringBuffer/SetStringBuffer with a non-null stringbuffer, to
- * call SetKnownLiveString, to call SetKnownLiveAtom, to call SetNull(), or to
- * call AsAString() and set the value in the resulting nsString.  These options
- * are mutually exclusive! Don't do more than one of them.
- *
  * The proper way to extract a value is to check IsNull().  If not null, then
- * check HasStringBuffer().  If that's true, check for a zero length, and if the
- * length is nonzero call StringBuffer().  If the length is zero this is the
- * empty string.  If HasStringBuffer() returns false, call AsAString() and get
- * the value from that.
+ * check IsEmpty().  If neither of those is true, check HasStringBuffer().  If
+ * that's true, call StringBuffer().  If HasStringBuffer() returns false, call
+ * AsAString() and get the value from that.
  */
 class MOZ_STACK_CLASS DOMString {
 public:
   DOMString()
     : mStringBuffer(nullptr)
     , mLength(0)
-    , mIsNull(false)
-    , mStringBufferOwned(false)
+    , mState(State::Empty)
   {}
   ~DOMString()
   {
     MOZ_ASSERT(!mString || !mStringBuffer,
                "Shouldn't have both present!");
-    if (mStringBufferOwned) {
+    if (mState == State::OwnedStringBuffer) {
       MOZ_ASSERT(mStringBuffer);
       mStringBuffer->Release();
     }
   }
 
   operator nsString&()
   {
     return AsAString();
@@ -69,184 +71,225 @@ public:
 
   // It doesn't make any sense to convert a DOMString to a const nsString or
   // nsAString reference; this class is meant for outparams only.
   operator const nsString&() = delete;
   operator const nsAString&() = delete;
 
   nsString& AsAString()
   {
+    MOZ_ASSERT(mState == State::Empty || mState == State::String,
+               "Moving from nonempty state to another nonempty state?");
     MOZ_ASSERT(!mStringBuffer, "We already have a stringbuffer?");
-    MOZ_ASSERT(!mIsNull, "We're already set as null");
     if (!mString) {
       mString.emplace();
+      mState = State::String;
     }
     return *mString;
   }
 
   bool HasStringBuffer() const
   {
     MOZ_ASSERT(!mString || !mStringBuffer,
                "Shouldn't have both present!");
-    MOZ_ASSERT(!mIsNull, "Caller should have checked IsNull() first");
-    return !mString;
+    MOZ_ASSERT(mState > State::Null,
+               "Caller should have checked IsNull() and IsEmpty() first");
+    return mState >= State::OwnedStringBuffer;
   }
 
   // Get the stringbuffer.  This can only be called if HasStringBuffer()
-  // returned true and StringBufferLength() is nonzero.  If that's true, it will
-  // never return null.  Note that constructing a string from this
-  // nsStringBuffer with length given by StringBufferLength() might give you
-  // something that is not null-terminated.
+  // returned true.  If that's true, it will never return null.  Note that
+  // constructing a string from this nsStringBuffer with length given by
+  // StringBufferLength() might give you something that is not null-terminated.
   nsStringBuffer* StringBuffer() const
   {
-    MOZ_ASSERT(!mIsNull, "Caller should have checked IsNull() first");
     MOZ_ASSERT(HasStringBuffer(),
                "Don't ask for the stringbuffer if we don't have it");
-    MOZ_ASSERT(StringBufferLength() != 0, "Why are you asking for this?");
     MOZ_ASSERT(mStringBuffer,
-               "If our length is nonzero, we better have a stringbuffer.");
+               "We better have a stringbuffer if we claim to");
     return mStringBuffer;
   }
 
   // Get the length of the stringbuffer.  Can only be called if
   // HasStringBuffer().
   uint32_t StringBufferLength() const
   {
     MOZ_ASSERT(HasStringBuffer(), "Don't call this if there is no stringbuffer");
     return mLength;
   }
 
   // Tell the DOMString to relinquish ownership of its nsStringBuffer to the
   // caller.  Can only be called if HasStringBuffer().
   void RelinquishBufferOwnership()
   {
     MOZ_ASSERT(HasStringBuffer(), "Don't call this if there is no stringbuffer");
-    if (mStringBufferOwned) {
+    if (mState == State::OwnedStringBuffer) {
       // Just hand that ref over.
-      mStringBufferOwned = false;
+      mState = State::UnownedStringBuffer;
     } else {
       // Caller should end up holding a ref.
       mStringBuffer->AddRef();
     }
   }
 
   // Initialize the DOMString to a (nsStringBuffer, length) pair.  The length
   // does NOT have to be the full length of the (null-terminated) string in the
   // nsStringBuffer.
   void SetKnownLiveStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength)
   {
-    MOZ_ASSERT(mString.isNothing(), "We already have a string?");
-    MOZ_ASSERT(!mIsNull, "We're already set as null");
-    MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
-    MOZ_ASSERT(aStringBuffer, "Why are we getting null?");
-    mStringBuffer = aStringBuffer;
-    mLength = aLength;
+    MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
+    if (aLength != 0) {
+      SetStringBufferInternal(aStringBuffer, aLength);
+      mState = State::UnownedStringBuffer;
+    }
+    // else nothing to do
   }
 
   // Like SetKnownLiveStringBuffer, but holds a reference to the nsStringBuffer.
   void SetStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength)
   {
-    // We rely on SetKnownLiveStringBuffer to ensure our state invariants.
-    SetKnownLiveStringBuffer(aStringBuffer, aLength);
-    aStringBuffer->AddRef();
-    mStringBufferOwned = true;
+    MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
+    if (aLength != 0) {
+      SetStringBufferInternal(aStringBuffer, aLength);
+      aStringBuffer->AddRef();
+      mState = State::OwnedStringBuffer;
+    }
+    // else nothing to do
   }
 
   void SetKnownLiveString(const nsAString& aString)
   {
     MOZ_ASSERT(mString.isNothing(), "We already have a string?");
-    MOZ_ASSERT(!mIsNull, "We're already set as null");
+    MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
     MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
-    nsStringBuffer* buf = nsStringBuffer::FromString(aString);
-    if (buf) {
-      SetKnownLiveStringBuffer(buf, aString.Length());
-    } else if (aString.IsVoid()) {
+    if (MOZ_UNLIKELY(aString.IsVoid())) {
       SetNull();
     } else if (!aString.IsEmpty()) {
-      AsAString() = aString;
+      nsStringBuffer* buf = nsStringBuffer::FromString(aString);
+      if (buf) {
+        SetKnownLiveStringBuffer(buf, aString.Length());
+      } else {
+        AsAString() = aString;
+      }
     }
   }
 
   enum NullHandling
   {
     eTreatNullAsNull,
     eTreatNullAsEmpty,
     eNullNotExpected
   };
 
   void SetKnownLiveAtom(nsAtom* aAtom, NullHandling aNullHandling)
   {
     MOZ_ASSERT(mString.isNothing(), "We already have a string?");
-    MOZ_ASSERT(!mIsNull, "We're already set as null");
+    MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
     MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
     MOZ_ASSERT(aAtom || aNullHandling != eNullNotExpected);
     if (aNullHandling == eNullNotExpected || aAtom) {
       if (aAtom->IsStaticAtom()) {
         // XXX: bug 1407858 will replace this with a direct assignment of the
         // static atom that doesn't go via nsString.
         AsAString().AssignLiteral(aAtom->GetUTF16String(), aAtom->GetLength());
       } else {
-        // Dynamic atoms always have a string buffer.
+        // Dynamic atoms always have a string buffer and never have 0 length,
+        // because nsGkAtoms::_empty is a static atom.
         SetKnownLiveStringBuffer(aAtom->GetStringBuffer(), aAtom->GetLength());
       }
     } else if (aNullHandling == eTreatNullAsNull) {
       SetNull();
     }
   }
 
   void SetNull()
   {
     MOZ_ASSERT(!mStringBuffer, "Should have no stringbuffer if null");
     MOZ_ASSERT(mString.isNothing(), "Should have no string if null");
-    mIsNull = true;
+    MOZ_ASSERT(mState == State::Empty, "Already set to a value?");
+    mState = State::Null;
   }
 
   bool IsNull() const
   {
     MOZ_ASSERT(!mStringBuffer || mString.isNothing(),
                "How could we have a stringbuffer and a nonempty string?");
-    return mIsNull || (mString && mString->IsVoid());
+    return mState == State::Null || (mString && mString->IsVoid());
+  }
+
+  bool IsEmpty() const
+  {
+    MOZ_ASSERT(!mStringBuffer || mString.isNothing(),
+               "How could we have a stringbuffer and a nonempty string?");
+    // This is not exact, because we might still have an empty XPCOM string.
+    // But that's OK; in that case the callers will try the XPCOM string
+    // themselves.
+    return mState == State::Empty;
   }
 
   void ToString(nsAString& aString)
   {
     if (IsNull()) {
       SetDOMStringToNull(aString);
+    } else if (IsEmpty()) {
+      aString.Truncate();
     } else if (HasStringBuffer()) {
-      if (StringBufferLength() == 0) {
-        aString.Truncate();
+      // Don't share the nsStringBuffer with aString if the result would not
+      // be null-terminated.
+      nsStringBuffer* buf = StringBuffer();
+      uint32_t len = StringBufferLength();
+      auto chars = static_cast<char16_t*>(buf->Data());
+      if (chars[len] == '\0') {
+        // Safe to share the buffer.
+        buf->ToString(len, aString);
       } else {
-        // Don't share the nsStringBuffer with aString if the result would not
-        // be null-terminated.
-        nsStringBuffer* buf = StringBuffer();
-        uint32_t len = StringBufferLength();
-        auto chars = static_cast<char16_t*>(buf->Data());
-        if (chars[len] == '\0') {
-          // Safe to share the buffer.
-          buf->ToString(len, aString);
-        } else {
-          // We need to copy, unfortunately.
-          aString.Assign(chars, len);
-        }
+        // We need to copy, unfortunately.
+        aString.Assign(chars, len);
       }
     } else {
       aString = AsAString();
     }
   }
 
 private:
+  void SetStringBufferInternal(nsStringBuffer* aStringBuffer, uint32_t aLength)
+  {
+    MOZ_ASSERT(mString.isNothing(), "We already have a string?");
+    MOZ_ASSERT(mState == State::Empty, "We're already set to a value");
+    MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
+    MOZ_ASSERT(aStringBuffer, "Why are we getting null?");
+    MOZ_ASSERT(aLength != 0, "Should not have empty string here");
+    mStringBuffer = aStringBuffer;
+    mLength = aLength;
+  }
+
+  enum class State : uint8_t
+  {
+    Empty, // An empty string.  Default state.
+    Null,  // Null (not a string at all)
+
+    // All states that involve actual string data should come after
+    // Empty and Null.
+
+    String, // An XPCOM string stored in mString.
+    OwnedStringBuffer, // mStringBuffer is valid and we have a ref to it.
+    UnownedStringBuffer, // mStringBuffer is valid; we are not holding a ref.
+    // The two string buffer values must come last.  This lets us avoid doing
+    // two tests to figure out whether we have a stringbuffer.
+  };
+
   // We need to be able to act like a string as needed
   Maybe<nsAutoString> mString;
 
-  // For callees that know we exist, we can be a stringbuffer/length/null-flag
-  // triple.
+  // The nsStringBuffer in the OwnedStringBuffer/UnownedStringBuffer cases.
   nsStringBuffer* MOZ_UNSAFE_REF("The ways in which this can be safe are "
                                  "documented above and enforced through "
                                  "assertions") mStringBuffer;
+  // Length in the stringbuffer cases.
   uint32_t mLength;
-  bool mIsNull;
-  bool mStringBufferOwned;
+
+  State mState;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_DOMString_h
--- a/js/xpconnect/src/xpcpublic.h
+++ b/js/xpconnect/src/xpcpublic.h
@@ -351,27 +351,27 @@ StringToJsval(JSContext* cx, const nsASt
 
 /**
  * As above, but for mozilla::dom::DOMString.
  */
 inline
 bool NonVoidStringToJsval(JSContext* cx, mozilla::dom::DOMString& str,
                           JS::MutableHandleValue rval)
 {
+    if (str.IsEmpty()) {
+        rval.set(JS_GetEmptyStringValue(cx));
+        return true;
+    }
+
     if (!str.HasStringBuffer()) {
         // It's an actual XPCOM string
         return NonVoidStringToJsval(cx, str.AsAString(), rval);
     }
 
     uint32_t length = str.StringBufferLength();
-    if (length == 0) {
-        rval.set(JS_GetEmptyStringValue(cx));
-        return true;
-    }
-
     nsStringBuffer* buf = str.StringBuffer();
     bool shared;
     if (!XPCStringConvert::StringBufferToJSVal(cx, buf, length, rval,
                                                &shared)) {
         return false;
     }
     if (shared) {
         // JS now needs to hold a reference to the buffer