Bug 1783242 - Part 3: Remove BufferList::Extract, r=glandium,ipc-reviewers,mccr8

The only uses of this method were removed in Part 1, meaning that it can
now be removed. Support for this method adds a significant amount of
complexity to `BufferList` and IPC serialization.

Differential Revision: https://phabricator.services.mozilla.com/D154439

ipc/chromium/src/base/pickle.cc

@@ -392,32 +392,6 @@ bool Pickle::ReadWString(PickleIterator* iter, std::wstring* result) const {
   return true;
 }
 
-bool Pickle::ExtractBuffers(PickleIterator* iter, size_t length,
-                            BufferList* buffers, uint32_t alignment) const {
-  DCHECK(iter);
-  DCHECK(buffers);
-  DCHECK(alignment == 4 || alignment == 8);
-  DCHECK(intptr_t(header_) % alignment == 0);
-
-  if (AlignInt(length) < length || iter->iter_.Done()) {
-    return false;
-  }
-
-  uint32_t padding_len = intptr_t(iter->iter_.Data()) % alignment;
-  if (!iter->iter_.AdvanceAcrossSegments(buffers_, padding_len)) {
-    return false;
-  }
-
-  bool success;
-  *buffers = const_cast<BufferList*>(&buffers_)->Extract(iter->iter_, length,
-                                                         &success);
-  if (!success) {
-    return false;
-  }
-
-  return iter->iter_.AdvanceAcrossSegments(buffers_, AlignInt(length) - length);
-}
-
 bool Pickle::ReadBytesInto(PickleIterator* iter, void* data,
                            uint32_t length) const {
   if (AlignInt(length) < length) {

ipc/chromium/src/base/pickle.h

@@ -113,9 +113,6 @@ class Pickle {
                                  std::wstring* result) const;
   [[nodiscard]] bool ReadBytesInto(PickleIterator* iter, void* data,
                                    uint32_t length) const;
-  [[nodiscard]] bool ExtractBuffers(
-      PickleIterator* iter, size_t length, BufferList* buffers,
-      uint32_t alignment = sizeof(memberAlignmentType)) const;
 
   // Safer version of ReadInt() checks for the result not being negative.
   // Use it for reading the object sizes.

ipc/chromium/src/chrome/common/ipc_message_utils.h

@@ -168,12 +168,6 @@ class MOZ_STACK_CLASS MessageReader final {
     return message_.ReadBytesInto(&iter_, data, length);
   }
 
-  [[nodiscard]] bool ExtractBuffers(
-      size_t length, mozilla::BufferList<InfallibleAllocPolicy>* buffers,
-      uint32_t alignment = sizeof(uint32_t)) {
-    return message_.ExtractBuffers(&iter_, length, buffers, alignment);
-  }
-
   [[nodiscard]] bool IgnoreBytes(uint32_t length) {
     return message_.IgnoreBytes(&iter_, length);
   }

ipc/glue/ByteBufUtils.h

@@ -41,9 +41,6 @@ struct ParamTraits<mozilla::ipc::ByteBuf> {
 
   static bool Read(MessageReader* aReader, paramType* aResult) {
     // We make a copy from the BufferList so that we get a contigous result.
-    // For users the can handle a non-contiguous result using ExtractBuffers
-    // is an option, alternatively if the users don't need to take ownership of
-    // the data they can use the removed FlattenBytes (bug 1297981)
     uint32_t length;
     if (!ReadParam(aReader, &length)) return false;
     if (!aResult->Allocate(length)) {

mfbt/BufferList.h

@@ -359,16 +359,6 @@ class BufferList : private AllocPolicy {
   BufferList<OtherAllocPolicy> MoveFallible(
       bool* aSuccess, OtherAllocPolicy aAP = OtherAllocPolicy());
 
-  // Return a new BufferList that adopts the byte range starting at Iter so that
-  // range [aIter, aIter + aSize) is transplanted to the returned BufferList.
-  // Contents of the buffer before aIter + aSize is left undefined.
-  // Extract can fail, in which case *aSuccess will be false upon return. The
-  // moved buffers are erased from the original BufferList. In case of extract
-  // fails, the original BufferList is intact.  All other iterators except aIter
-  // are invalidated.
-  // This method requires aIter and aSize to be 8-byte aligned.
-  BufferList Extract(IterImpl& aIter, size_t aSize, bool* aSuccess);
-
   // Return the number of bytes from 'start' to 'end', two iterators within
   // this BufferList.
   size_t RangeLength(const IterImpl& start, const IterImpl& end) const {
@@ -557,154 +547,6 @@ BufferList<OtherAllocPolicy> BufferList<AllocPolicy>::MoveFallible(
   return result;
 }
 
-template <typename AllocPolicy>
-BufferList<AllocPolicy> BufferList<AllocPolicy>::Extract(IterImpl& aIter,
-                                                         size_t aSize,
-                                                         bool* aSuccess) {
-  MOZ_RELEASE_ASSERT(aSize);
-  MOZ_RELEASE_ASSERT(mOwning);
-  MOZ_ASSERT(aSize % kSegmentAlignment == 0);
-  MOZ_ASSERT(intptr_t(aIter.mData) % kSegmentAlignment == 0);
-
-  auto failure = [this, aSuccess]() {
-    *aSuccess = false;
-    return BufferList(0, 0, mStandardCapacity);
-  };
-
-  // Number of segments we'll need to copy data from to satisfy the request.
-  size_t segmentsNeeded = 0;
-  // If this is None then the last segment is a full segment, otherwise we need
-  // to copy this many bytes.
-  Maybe<size_t> lastSegmentSize;
-  {
-    // Copy of the iterator to walk the BufferList and see how many segments we
-    // need to copy.
-    IterImpl iter = aIter;
-    size_t remaining = aSize;
-    while (!iter.Done() && remaining &&
-           remaining >= iter.RemainingInSegment()) {
-      remaining -= iter.RemainingInSegment();
-      iter.Advance(*this, iter.RemainingInSegment());
-      segmentsNeeded++;
-    }
-
-    if (remaining) {
-      if (iter.Done()) {
-        // We reached the end of the BufferList and there wasn't enough data to
-        // satisfy the request.
-        return failure();
-      }
-      lastSegmentSize.emplace(remaining);
-      // The last block also counts as a segment. This makes the conditionals
-      // on segmentsNeeded work in the rest of the function.
-      segmentsNeeded++;
-    }
-  }
-
-  BufferList result(0, 0, mStandardCapacity);
-  if (!result.mSegments.reserve(segmentsNeeded + lastSegmentSize.isSome())) {
-    return failure();
-  }
-
-  // Copy the first segment, it's special because we can't just steal the
-  // entire Segment struct from this->mSegments.
-  //
-  // As we leave the data before the new `aIter` position as "unspecified", we
-  // leave this data in the existing buffer, despite copying it into the new
-  // buffer.
-  size_t firstSegmentSize = std::min(aSize, aIter.RemainingInSegment());
-  if (!result.WriteBytes(aIter.Data(), firstSegmentSize)) {
-    return failure();
-  }
-  aIter.Advance(*this, firstSegmentSize);
-  segmentsNeeded--;
-
-  // The entirety of the request wasn't in the first segment, now copy the
-  // rest.
-  if (segmentsNeeded) {
-    size_t finalSegmentCapacity = 0;
-    char* finalSegment = nullptr;
-    // Pre-allocate the final segment so that if this fails, we return before
-    // we delete the elements from |this->mSegments|.
-    if (lastSegmentSize.isSome()) {
-      finalSegmentCapacity = std::max(mStandardCapacity, *lastSegmentSize);
-      finalSegment = this->template pod_malloc<char>(finalSegmentCapacity);
-      if (!finalSegment) {
-        return failure();
-      }
-    }
-
-    size_t removedBytes = 0;
-    size_t copyStart = aIter.mSegment;
-    // Copy segments from this over to the result and remove them from our
-    // storage. Not needed if the only segment we need to copy is the last
-    // partial one.
-    size_t segmentsToCopy = segmentsNeeded - lastSegmentSize.isSome();
-    for (size_t i = 0; i < segmentsToCopy; ++i) {
-      result.mSegments.infallibleAppend(Segment(
-          mSegments[aIter.mSegment].mData, mSegments[aIter.mSegment].mSize,
-          mSegments[aIter.mSegment].mCapacity));
-      removedBytes += mSegments[aIter.mSegment].mSize;
-      aIter.Advance(*this, aIter.RemainingInSegment());
-    }
-    // Due to the way IterImpl works, there are two cases here: (1) if we've
-    // consumed the entirety of the BufferList, then the iterator is pointed at
-    // the end of the final segment, (2) otherwise it is pointed at the start
-    // of the next segment. We want to verify that we really consumed all
-    // |segmentsToCopy| segments.
-    MOZ_RELEASE_ASSERT(
-        (aIter.mSegment == copyStart + segmentsToCopy) ||
-        (aIter.Done() && aIter.mSegment == copyStart + segmentsToCopy - 1));
-    mSegments.erase(mSegments.begin() + copyStart,
-                    mSegments.begin() + copyStart + segmentsToCopy);
-
-    // Reset the iter's position for what we just deleted.
-    aIter.mSegment -= segmentsToCopy;
-    aIter.mAbsoluteOffset -= removedBytes;
-    mSize -= removedBytes;
-
-    // If our iterator is already at the end, we just removed the very last
-    // segment of our buffer list and need to shift the iterator back to point
-    // at the end of the previous segment.
-    if (aIter.Done()) {
-      MOZ_ASSERT(lastSegmentSize.isNothing());
-      if (mSegments.empty()) {
-        MOZ_ASSERT(aIter.mSegment == 0);
-        aIter.mData = aIter.mDataEnd = nullptr;
-      } else {
-        MOZ_ASSERT(aIter.mSegment == mSegments.length() - 1);
-        aIter.mData = aIter.mDataEnd = mSegments.back().End();
-      }
-    }
-
-    if (lastSegmentSize.isSome()) {
-      // We called reserve() on result.mSegments so infallibleAppend is safe.
-      result.mSegments.infallibleAppend(
-          Segment(finalSegment, 0, finalSegmentCapacity));
-      bool r = result.WriteBytes(aIter.Data(), *lastSegmentSize);
-      MOZ_RELEASE_ASSERT(r);
-      aIter.Advance(*this, *lastSegmentSize);
-    }
-  }
-
-  result.mSize = aSize;
-
-  AssertConsistentSize();
-  result.AssertConsistentSize();
-
-  // Ensure that the iterator is still valid when Extract returns.
-#ifdef DEBUG
-  if (!mSegments.empty()) {
-    auto& segment = mSegments[aIter.mSegment];
-    MOZ_ASSERT(segment.Start() <= aIter.mData);
-    MOZ_ASSERT(aIter.mDataEnd == segment.End());
-  }
-#endif
-
-  *aSuccess = true;
-  return result;
-}
-
 template <typename AllocPolicy>
 size_t BufferList<AllocPolicy>::Truncate(IterImpl& aIter) {
   MOZ_ASSERT(aIter.IsIn(*this) || aIter.Done());

mfbt/tests/TestBufferList.cpp

@@ -242,69 +242,6 @@ int main(void) {
   MOZ_RELEASE_ASSERT(iter2.AdvanceAcrossSegments(bl2, kBorrowSize - 5));
   MOZ_RELEASE_ASSERT(iter1.Data() == iter2.Data());
 
-  // Extracting.
-
-  const size_t kExtractStart = 8;
-  const size_t kExtractSize = 24;
-  const size_t kExtractOverSize = 1000;
-
-  iter = bl.Iter();
-  iter.Advance(bl, kExtractStart);
-  bl2 = bl.Extract(iter, kExtractSize, &success);
-  MOZ_RELEASE_ASSERT(success);
-  MOZ_RELEASE_ASSERT(bl2.Size() == kExtractSize);
-
-  BufferList bl3 = bl.Extract(iter, kExtractOverSize, &success);
-  MOZ_RELEASE_ASSERT(!success);
-
-  iter = bl2.Iter();
-  MOZ_RELEASE_ASSERT(iter.AdvanceAcrossSegments(bl2, kExtractSize));
-  MOZ_RELEASE_ASSERT(iter.Done());
-
-  BufferList bl4(8, 8, 8);
-  MOZ_ALWAYS_TRUE(bl4.WriteBytes("abcd1234", 8));
-  iter = bl4.Iter();
-  iter.Advance(bl4, 8);
-
-  BufferList bl5 = bl4.Extract(iter, kExtractSize, &success);
-  MOZ_RELEASE_ASSERT(!success);
-
-  BufferList bl6(0, 0, 16);
-  MOZ_ALWAYS_TRUE(bl6.WriteBytes("abcdefgh12345678", 16));
-  MOZ_ALWAYS_TRUE(bl6.WriteBytes("ijklmnop87654321", 16));
-  iter = bl6.Iter();
-  iter.Advance(bl6, 8);
-  BufferList bl7 = bl6.Extract(iter, 16, &success);
-  MOZ_RELEASE_ASSERT(success);
-  char data[16];
-  MOZ_RELEASE_ASSERT(bl6.ReadBytes(iter, data, 8));
-  MOZ_RELEASE_ASSERT(memcmp(data, "87654321", 8) == 0);
-  iter = bl7.Iter();
-  MOZ_RELEASE_ASSERT(bl7.ReadBytes(iter, data, 16));
-  MOZ_RELEASE_ASSERT(memcmp(data, "12345678ijklmnop", 16) == 0);
-
-  BufferList bl8(0, 0, 16);
-  MOZ_ALWAYS_TRUE(bl8.WriteBytes("abcdefgh12345678", 16));
-  iter = bl8.Iter();
-  BufferList bl9 = bl8.Extract(iter, 8, &success);
-  MOZ_RELEASE_ASSERT(success);
-  MOZ_RELEASE_ASSERT(bl9.Size() == 8);
-  MOZ_RELEASE_ASSERT(!iter.Done());
-
-  BufferList bl10(0, 0, 8);
-  MOZ_ALWAYS_TRUE(bl10.WriteBytes("abcdefgh", 8));
-  MOZ_ALWAYS_TRUE(bl10.WriteBytes("12345678", 8));
-  iter = bl10.Iter();
-  BufferList bl11 = bl10.Extract(iter, 16, &success);
-  MOZ_RELEASE_ASSERT(success);
-  MOZ_RELEASE_ASSERT(bl11.Size() == 16);
-  MOZ_RELEASE_ASSERT(iter.Done());
-  MOZ_RELEASE_ASSERT(iter.AdvanceAcrossSegments(bl10, 0));
-  MOZ_RELEASE_ASSERT(iter.Done());
-  iter = bl11.Iter();
-  MOZ_RELEASE_ASSERT(bl11.ReadBytes(iter, data, 16));
-  MOZ_RELEASE_ASSERT(memcmp(data, "abcdefgh12345678", 16) == 0);
-
   // RangeLength.
 
   BufferList bl12(0, 0, 8);