Media Kit: BBufferCache: if not reclaimed, only mark the buffer for deletion

hrev53379 clears the buffer cache for disconnected clients, and also delete buffers.
This is too early (see #15263, media_addon_server crash), and should only happen
after the buffer is recycled. This can be resolved by abusing the fFlags field of
BBuffer to mark the buffer for deletion, and mark the buffer to be reclaimed.
Some BBuffers don't reside in the SharedBufferList, so we have to mark them as to
be reclaimed. For those in the SharedBufferList, call a new RemoveBuffer(), which
can check whether the buffer is still to be reclaimed. For reclaimed BBuffers,
delete them right away, others can be marked for deletion.
fixes #15606 #15263, possibly #15433

Change-Id: I66e94138e7e10a40d4c48e2ac042f816c79f5aab
Reviewed-on: https://review.haiku-os.org/c/haiku/+/2245
Reviewed-by: Adrien Destugues <pulkomandy@gmail.com>
Reviewed-by: X512 <danger_mail@list.ru>
Reviewed-by: Ryan Leavengood <leavengood@gmail.com>

headers/private/media/MediaMisc.h

@@ -44,6 +44,10 @@ progress_shutdown(int stage,
 #define MEDIA_SERVER_PORT_NAME			"__media_server_port"
 #define MEDIA_ADDON_SERVER_PORT_NAME	"__media_addon_server_port"
 
+#define BUFFER_TO_RECLAIM				0x20000000
+#define BUFFER_MARKED_FOR_DELETION		0x40000000
+
+
 extern const char *B_MEDIA_ADDON_SERVER_SIGNATURE;
 
 namespace BPrivate { namespace media {

headers/private/media/SharedBufferList.h

@@ -30,6 +30,7 @@ public:
 			status_t					AddBuffer(sem_id groupReclaimSem,
 											const buffer_clone_info& info,
 											BBuffer** buffer);
+			status_t					RemoveBuffer(BBuffer* buffer);
 
 			// Call AddBuffer and CheckID locked
 			status_t					AddBuffer(sem_id groupReclaimSem,

src/kits/media/Buffer.cpp

@@ -38,6 +38,7 @@
 #include <MediaDefs.h>
 
 #include "MediaDebug.h"
+#include "MediaMisc.h"
 #include "DataExchange.h"
 #include "SharedBufferList.h"
 
@@ -114,7 +115,11 @@ BBuffer::Recycle()
 	CALLED();
 	if (fBufferList == NULL)
 		return;
-	fBufferList->RecycleBuffer(this);
+	fFlags &= ~BUFFER_TO_RECLAIM;
+	if ((fFlags & BUFFER_MARKED_FOR_DELETION) != 0)
+		delete this;
+	else
+		fBufferList->RecycleBuffer(this);
 }
 
 

src/kits/media/BufferCache.cpp

@@ -14,6 +14,8 @@
 #include <Buffer.h>
 
 #include "MediaDebug.h"
+#include "MediaMisc.h"
+#include "SharedBufferList.h"
 
 
 namespace BPrivate {
@@ -41,8 +43,10 @@ BufferCache::GetBuffer(media_buffer_id id, port_id port)
 		return NULL;
 
 	buffer_cache_entry* existing;
-	if (fMap.Get(id, existing))
+	if (fMap.Get(id, existing)) {
+		existing->buffer->fFlags |= BUFFER_TO_RECLAIM;
 		return existing->buffer;
+	}
 
 	buffer_clone_info info;
 	info.buffer = id;
@@ -65,6 +69,7 @@ BufferCache::GetBuffer(media_buffer_id id, port_id port)
 		return NULL;
 	}
 
+	buffer->fFlags |= BUFFER_TO_RECLAIM;
 	return buffer;
 }
 
@@ -76,8 +81,17 @@ BufferCache::FlushCacheForPort(port_id port)
 	while (iterator.HasNext()) {
 		BufferMap::Entry entry = iterator.Next();
 		if (entry.value.port == port) {
-			// Delete the buffer
-			delete entry.value.buffer;
+			BBuffer* buffer = entry.value.buffer;
+			bool isReclaimed = (buffer->fFlags & BUFFER_TO_RECLAIM) == 0;
+			if (isReclaimed && buffer->fBufferList != NULL)
+				isReclaimed = buffer->fBufferList->RemoveBuffer(buffer) == B_OK;
+
+			if (isReclaimed)
+				delete buffer;
+			else {
+				// mark the buffer for deletion
+				buffer->fFlags |= BUFFER_MARKED_FOR_DELETION;
+			}
 			// Then remove it from the map
 			fMap.Remove(iterator);
 		}

src/kits/media/SharedBufferList.cpp

@@ -378,6 +378,48 @@ SharedBufferList::RecycleBuffer(BBuffer* buffer)
 }
 
 
+status_t
+SharedBufferList::RemoveBuffer(BBuffer* buffer)
+{
+	CALLED();
+
+	media_buffer_id id = buffer->ID();
+
+	if (Lock() != B_OK)
+		return B_ERROR;
+
+	int32 notRemovedCount = 0;
+
+	for (int32 i = 0; i < fCount; i++) {
+		// find the buffer id, and remove it in all groups it belongs to
+		if (fInfos[i].id == id) {
+			if (!fInfos[i].reclaimed) {
+				notRemovedCount++;
+				ERROR("SharedBufferList::RequestBuffer, BBuffer %p, id = %"
+					B_PRId32 " not reclaimed\n", buffer, id);
+				DEBUG_ONLY(debugger("buffer not reclaimed"));
+				continue;
+			}
+			fInfos[i].buffer = NULL;
+			fInfos[i].id = -1;
+			fInfos[i].reclaim_sem = -1;
+		}
+	}
+
+	if (Unlock() != B_OK)
+		return B_ERROR;
+
+	if (notRemovedCount != 0) {
+		ERROR("SharedBufferList::RemoveBuffer, BBuffer %p, id = %" B_PRId32
+			" not reclaimed\n", buffer, id);
+		return B_ERROR;
+	}
+
+	return B_OK;
+}
+
+
+
 /*!	Returns exactly \a bufferCount buffers from the group specified via its
 	\a groupReclaimSem if successful.
 */