Bug 1582357 - Allow profiler entries up to the size of the buffer - r=gregtatum

In some situations, entries may in fact take more than half the buffer size
(e.g., when duplicating a stack into a small temporary buffer).
So we now allow blocks to take the full buffer size -- but not more, as they
would start overwriting themselves!

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

--HG--
extra : moz-landing-system : lando

mozglue/baseprofiler/public/BlocksRingBuffer.h

@@ -841,23 +841,22 @@ class BlocksRingBuffer {
     {  // Locked block.
       baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
       if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
-        Length bytes = std::forward<CallbackBytes>(aCallbackBytes)();
-        // Don't allow even half of the buffer length. More than that would
-        // probably be unreasonable, and much more would risk having an entry
-        // wrapping around and overwriting itself!
+        const Length entryBytes = std::forward<CallbackBytes>(aCallbackBytes)();
+        const Length bufferBytes =
+            mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value();
         MOZ_RELEASE_ASSERT(
-            bytes < mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value() / 2);
+            entryBytes <= bufferBytes - BufferWriter::ULEB128Size(entryBytes),
+            "Entry would wrap and overwrite itself");
         // COmpute block size from the requested entry size.
-        const Length blockBytes = EntryWriter::BlockSizeForEntrySize(bytes);
+        const Length blockBytes =
+            EntryWriter::BlockSizeForEntrySize(entryBytes);
         // We will put this new block at the end of the current buffer.
         const BlockIndex blockIndex = mNextWriteIndex;
         // Compute the end of this new block...
         const Index blockEnd = Index(blockIndex) + blockBytes;
         // ... which is where the following block will go.
         mNextWriteIndex = BlockIndex(blockEnd);
-        while (blockEnd >
-               Index(mFirstReadIndex) +
-                   mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value()) {
+        while (blockEnd > Index(mFirstReadIndex) + bufferBytes) {
           // About to trample on an old block.
           EntryReader reader = ReaderInBlockAt(mFirstReadIndex);
           // Call provided entry destructor for that entry.
@@ -871,7 +870,7 @@ class BlocksRingBuffer {
         }
         mMaybeUnderlyingBuffer->mPushedBlockCount += 1;
         // Finally, let aCallback write into the entry.
-        EntryWriter entryWriter(*this, blockIndex, bytes);
+        EntryWriter entryWriter(*this, blockIndex, entryBytes);
         return std::forward<Callback>(aCallback)(&entryWriter);
       }
     }  // End of locked block.
@@ -951,9 +950,11 @@ class BlocksRingBuffer {
       return BlockIndex{};
     }
 
-    // Don't allow an entry to wrap around and overwrite itself!
-    MOZ_RELEASE_ASSERT(bytesToCopy <=
-                       mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value());
+    const Length bufferBytes =
+        mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value();
+
+    MOZ_RELEASE_ASSERT(bytesToCopy <= bufferBytes,
+                       "Entry would wrap and overwrite itself");
 
     // We will put all copied blocks at the end of the current buffer.
     const Index dstStartIndex = Index(mNextWriteIndex);
@@ -962,9 +963,7 @@ class BlocksRingBuffer {
     // ... which is where the following block will go.
     mNextWriteIndex = BlockIndex(dstEndIndex);
 
-    while (dstEndIndex >
-           Index(mFirstReadIndex) +
-               mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value()) {
+    while (dstEndIndex > Index(mFirstReadIndex) + bufferBytes) {
       // About to trample on an old block.
       EntryReader reader = ReaderInBlockAt(mFirstReadIndex);
       // Call provided entry destructor for that entry.
@@ -1086,10 +1085,11 @@ class BlocksRingBuffer {
     BufferReader br =
         mMaybeUnderlyingBuffer->mBuffer.ReaderAt(Index(aBlockIndex));
     Length entryBytes = br.ReadULEB128<Length>();
-    // It should be between 1 and half of the buffer length max.
-    MOZ_ASSERT(entryBytes > 0);
-    MOZ_ASSERT(entryBytes <
-               mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value() / 2);
+    MOZ_ASSERT(entryBytes > 0, "Empty entries are not allowed");
+    MOZ_ASSERT(
+        entryBytes < mMaybeUnderlyingBuffer->mBuffer.BufferLength().Value() -
+                         BufferReader::ULEB128Size(entryBytes),
+        "Entry would wrap and overwrite itself");
     // The end of the block should be inside the live buffer range.
     MOZ_ASSERT(Index(aBlockIndex) + BufferReader::ULEB128Size(entryBytes) +
                    entryBytes <=
@@ -1187,23 +1187,35 @@ class BlocksRingBuffer {
 
   struct UnderlyingBuffer {
     // Create a buffer of the given length.
-    explicit UnderlyingBuffer(PowerOfTwo<Length> aLength) : mBuffer(aLength) {}
+    explicit UnderlyingBuffer(PowerOfTwo<Length> aLength) : mBuffer(aLength) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Take ownership of an existing buffer.
     UnderlyingBuffer(UniquePtr<Buffer::Byte[]> aExistingBuffer,
                      PowerOfTwo<Length> aLength)
-        : mBuffer(std::move(aExistingBuffer), aLength) {}
+        : mBuffer(std::move(aExistingBuffer), aLength) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Use an externally-owned buffer.
     UnderlyingBuffer(Buffer::Byte* aExternalBuffer, PowerOfTwo<Length> aLength)
-        : mBuffer(aExternalBuffer, aLength) {}
+        : mBuffer(aExternalBuffer, aLength) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Create a buffer of the given length.
     template <typename EntryDestructor>
     explicit UnderlyingBuffer(PowerOfTwo<Length> aLength,
                               EntryDestructor&& aEntryDestructor)
         : mBuffer(aLength),
-          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {}
+          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Take ownership of an existing buffer.
     template <typename EntryDestructor>
@@ -1211,7 +1223,10 @@ class BlocksRingBuffer {
                               PowerOfTwo<Length> aLength,
                               EntryDestructor&& aEntryDestructor)
         : mBuffer(std::move(aExistingBuffer), aLength),
-          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {}
+          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Use an externally-owned buffer.
     template <typename EntryDestructor>
@@ -1219,7 +1234,10 @@ class BlocksRingBuffer {
                               PowerOfTwo<Length> aLength,
                               EntryDestructor&& aEntryDestructor)
         : mBuffer(aExternalBuffer, aLength),
-          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {}
+          mEntryDestructor(std::forward<EntryDestructor>(aEntryDestructor)) {
+      MOZ_ASSERT(aLength.Value() > ULEB128MaxSize<Length>(),
+                 "Buffer should be able to contain more than a block size");
+    }
 
     // Only allow move-construction.
     UnderlyingBuffer(UnderlyingBuffer&&) = default;