Bug 526500: Fix bug in OOM handing for nsTSubstring::SetLength. Also change Capacity to return 0 rather than -1 for immutable strings to avoid similar surprises in the future. Finally Make SetCapacity return a boolean indicating success/failure to make it easier to check for oom handling. r=bsmedberg sr=dbaron

xpcom/string/public/nsTSubstring.h

@@ -452,7 +452,15 @@ class nsTSubstring_CharT
          * buffer sizing
          */
 
-      NS_COM void NS_FASTCALL SetCapacity( size_type newCapacity );
+        /**
+         * Attempts to set the capacity to the given size, without affecting
+         * the length of the string. Also ensures that the buffer is mutable.
+         *
+         * @returns PR_TRUE on success
+         *          PR_FALSE on out-of-memory, or if requesting a size bigger
+         *                   than a string can hold (2^31 chars).
+         */
+      NS_COM PRBool NS_FASTCALL SetCapacity( size_type newCapacity );
 
       NS_COM void NS_FASTCALL SetLength( size_type newLength );
 
@@ -616,7 +624,8 @@ class nsTSubstring_CharT
          * returns the number of writable storage units starting at mData.
          * the value does not include space for the null-terminator character.
          *
-         * NOTE: this function returns size_type(-1) if mData is immutable.
+         * NOTE: this function returns 0 if mData is immutable (or the buffer
+         *       is 0-sized).
          */
       size_type NS_FASTCALL Capacity() const;
 

xpcom/string/src/nsPrintfCString.cpp

@@ -70,8 +70,7 @@ nsPrintfCString::nsPrintfCString( size_type n, const char_type* format, ... )
     size_type logical_capacity = kLocalBufferSize;
     if ( n > logical_capacity )
       {
-        SetCapacity(n);
-        if (Capacity() < n)
+        if (!SetCapacity(n))
           return; // out of memory !!
         logical_capacity = n;
       }

xpcom/string/src/nsTSubstring.cpp

@@ -101,17 +101,17 @@ nsTSubstring_CharT::MutatePrep( size_type capacity, char_type** oldData, PRUint3
     // able to allocate it.  Just bail out in cases like that.  We don't want
     // to be allocating 2GB+ strings anyway.
     if (capacity > size_type(-1)/2) {
-      // Also assert for |capacity| equal to |size_type(-1)|, since we use that value to
-      // flag immutability.
+      // Also assert for |capacity| equal to |size_type(-1)|, since we used to
+      // use that value to flag immutability.
       NS_ASSERTION(capacity != size_type(-1), "Bogus capacity");
       return PR_FALSE;
     }
 
-    // |curCapacity == size_type(-1)| means that the buffer is immutable, so we
-    // need to allocate a new buffer.  we cannot use the existing buffer even
+    // |curCapacity == 0| means that the buffer is immutable or 0-sized, so we
+    // need to allocate a new buffer. We cannot use the existing buffer even
     // though it might be large enough.
 
-    if (curCapacity != size_type(-1))
+    if (curCapacity != 0)
       {
         if (capacity <= curCapacity) {
           mFlags &= ~F_VOIDED;  // mutation clears voided flag
@@ -272,7 +272,7 @@ nsTSubstring_CharT::ReplacePrep( index_type cutStart, size_type cutLen, size_typ
 nsTSubstring_CharT::size_type
 nsTSubstring_CharT::Capacity() const
   {
-    // return size_type(-1) to indicate an immutable buffer
+    // return 0 to indicate an immutable or 0-sized buffer
 
     size_type capacity;
     if (mFlags & F_SHARED)
@@ -280,9 +280,10 @@ nsTSubstring_CharT::Capacity() const
         // if the string is readonly, then we pretend that it has no capacity.
         nsStringBuffer* hdr = nsStringBuffer::FromData(mData);
         if (hdr->IsReadonly())
-          capacity = size_type(-1);
-        else
+          capacity = 0;
+        else {
           capacity = (hdr->StorageSize() / sizeof(char_type)) - 1;
+        }
       }
     else if (mFlags & F_FIXED)
       {
@@ -298,7 +299,7 @@ nsTSubstring_CharT::Capacity() const
       }
     else
       {
-        capacity = size_type(-1);
+        capacity = 0;
       }
 
     return capacity;
@@ -554,7 +555,7 @@ nsTSubstring_CharT::Replace( index_type cutStart, size_type cutLength, const sub
       tuple.WriteTo(mData + cutStart, length);
   }
 
-void
+PRBool
 nsTSubstring_CharT::SetCapacity( size_type capacity )
   {
     // capacity does not include room for the terminating null char
@@ -572,7 +573,7 @@ nsTSubstring_CharT::SetCapacity( size_type capacity )
         char_type* oldData;
         PRUint32 oldFlags;
         if (!MutatePrep(capacity, &oldData, &oldFlags))
-          return; // out-of-memory
+          return PR_FALSE; // out-of-memory
 
         // compute new string length
         size_type newLen = NS_MIN(mLength, capacity);
@@ -594,23 +595,14 @@ nsTSubstring_CharT::SetCapacity( size_type capacity )
         // for backwards compat with the old string implementation.
         mData[capacity] = char_type(0);
       }
+
+    return PR_TRUE;
   }
 
 void
 nsTSubstring_CharT::SetLength( size_type length )
   {
-    if (mLength == length) {
-      mFlags &= ~F_VOIDED;  // mutation clears voided flag
-      return;
-    }
-
-    SetCapacity(length);
-
-    // XXX(darin): SetCapacity may fail, but it doesn't give us a way to find
-    // out.  We should improve that.  For now we just verify that the capacity
-    // changed as expected as a means of error checking.
- 
-    if (Capacity() >= length)
+    if (SetCapacity(length))
       mLength = length;
   }