Bug 1484990 - Use BulkWrite instead of write past length via BeginWriting() in XHR. r=baku
authorHenri Sivonen <hsivonen@hsivonen.fi>
Thu, 30 Aug 2018 14:24:58 +0000
changeset 491818 47f6709be61f3b1f1c29334d4bc9ab5076327f5c
parent 491817 24ecf28225607eabadeb38c2a76244c980755495
child 491819 91dc93fd924586ad3db866bb5ea0db0260c8209d
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1484990
milestone63.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 1484990 - Use BulkWrite instead of write past length via BeginWriting() in XHR. r=baku The old code assumes that it's OK to use nsAString::BeginWriting() to write past the string's logical length if the string has enough capacity. This is bogus, because the string doesn't know of data written past its logical length. The BulkWrite API has been created precisely for this purpose and allows orderly capacity-aware low-level writes to the string. MozReview-Commit-ID: BYQHl8Z9Fbd Differential Revision: https://phabricator.services.mozilla.com/D3886
dom/xhr/XMLHttpRequestMainThread.cpp
dom/xhr/XMLHttpRequestMainThread.h
dom/xhr/XMLHttpRequestString.cpp
dom/xhr/XMLHttpRequestString.h
--- a/dom/xhr/XMLHttpRequestMainThread.cpp
+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
@@ -488,56 +488,60 @@ XMLHttpRequestMainThread::DetectCharset(
   } else {
     mDecoder = encoding->NewDecoder();
   }
 
   return NS_OK;
 }
 
 nsresult
-XMLHttpRequestMainThread::AppendToResponseText(const char * aSrcBuffer,
-                                               uint32_t aSrcBufferLen,
+XMLHttpRequestMainThread::AppendToResponseText(Span<const uint8_t> aBuffer,
                                                bool aLast)
 {
   // Call this with an empty buffer to send the decoder the signal
   // that we have hit the end of the stream.
 
   NS_ENSURE_STATE(mDecoder);
 
   CheckedInt<size_t> destBufferLen =
-    mDecoder->MaxUTF16BufferLength(aSrcBufferLen);
-  if (!destBufferLen.isValid()) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  CheckedInt32 size = mResponseText.Length();
-  size += destBufferLen.value();
-  if (!size.isValid()) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  XMLHttpRequestStringWriterHelper helper(mResponseText);
-
-  if (!helper.AddCapacity(destBufferLen.value())) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  uint32_t result;
-  size_t read;
-  size_t written;
-  bool hadErrors;
-  Tie(result, read, written, hadErrors) = mDecoder->DecodeToUTF16(
-    AsBytes(MakeSpan(aSrcBuffer, aSrcBufferLen)),
-    MakeSpan(helper.EndOfExistingData(), destBufferLen.value()),
-    aLast);
-  MOZ_ASSERT(result == kInputEmpty);
-  MOZ_ASSERT(read == aSrcBufferLen);
-  MOZ_ASSERT(written <= destBufferLen.value());
-  Unused << hadErrors;
-  helper.AddLength(written);
+    mDecoder->MaxUTF16BufferLength(aBuffer.Length());
+
+  { // scope for holding the mutex that protects mResponseText
+    XMLHttpRequestStringWriterHelper helper(mResponseText);
+
+    uint32_t len = helper.Length();
+
+    destBufferLen += len;
+    if (!destBufferLen.isValid() || destBufferLen.value() > UINT32_MAX) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    nsresult rv;
+    BulkWriteHandle<char16_t> handle =
+      helper.BulkWrite(destBufferLen.value(), rv);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
+    uint32_t result;
+    size_t read;
+    size_t written;
+    bool hadErrors;
+    Tie(result, read, written, hadErrors) = mDecoder->DecodeToUTF16(
+      aBuffer,
+      handle.AsSpan().From(len),
+      aLast);
+    MOZ_ASSERT(result == kInputEmpty);
+    MOZ_ASSERT(read == aBuffer.Length());
+    len += written;
+    MOZ_ASSERT(len <= destBufferLen.value());
+    Unused << hadErrors;
+    handle.Finish(len, false);
+  } // release mutex
+
   if (aLast) {
     // Drop the finished decoder to avoid calling into a decoder
     // that has finished.
     mDecoder = nullptr;
     mEofDecoded = true;
   }
   return NS_OK;
 }
@@ -592,18 +596,18 @@ XMLHttpRequestMainThread::GetResponseTex
     return;
   }
 
   MatchCharsetAndDecoderToResponseDocument();
 
   MOZ_ASSERT(mResponseBodyDecodedPos < mResponseBody.Length() ||
              mState == XMLHttpRequest_Binding::DONE,
              "Unexpected mResponseBodyDecodedPos");
-  aRv = AppendToResponseText(mResponseBody.get() + mResponseBodyDecodedPos,
-                             mResponseBody.Length() - mResponseBodyDecodedPos,
+  Span<const uint8_t> span = mResponseBody;
+  aRv = AppendToResponseText(span.From(mResponseBodyDecodedPos),
                              mState == XMLHttpRequest_Binding::DONE);
   if (aRv.Failed()) {
     return;
   }
 
   mResponseBodyDecodedPos = mResponseBody.Length();
 
   if (mEofDecoded) {
@@ -1603,17 +1607,17 @@ XMLHttpRequestMainThread::StreamReaderFu
     if (!xmlHttpRequest->mResponseBody.Append(fromRawSegment, count, fallible)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
   } else if (xmlHttpRequest->mResponseType == XMLHttpRequestResponseType::_empty ||
              xmlHttpRequest->mResponseType == XMLHttpRequestResponseType::Text ||
              xmlHttpRequest->mResponseType == XMLHttpRequestResponseType::Json) {
     MOZ_ASSERT(!xmlHttpRequest->mResponseXML,
                "We shouldn't be parsing a doc here");
-    rv = xmlHttpRequest->AppendToResponseText(fromRawSegment, count);
+    rv = xmlHttpRequest->AppendToResponseText(AsBytes(MakeSpan(fromRawSegment, count)));
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
   if (xmlHttpRequest->mFlagParseBody) {
     // Give the same data to the parser.
 
@@ -2094,17 +2098,17 @@ XMLHttpRequestMainThread::OnStopRequest(
   }
 
   // Send the decoder the signal that we've hit the end of the stream,
   // but only when decoding text eagerly.
   if (mDecoder &&
       ((mResponseType == XMLHttpRequestResponseType::Text) ||
         (mResponseType == XMLHttpRequestResponseType::Json) ||
         (mResponseType == XMLHttpRequestResponseType::_empty && !mResponseXML))) {
-    AppendToResponseText(nullptr, 0, true);
+    AppendToResponseText(Span<const uint8_t>(), true);
   }
 
   mWaitingForOnStopRequest = false;
 
   if (mRequestObserver) {
     NS_ASSERTION(mFirstStartRequestSeen, "Inconsistent state!");
     mFirstStartRequestSeen = false;
     mRequestObserver->OnStopRequest(request, ctxt, status);
--- a/dom/xhr/XMLHttpRequestMainThread.h
+++ b/dom/xhr/XMLHttpRequestMainThread.h
@@ -476,17 +476,17 @@ public:
                           Blob* aBlob,
                           nsresult aResult) override;
 
   void
   LocalFileToBlobCompleted(Blob* aBlob);
 
 protected:
   nsresult DetectCharset();
-  nsresult AppendToResponseText(const char * aBuffer, uint32_t aBufferLen,
+  nsresult AppendToResponseText(Span<const uint8_t> aBuffer,
                                 bool aLast = false);
   static nsresult StreamReaderFunc(nsIInputStream* in,
                                    void* closure,
                                    const char* fromRawSegment,
                                    uint32_t toOffset,
                                    uint32_t count,
                                    uint32_t *writeCount);
   nsresult CreateResponseParsedJSON(JSContext* aCx);
--- a/dom/xhr/XMLHttpRequestString.cpp
+++ b/dom/xhr/XMLHttpRequestString.cpp
@@ -33,16 +33,22 @@ public:
   }
 
   uint32_t
   UnsafeLength() const
   {
     return mData.Length();
   }
 
+  mozilla::BulkWriteHandle<char16_t>
+  UnsafeBulkWrite(uint32_t aCapacity, nsresult& aRv)
+  {
+    return mData.BulkWrite(aCapacity, UnsafeLength(), false, aRv);
+  }
+
   void
   Append(const nsAString& aString)
   {
     NS_ASSERT_OWNINGTHREAD(XMLHttpRequestStringBuffer);
 
     MutexAutoLock lock(mMutex);
     mData.Append(aString);
   }
@@ -221,32 +227,26 @@ XMLHttpRequestStringSnapshot::GetAsStrin
 // XMLHttpRequestStringWriterHelper
 
 XMLHttpRequestStringWriterHelper::XMLHttpRequestStringWriterHelper(XMLHttpRequestString& aString)
   : mBuffer(aString.mBuffer)
   , mLock(aString.mBuffer->mMutex)
 {
 }
 
-bool
-XMLHttpRequestStringWriterHelper::AddCapacity(int32_t aCapacity)
+uint32_t
+XMLHttpRequestStringWriterHelper::Length() const
 {
-  return mBuffer->UnsafeData().SetCapacity(mBuffer->UnsafeLength() + aCapacity, fallible);
+  return mBuffer->UnsafeLength();
 }
 
-char16_t*
-XMLHttpRequestStringWriterHelper::EndOfExistingData()
+mozilla::BulkWriteHandle<char16_t>
+XMLHttpRequestStringWriterHelper::BulkWrite(uint32_t aCapacity, nsresult& aRv)
 {
-  return mBuffer->UnsafeData().BeginWriting() + mBuffer->UnsafeLength();
-}
-
-void
-XMLHttpRequestStringWriterHelper::AddLength(int32_t aLength)
-{
-  mBuffer->UnsafeData().SetLength(mBuffer->UnsafeLength() + aLength);
+  return mBuffer->UnsafeBulkWrite(aCapacity, aRv);
 }
 
 // ---------------------------------------------------------------------------
 // XMLHttpRequestStringReaderHelper
 
 XMLHttpRequestStringSnapshotReaderHelper::XMLHttpRequestStringSnapshotReaderHelper(XMLHttpRequestStringSnapshot& aSnapshot)
   : mBuffer(aSnapshot.mBuffer)
   , mLock(aSnapshot.mBuffer->mMutex)
--- a/dom/xhr/XMLHttpRequestString.h
+++ b/dom/xhr/XMLHttpRequestString.h
@@ -59,24 +59,24 @@ private:
 };
 
 // This class locks the buffer and allows the callee to write data into it.
 class MOZ_STACK_CLASS XMLHttpRequestStringWriterHelper final
 {
 public:
   explicit XMLHttpRequestStringWriterHelper(XMLHttpRequestString& aString);
 
-  bool
-  AddCapacity(int32_t aCapacity);
+  /**
+   * The existing length of the string. Do not call during BulkWrite().
+   */
+  uint32_t
+  Length() const;
 
-  char16_t*
-  EndOfExistingData();
-
-  void
-  AddLength(int32_t aLength);
+  mozilla::BulkWriteHandle<char16_t>
+  BulkWrite(uint32_t aCapacity, nsresult& aRv);
 
 private:
   XMLHttpRequestStringWriterHelper(const XMLHttpRequestStringWriterHelper&) = delete;
   XMLHttpRequestStringWriterHelper& operator=(const XMLHttpRequestStringWriterHelper&) = delete;
   XMLHttpRequestStringWriterHelper& operator=(const XMLHttpRequestStringWriterHelper&&) = delete;
 
   RefPtr<XMLHttpRequestStringBuffer> mBuffer;
   MutexAutoLock mLock;