Bug 1330759 part 3. Add a "stringbuffer we own" mode to DOMString. r=froydnj
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 18 Jan 2017 22:20:14 -0500
changeset 377374 04378bf08ef9fb8b1bfd91edae692a7b80c472cd
parent 377373 3afcfb43c465cd48e8ca859ae21808c84cb52b43
child 377375 22ae3a4fb412d9278eb472395fa96c82f68a4809
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1330759
milestone53.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 1330759 part 3. Add a "stringbuffer we own" mode to DOMString. r=froydnj We're going to need it because we're going to add a consumer that cannot in fact promise that its stringbuffer reference will outlive the DOMString.
dom/bindings/DOMString.h
js/xpconnect/src/xpcpublic.h
--- a/dom/bindings/DOMString.h
+++ b/dom/bindings/DOMString.h
@@ -17,43 +17,54 @@
 
 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 SetStringBuffer or SetOwnedString or SetOwnedAtom on this object, but
- * only if it plans to keep holding a strong ref to the internal stringbuffer!
+ * call SetStringBuffer or SetEphemeralStringBuffer or SetOwnedString or
+ * SetOwnedAtom on this object.  It's only OK to call
+ * SetStringBuffer/SetOwnedString/SetOwnedAtom 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 SetStringBuffer with a
- * non-null stringbuffer, to call SetOwnedString, to call SetOwnedAtom, 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.
+ * (which leaves this as an empty string), to call
+ * SetStringBuffer/SetEphemeralStringBuffer with a non-null stringbuffer, to
+ * call SetOwnedString, to call SetOwnedAtom, 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.
  */
 class MOZ_STACK_CLASS DOMString {
 public:
   DOMString()
     : mStringBuffer(nullptr)
     , mLength(0)
     , mIsNull(false)
+    , mStringBufferOwned(false)
   {}
   ~DOMString()
   {
     MOZ_ASSERT(!mString || !mStringBuffer,
                "Shouldn't have both present!");
+    if (mStringBufferOwned) {
+      MOZ_ASSERT(mStringBuffer);
+      mStringBuffer->Release();
+    }
   }
 
   operator nsString&()
   {
     return AsAString();
   }
 
   // It doesn't make any sense to convert a DOMString to a const nsString or
@@ -98,29 +109,52 @@ public:
   // 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) {
+      // Just hand that ref over.
+      mStringBufferOwned = false;
+    } 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 SetStringBuffer(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;
   }
 
+  // Like SetStringBuffer, but holds a reference to the nsStringBuffer.
+  void SetEphemeralStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength)
+  {
+    // We rely on SetStringBuffer to ensure our state invariants.
+    SetStringBuffer(aStringBuffer, aLength);
+    aStringBuffer->AddRef();
+    mStringBufferOwned = true;
+  }
+
   void SetOwnedString(const nsAString& aString)
   {
     MOZ_ASSERT(mString.isNothing(), "We already have a string?");
     MOZ_ASSERT(!mIsNull, "We're already set as null");
     MOZ_ASSERT(!mStringBuffer, "Setting stringbuffer twice?");
     nsStringBuffer* buf = nsStringBuffer::FromString(aString);
     if (buf) {
       SetStringBuffer(buf, aString.Length());
@@ -197,14 +231,15 @@ private:
 
   // For callees that know we exist, we can be a stringbuffer/length/null-flag
   // triple.
   nsStringBuffer* MOZ_UNSAFE_REF("The ways in which this can be safe are "
                                  "documented above and enforced through "
                                  "assertions") mStringBuffer;
   uint32_t mLength;
   bool mIsNull;
+  bool mStringBufferOwned;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_DOMString_h
--- a/js/xpconnect/src/xpcpublic.h
+++ b/js/xpconnect/src/xpcpublic.h
@@ -352,17 +352,17 @@ bool NonVoidStringToJsval(JSContext* cx,
     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
-        buf->AddRef();
+        str.RelinquishBufferOwnership();
     }
     return true;
 }
 
 MOZ_ALWAYS_INLINE
 bool StringToJsval(JSContext* cx, mozilla::dom::DOMString& str,
                    JS::MutableHandleValue rval)
 {