Final PacketBufferHandle cleanup. (project-chip#4364)

* Final PacketBufferHandle cleanup.

#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
A few references and transitional interfaces remain.

#### Summary of Changes

- Promote `Release_ForNow()` to `UnsafeRelease()`. It's impractical
  to remove entirely since it's used conjunction with platform event
  queues.
- Rename `GetLwIPpbuf()` to `UnsafeGetLwIPpbuf()` for consistency.
- Privatize `AddRef()` and raw `SetDataLength()`.
- Remove `AddToEnd_ForNow()`.
- Remove various `using PacketBuffer` declarations.
- Remove obsolete comments about `PacketBuffer *` ownership, and
  rephrase comments mentioning the `PacketBuffer` class by name.

Fixes project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system

examples/chip-tool/commands/common/Command.h

@@ -78,7 +78,6 @@ class Command
     using ChipDevice             = ::chip::Controller::Device;
     using PeerAddress            = ::chip::Transport::PeerAddress;
     using IPAddress              = ::chip::Inet::IPAddress;
-    using PacketBuffer           = ::chip::System::PacketBuffer;
     using PacketBufferHandle     = ::chip::System::PacketBufferHandle;
     using NodeId                 = ::chip::NodeId;
 

src/app/Command.cpp

@@ -148,7 +148,7 @@ chip::TLV::TLVWriter & Command::CreateCommandDataElementTLVWriter()
     mCommandDataBuf = chip::System::PacketBuffer::New();
     if (mCommandDataBuf.IsNull())
     {
-        ChipLogDetail(DataManagement, "Unable to allocate PacketBuffer");
+        ChipLogDetail(DataManagement, "Unable to allocate packet buffer");
     }
 
     mCommandDataWriter.Init(mCommandDataBuf.Retain());

src/app/encoder.cpp

@@ -77,7 +77,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
     PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize);                                               \
     if (payload.IsNull())                                                                                                          \
     {                                                                                                                              \
-        ChipLogError(Zcl, "Could not allocate PacketBuffer while trying to encode %s command", kName);                             \
+        ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName);                            \
         return payload;                                                                                                            \
     }                                                                                                                              \
                                                                                                                                    \

src/app/util/chip-message-send.cpp

@@ -23,7 +23,7 @@
 #include "chip-message-send.h"
 
 #include <assert.h>
-#include <inet/InetLayer.h> // PacketBuffer and the like
+#include <inet/InetLayer.h>
 #include <support/logging/CHIPLogging.h>
 #include <transport/SecureSessionMgr.h> // For SecureSessionMgr
 

src/app/zap-templates/templates/chip/encoder-src.zapt

@@ -63,7 +63,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
     PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize);                                               \
     if (payload.IsNull())                                                                                                          \
     {                                                                                                                              \
-        ChipLogError(Zcl, "Could not allocate PacketBuffer while trying to encode %s command", kName);                             \
+        ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName);                            \
         return payload;                                                                                                            \
     }                                                                                                                              \
                                                                                                                                    \

src/ble/BLEEndPoint.cpp

@@ -115,7 +115,7 @@ BLE_ERROR BLEEndPoint::StartConnect()
     mState = kState_Connecting;
 
     // Build BLE transport protocol capabilities request.
-    buf = PacketBuffer::New();
+    buf = System::PacketBuffer::New();
     VerifyOrExit(!buf.IsNull(), err = BLE_ERROR_NO_MEMORY);
 
     // Zero-initialize BLE transport capabilities request.
@@ -140,9 +140,8 @@ BLE_ERROR BLEEndPoint::StartConnect()
 
     // Send BLE transport capabilities request to peripheral via GATT write.
     // Add reference to message fragment for duration of platform's GATT write attempt. CHIP retains partial
-    // ownership of message fragment's PacketBuffer, since this is the same buffer as that of the whole message, just
+    // ownership of message fragment's packet buffer, since this is the same buffer as that of the whole message, just
     // with a fragmenter-modified payload offset and data length, by a Retain() on the handle when calling this function.
-    // Buffer must be decref'd (i.e. PacketBuffer::Free'd) by platform when BLE GATT operation completes.
     if (!SendWrite(buf.Retain()))
     {
         err = BLE_ERROR_GATT_WRITE_FAILED;
@@ -224,9 +223,8 @@ void BLEEndPoint::HandleSubscribeReceived()
     VerifyOrExit(mBtpEngine.PopPacketTag(mSendQueue) == kType_Data, err = BLE_ERROR_INVALID_BTP_HEADER_FLAGS);
 #endif
     // Add reference to message fragment for duration of platform's GATT indication attempt. CHIP retains partial
-    // ownership of message fragment's PacketBuffer, since this is the same buffer as that of the whole message, just
-    // with a fragmenter-modified payload offset and data length. Buffer must be decref'd (i.e. PacketBuffer::Free'd) by
-    // platform when BLE GATT operation completes.
+    // ownership of message fragment's packet buffer, since this is the same buffer as that of the whole message, just
+    // with a fragmenter-modified payload offset and data length.
     if (!SendIndication(mSendQueue.Retain()))
     {
         // Ensure transmit queue is empty and set to NULL.
@@ -380,11 +378,6 @@ void BLEEndPoint::FinalizeClose(uint8_t oldState, uint8_t flags, BLE_ERROR err)
     mSendQueue = nullptr;
     QueueTxUnlock();
 
-#if CHIP_ENABLE_CHIPOBLE_TEST
-    PacketBuffer::Free(mBtpEngineTest.mCommandReceiveQueue);
-    mBtpEngineTest.mCommandReceiveQueue = NULL;
-#endif
-
     // Fire application's close callback if we haven't already, and it's not suppressed.
     if (oldState != kState_Closing && (flags & kBleCloseFlag_SuppressCallback) == 0)
     {
@@ -660,7 +653,7 @@ BLE_ERROR BLEEndPoint::Send(PacketBufferHandle data)
     VerifyOrExit(!data.IsNull(), err = BLE_ERROR_BAD_ARGS);
     VerifyOrExit(IsConnected(mState), err = BLE_ERROR_INCORRECT_STATE);
 
-    // Ensure outgoing message fits in a single contiguous PacketBuffer, as currently required by the
+    // Ensure outgoing message fits in a single contiguous packet buffer, as currently required by the
     // message fragmentation and reassembly engine.
     if (data->HasChainedBuffer())
     {
@@ -886,7 +879,7 @@ BLE_ERROR BLEEndPoint::HandleFragmentConfirmationReceived()
     // Ensure we're in correct state to receive confirmation of non-handshake GATT send.
     VerifyOrExit(IsConnected(mState), err = BLE_ERROR_INCORRECT_STATE);
 
-    // TODO PacketBuffer high water mark optimization: if ack pending, but fragmenter state == complete, free fragmenter's
+    // TODO Packet buffer high water mark optimization: if ack pending, but fragmenter state == complete, free fragmenter's
     // tx buf before sending ack.
 
     if (GetFlag(mConnStateFlags, kConnState_StandAloneAckInFlight))
@@ -952,7 +945,7 @@ BLE_ERROR BLEEndPoint::DriveStandAloneAck()
     // If stand-alone ack not already pending, allocate new payload buffer here.
     if (mAckToSend.IsNull())
     {
-        mAckToSend = PacketBuffer::New();
+        mAckToSend = System::PacketBuffer::New();
         VerifyOrExit(!mAckToSend.IsNull(), err = BLE_ERROR_NO_MEMORY);
     }
 
@@ -1094,7 +1087,7 @@ BLE_ERROR BLEEndPoint::HandleCapabilitiesRequestReceived(PacketBufferHandle data
     err = BleTransportCapabilitiesRequestMessage::Decode(data, req);
     SuccessOrExit(err);
 
-    responseBuf = PacketBuffer::New();
+    responseBuf = System::PacketBuffer::New();
     VerifyOrExit(!responseBuf.IsNull(), err = BLE_ERROR_NO_MEMORY);
 
     // Determine BLE connection's negotiated ATT MTU, if possible.
@@ -1417,7 +1410,7 @@ BLE_ERROR BLEEndPoint::Receive(PacketBufferHandle data)
     // If we've reassembled a whole message...
     if (mBtpEngine.RxState() == BtpEngine::kState_Complete)
     {
-        // Take ownership of message PacketBuffer
+        // Take ownership of message buffer
         System::PacketBufferHandle full_packet = mBtpEngine.TakeRxPacket();
 
         ChipLogDebugBleEndPoint(Ble, "reassembled whole msg, len = %d", full_packet->DataLength());

src/ble/BleLayer.h

@@ -267,12 +267,10 @@ class DLL_EXPORT BleLayer
      *
      *     Beyond each call, no guarantees are provided as to the lifetime of UUID arguments.
      *
-     *     A 'true' return value means the CHIP stack successfully handled the
-     *     corresponding message or state indication. 'false' means the CHIP stack either
-     *     failed or chose not to handle this. In case of 'false,' the CHIP stack will not
-     *     have freed or taken ownership of any PacketBuffer arguments. This contract allows the
-     *     platform to pass BLE events to CHIP without needing to know which characteristics
-     *     CHIP cares about.
+     *     A 'true' return value means the CHIP stack successfully handled the corresponding message
+     *     or state indication. 'false' means the CHIP stack either failed or chose not to handle this.
+     *     This contract allows the platform to pass BLE events to CHIP without needing to know which
+     *     characteristics CHIP cares about.
 
      *     Platform must call this function when a GATT subscription has been established to any CHIP service
      *     charateristic.

src/ble/BlePlatformDelegate.h

@@ -58,22 +58,9 @@ class DLL_EXPORT BlePlatformDelegate
     virtual uint16_t GetMTU(BLE_CONNECTION_OBJECT connObj) const = 0;
 
     // Data path calling convention:
-    //   The CHIP stack retains partial ownership of pBufs sent via the below functions. These buffers are freed by
-    //   CHIP after either they're acknowledged by the peer's BLE controller, or CHIP shuts down the pBuf's
-    //   associated BLEEndPoint.
-    //
-    //   For its part, the platform MUST call PacketBuffer::Free on each pBuf it receives via a Send* function once it no
-    //   longer requires a reference to this buffer, e.g. when a CHIP_CLIENT_EVENT_BLE_PBUF_CLEAR event is received on
-    //   platforms with the CHIP BLE SDK.
-    //
-    //   On platforms such as iOS or Android where the contents of the pBuf PacketBuffer are copied into a separate
-    //   buffer for transmission, pBuf may be freed on the downcall to the platform delegate once the copy completes.
-    //
     //   A 'true' return value from a Send* function indicates that the characteristic was written or updated
     //   successfully. A 'false' value indicates failure, and is used to report this failure to the user via the return
     //   value of chipConnection::SendMessage.
-    //
-    //   If a Send* function returns false, it must release its reference to pBuf prior to return.
 
     // Send GATT characteristic indication request
     virtual bool SendIndication(BLE_CONNECTION_OBJECT connObj, const ChipBleUUID * svcId, const ChipBleUUID * charId,

src/ble/BtpEngine.cpp

@@ -335,7 +335,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat
         data->ConsumeHead(startReader.OctetsRead());
 
         // Create a new buffer for use as the Rx re-assembly area.
-        mRxBuf = PacketBuffer::New();
+        mRxBuf = System::PacketBuffer::New();
 
         VerifyOrExit(!mRxBuf.IsNull(), err = BLE_ERROR_NO_MEMORY);
 
@@ -367,7 +367,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat
 
     if (rx_flags & kHeaderFlag_EndMessage)
     {
-        // Trim remainder, if any, of received PacketBuffer based on sender-specified length of reassembled message.
+        // Trim remainder, if any, of the received packet buffer based on sender-specified length of reassembled message.
         int padding = mRxBuf->DataLength() - mRxLength;
 
         if (padding > 0)

src/ble/BtpEngine.h

@@ -43,7 +43,6 @@
 namespace chip {
 namespace Ble {
 
-using ::chip::System::PacketBuffer;
 using ::chip::System::PacketBufferHandle;
 
 typedef uint8_t SequenceNumber_t; // If type changed from uint8_t, adjust assumptions in BtpEngine::IsValidAck and

src/controller/python/ChipDeviceController-BlePlatformDelegate.cpp

@@ -99,7 +99,7 @@ bool DeviceController_BlePlatformDelegate::SendWriteRequest(BLE_CONNECTION_OBJEC
 
     // Going out of scope releases delegate's reference to pBuf. pBuf will be freed when both platform delegate and Chip stack free
     // their references to it. We release pBuf's reference here since its payload bytes were copied into a new NSData object by
-    // ChipBleMgr.py's writeCB, and in both the error and succees cases this code has no further use for the pBuf PacketBuffer.
+    // ChipBleMgr.py's writeCB, and in both the error and succees cases this code has no further use for it.
     return ret;
 }
 

src/inet/IPEndPointBasis.cpp

@@ -99,8 +99,6 @@
 namespace chip {
 namespace Inet {
 
-using chip::System::PacketBuffer;
-
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
 union PeerSockAddr
 {
@@ -678,12 +676,12 @@ IPPacketInfo * IPEndPointBasis::GetPacketInfo(const System::PacketBufferHandle &
 System::Error IPEndPointBasis::PostPacketBufferEvent(chip::System::Layer & aLayer, System::Object & aTarget,
                                                      System::EventType aEventType, System::PacketBufferHandle aBuffer)
 {
-    System::PacketBuffer * buf = aBuffer.Release_ForNow();
-    System::Error error        = aLayer.PostEvent(aTarget, aEventType, (uintptr_t) buf);
-    if (error != INET_NO_ERROR)
+    System::Error error =
+        aLayer.PostEvent(aTarget, aEventType, (uintptr_t) System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(aBuffer));
+    if (error == INET_NO_ERROR)
     {
-        // If PostEvent() failed, it has not taken ownership of the buffer, so we need to retake it so that it will be freed.
-        (void) System::PacketBufferHandle::Adopt(buf);
+        // If PostEvent() succeeded, it has ownership of the buffer, so we need to release it (without freeing it).
+        static_cast<void>(std::move(aBuffer).UnsafeRelease());
     }
     return error;
 }
@@ -1070,7 +1068,7 @@ void IPEndPointBasis::HandlePendingIO(uint16_t aPort)
     lPacketInfo.Clear();
     lPacketInfo.DestPort = aPort;
 
-    lBuffer = PacketBuffer::New(0);
+    lBuffer = System::PacketBuffer::New(0);
 
     if (!lBuffer.IsNull())
     {

src/inet/RawEndPoint.cpp

@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2020 Project CHIP Authors
+ *    Copyright (c) 2020-2021 Project CHIP Authors
  *    Copyright (c) 2018 Google LLC.
  *    Copyright (c) 2013-2018 Nest Labs, Inc.
  *
@@ -74,8 +74,6 @@
 namespace chip {
 namespace Inet {
 
-using chip::System::PacketBuffer;
-
 chip::System::ObjectPool<RawEndPoint, INET_CONFIG_NUM_RAW_ENDPOINTS> RawEndPoint::sPool;
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
@@ -624,20 +622,20 @@ INET_ERROR RawEndPoint::SendMsg(const IPPacketInfo * pktInfo, chip::System::Pack
 #if LWIP_VERSION_MAJOR > 1 || LWIP_VERSION_MINOR >= 5
         ip_addr_t ipAddr = addr.ToLwIPAddr();
 
-        lwipErr = raw_sendto(mRaw, msg.GetLwIPpbuf(), &ipAddr);
+        lwipErr = raw_sendto(mRaw, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &ipAddr);
 #else // LWIP_VERSION_MAJOR <= 1 && LWIP_VERSION_MINOR < 5
         if (PCB_ISIPV6(mRaw))
         {
             ip6_addr_t ipv6Addr = addr.ToIPv6();
 
-            lwipErr = raw_sendto_ip6(mRaw, msg.GetLwIPpbuf(), &ipv6Addr);
+            lwipErr = raw_sendto_ip6(mRaw, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &ipv6Addr);
         }
 #if INET_CONFIG_ENABLE_IPV4
         else
         {
             ip4_addr_t ipv4Addr = addr.ToIPv4();
 
-            lwipErr = raw_sendto(mRaw, msg.GetLwIPpbuf(), &ipv4Addr);
+            lwipErr = raw_sendto(mRaw, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &ipv4Addr);
         }
 #endif // INET_CONFIG_ENABLE_IPV4
 #endif // LWIP_VERSION_MAJOR <= 1 || LWIP_VERSION_MINOR >= 5

src/inet/TCPEndPoint.cpp

@@ -109,8 +109,6 @@ err_t start_tcp_timers(void)
 namespace chip {
 namespace Inet {
 
-using chip::System::PacketBuffer;
-
 chip::System::ObjectPool<TCPEndPoint, INET_CONFIG_NUM_TCP_ENDPOINTS> TCPEndPoint::sPool;
 
 INET_ERROR TCPEndPoint::Bind(IPAddressType addrType, const IPAddress & addr, uint16_t port, bool reuseAddr)
@@ -2404,12 +2402,12 @@ void TCPEndPoint::ReceiveData()
     bool isNewBuf = true;
 
     if (mRcvQueue.IsNull())
-        rcvBuf = PacketBuffer::New(0);
+        rcvBuf = System::PacketBuffer::New(0);
     else
     {
         rcvBuf = mRcvQueue->Last();
         if (rcvBuf->AvailableDataLength() == 0)
-            rcvBuf = PacketBuffer::New(0);
+            rcvBuf = System::PacketBuffer::New(0);
         else
         {
             isNewBuf = false;

src/inet/TCPEndPoint.h

@@ -316,7 +316,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis
      *  This method may only be called by data reception event handlers to
      *  put an unacknowledged portion of data back on the receive queue. The
      *  operational semantics are undefined if the caller is outside the scope
-     *  of a data reception event handler, \c data is not the \c chip::System::PacketBuffer
+     *  of a data reception event handler, \c data is not the packet buffer
      *  provided to the handler, or \c data does not contain the unacknowledged
      *  portion remaining after the bytes acknowledged by a prior call to the
      *  <tt>AckReceive(uint16_t len)</tt> method.

src/inet/UDPEndPoint.cpp

@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2020 Project CHIP Authors
+ *    Copyright (c) 2020-2021 Project CHIP Authors
  *    Copyright (c) 2018 Google LLC.
  *    Copyright (c) 2013-2018 Nest Labs, Inc.
  *
@@ -76,8 +76,6 @@
 namespace chip {
 namespace Inet {
 
-using chip::System::PacketBuffer;
-
 chip::System::ObjectPool<UDPEndPoint, INET_CONFIG_NUM_UDP_ENDPOINTS> UDPEndPoint::sPool;
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
@@ -578,9 +576,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
         }
 
         if (intfId != INET_NULL_INTERFACEID)
-            lwipErr = udp_sendto_if(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
+            lwipErr = udp_sendto_if(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort, intfId);
         else
-            lwipErr = udp_sendto(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
+            lwipErr = udp_sendto(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort);
 
         ip_addr_copy(mUDP->local_ip, boundAddr);
 
@@ -600,9 +598,10 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
             }
 
             if (intfId != INET_NULL_INTERFACEID)
-                lwipErr = udp_sendto_if_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
+                lwipErr =
+                    udp_sendto_if_ip6(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort, intfId);
             else
-                lwipErr = udp_sendto_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
+                lwipErr = udp_sendto_ip6(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort);
         }
 
 #if INET_CONFIG_ENABLE_IPV4
@@ -619,9 +618,10 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
             }
 
             if (intfId != INET_NULL_INTERFACEID)
-                lwipErr = udp_sendto_if(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
+                lwipErr =
+                    udp_sendto_if(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort, intfId);
             else
-                lwipErr = udp_sendto(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
+                lwipErr = udp_sendto(mUDP, System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(msg), &lwipDestAddr, destPort);
         }
 
         ipX_addr_copy(mUDP->local_ip, boundAddr);

src/inet/tests/TestInetEndPoint.cpp

@@ -539,8 +539,6 @@ static const nlTest sTests[] = { NL_TEST_DEF("InetEndPoint::PreTest", TestInetPr
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
 /**
  *  Set up the test suite.
- *  This is a work-around to initiate PacketBuffer protected class instance's
- *  data and set it to a known state, before an instance is created.
  */
 static int TestSetup(void * inContext)
 {