Separate owning and non-owning net:IOBuffers by class.

Currently, this is determined based upon constructor overload.
Instead, simplify by making ownership depend upon the class itself.
Re-use the existing IOBufferWithSize class for the owning class, just
to cut down on the number of places touched by the code change. Once
functional, this can be mass renamed to something more meaningful.

Bug: 1493389
Change-Id: I864f248b275f747ee9d6fbbb36f6f07ac9041d47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4977740
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1222411}

components/services/storage/public/cpp/big_io_buffer.cc

@@ -10,21 +10,18 @@
 namespace storage {
 
 BigIOBuffer::BigIOBuffer(mojo_base::BigBuffer buffer)
-    : net::IOBufferWithSize(nullptr, buffer.size()),
-      buffer_(std::move(buffer)) {
+    : net::IOBuffer(nullptr, buffer.size()), buffer_(std::move(buffer)) {
   data_ = reinterpret_cast<char*>(buffer_.data());
 }
 
 BigIOBuffer::BigIOBuffer(size_t size)
-    : net::IOBufferWithSize(nullptr, size),
-      buffer_(mojo_base::BigBuffer(size)) {
+    : net::IOBuffer(nullptr, size), buffer_(mojo_base::BigBuffer(size)) {
   data_ = reinterpret_cast<char*>(buffer_.data());
   DCHECK(data_);
 }
 
 BigIOBuffer::~BigIOBuffer() {
-  // Must clear `data_` so base class doesn't attempt to delete[] a pointer
-  // it doesn't own.
+  // Must clear `data_` so base class doesn't hold a dangling ptr.
   data_ = nullptr;
   size_ = 0UL;
 }

components/services/storage/public/cpp/big_io_buffer.h

@@ -13,7 +13,7 @@ namespace storage {
 // A net::IOBufferWithSize backed by a mojo_base::BigBuffer. Avoids having to
 // copy an IOBuffer to a BigBuffer to return the results of an IO operation.
 class COMPONENT_EXPORT(STORAGE_SERVICE_PUBLIC) BigIOBuffer
-    : public net::IOBufferWithSize {
+    : public net::IOBuffer {
  public:
   BigIOBuffer(const BigIOBuffer&) = delete;
   BigIOBuffer& operator=(const BigIOBuffer&) = delete;
@@ -30,4 +30,4 @@ class COMPONENT_EXPORT(STORAGE_SERVICE_PUBLIC) BigIOBuffer
 
 }  // namespace storage
 
-#endif  // COMPONENTS_SERVICES_STORAGE_PUBLIC_CPP_BIG_IO_BUFFER_H_
+#endif  // COMPONENTS_SERVICES_STORAGE_PUBLIC_CPP_BIG_IO_BUFFER_H_

net/base/io_buffer.cc

@@ -23,31 +23,26 @@ void IOBuffer::AssertValidBufferSize(size_t size) {
 
 IOBuffer::IOBuffer() = default;
 
-IOBuffer::IOBuffer(size_t buffer_size) {
+IOBuffer::IOBuffer(char* data, size_t size) : data_(data), size_(size) {
+  AssertValidBufferSize(size);
+}
+
+IOBuffer::~IOBuffer() = default;
+
+IOBufferWithSize::IOBufferWithSize() = default;
+
+IOBufferWithSize::IOBufferWithSize(size_t buffer_size) {
   AssertValidBufferSize(buffer_size);
   if (buffer_size) {
     size_ = buffer_size;
     data_ = new char[buffer_size];
   }
 }
 
-IOBuffer::IOBuffer(char* data, size_t size) : data_(data), size_(size) {
-  AssertValidBufferSize(size);
-}
-
-IOBuffer::~IOBuffer() {
+IOBufferWithSize::~IOBufferWithSize() {
   data_.ClearAndDeleteArray();
 }
 
-IOBufferWithSize::IOBufferWithSize() = default;
-
-IOBufferWithSize::IOBufferWithSize(size_t size) : IOBuffer(size) {}
-
-IOBufferWithSize::IOBufferWithSize(char* data, size_t size)
-    : IOBuffer(data, size) {}
-
-IOBufferWithSize::~IOBufferWithSize() = default;
-
 StringIOBuffer::StringIOBuffer(std::string s) : string_data_(std::move(s)) {
   // Can't pass `s.data()` directly to IOBuffer constructor since moving
   // from `s` may invalidate it. This is especially true for libc++ short
@@ -59,8 +54,7 @@ StringIOBuffer::StringIOBuffer(std::string s) : string_data_(std::move(s)) {
 }
 
 StringIOBuffer::~StringIOBuffer() {
-  // We haven't allocated the buffer, so remove it before the base class
-  // destructor tries to delete[] it.
+  // Clear pointer before this destructor makes it dangle.
   data_ = nullptr;
 }
 
@@ -88,7 +82,8 @@ void DrainableIOBuffer::SetOffset(int bytes) {
 }
 
 DrainableIOBuffer::~DrainableIOBuffer() {
-  // The buffer is owned by the |base_| instance.
+  // Clear ptr before this destructor destroys the |base_| instance,
+  // making it dangle.
   data_ = nullptr;
 }
 
@@ -138,14 +133,13 @@ void PickledIOBuffer::Done() {
 }
 
 PickledIOBuffer::~PickledIOBuffer() {
+  // Avoid dangling ptr when this destructor destroys the pickle.
   data_ = nullptr;
 }
 
 WrappedIOBuffer::WrappedIOBuffer(const char* data, size_t size)
     : IOBuffer(const_cast<char*>(data), size) {}
 
-WrappedIOBuffer::~WrappedIOBuffer() {
-  data_ = nullptr;
-}
+WrappedIOBuffer::~WrappedIOBuffer() = default;
 
 }  // namespace net

net/base/io_buffer.h

@@ -75,11 +75,10 @@ namespace net {
 // and hence the buffer it was reading into must remain alive. Using
 // reference counting we can add a reference to the IOBuffer and make sure
 // it is not destroyed until after the synchronous operation has completed.
+
+// Base class, never instantiated, does not own the buffer.
 class NET_EXPORT IOBuffer : public base::RefCountedThreadSafe<IOBuffer> {
  public:
-  IOBuffer();
-  explicit IOBuffer(size_t buffer_size);
-
   int size() const { return size_; }
 
   char* data() { return data_; }
@@ -95,8 +94,7 @@ class NET_EXPORT IOBuffer : public base::RefCountedThreadSafe<IOBuffer> {
 
   static void AssertValidBufferSize(size_t size);
 
-  // Only allow derived classes to specify data_.
-  // In all other cases, we own data_, and must delete it at destruction time.
+  IOBuffer();
   IOBuffer(char* data, size_t size);
 
   virtual ~IOBuffer();
@@ -106,19 +104,13 @@ class NET_EXPORT IOBuffer : public base::RefCountedThreadSafe<IOBuffer> {
   int size_ = 0;
 };
 
-// Currently, this is the same as IOBuffer.
-// TODO(tsepez): Long-term this should become the only class which owns
-// its buffers, with IOBuffer becoming a non-owning root class.
+// Class which owns its buffer and manages its destruction.
 class NET_EXPORT IOBufferWithSize : public IOBuffer {
  public:
   IOBufferWithSize();
   explicit IOBufferWithSize(size_t size);
 
  protected:
-  // Purpose of this constructor is to give a subclass access to the base class
-  // constructor IOBuffer(char*) thus allowing subclass to use underlying
-  // memory it does not own.
-  IOBufferWithSize(char* data, size_t size);
   ~IOBufferWithSize() override;
 };