From 4177bfe69fb9f3fddefcf3776fa13744c4b0cf31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= <superstippi@gmx.de>
Date: Tue, 31 Aug 2010 22:07:50 +0000
Subject: [PATCH] * AppendPicture() and RemovePicture() are weird, since   the
 ServerPicture calls those itself from SetOwner().   Since there are asserts
 in ServerPicture about fOwner,   it was easiest to fix the code by using
 *only* SetOwner()   from within ServerApp to add or remove pictures. *
 SetOwner() was broken, since it called a method which   potentially removed
 the last reference and then still   accessed memory of the now free'd
 ServerPicture instance.   The easiest fix is to just increase the reference
 count   temporarily. * SetOwner() wrongly returned false when the new owner
 was NULL. * NestPicture() should simply add it's own reference. There   are
 two places where it is called, and only one of them   added the extra
 reference. The other one only acquired the   implicit reference that the
 ServerApp owns, but pictures   that remove nested children remove a reference
 from them.   This could leave stale pointers around of course. * Added more
 asserts about fOwner.

git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@38484 a95241bf-73f2-0310-859d-f6bbb57e9c96
---
 src/servers/app/ServerApp.cpp     | 13 +++++++++----
 src/servers/app/ServerPicture.cpp | 24 ++++++++++++++++++------
 src/servers/app/ServerPicture.h   |  1 +
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/src/servers/app/ServerApp.cpp b/src/servers/app/ServerApp.cpp
index 96b21d5211..8892b55619 100644
--- a/src/servers/app/ServerApp.cpp
+++ b/src/servers/app/ServerApp.cpp
@@ -197,7 +197,7 @@ ServerApp::~ServerApp()
 		_DeleteBitmap(fBitmapMap.begin()->second);
 
 	while (!fPictureMap.empty())
-		RemovePicture(fPictureMap.begin()->second);
+		fPictureMap.begin()->second->SetOwner(NULL);
 
 	fDesktop->GetCursorManager().DeleteCursors(fClientTeam);
 
@@ -453,11 +453,14 @@ ServerApp::GetPicture(int32 token) const
 }
 
 
+/*! To be called only by ServerPicture itself.*/
 bool
 ServerApp::AddPicture(ServerPicture* picture)
 {
 	BAutolock _(fMapLocker);
 
+	ASSERT(picture->Owner() == NULL);
+
 	try {
 		fPictureMap.insert(std::make_pair(picture->Token(), picture));
 	} catch (std::bad_alloc& exception) {
@@ -468,11 +471,14 @@ ServerApp::AddPicture(ServerPicture* picture)
 }
 
 
+/*! To be called only by ServerPicture itself.*/
 void
 ServerApp::RemovePicture(ServerPicture* picture)
 {
 	BAutolock _(fMapLocker);
 
+	ASSERT(picture->Owner() == this);
+
 	fPictureMap.erase(picture->Token());
 	picture->ReleaseReference();
 }
@@ -834,8 +840,7 @@ ServerApp::_DispatchMessage(int32 code, BPrivate::LinkReceiver& link)
 					int32 token = -1;
 					link.Read<int32>(&token);
 
-					// TODO: do we actually need another reference here?
-					if (ServerPicture* subPicture = GetPicture(token))
+					if (ServerPicture* subPicture = _FindPicture(token))
 						picture->NestPicture(subPicture);
 				}
 				status = picture->ImportData(link);
@@ -859,7 +864,7 @@ ServerApp::_DispatchMessage(int32 code, BPrivate::LinkReceiver& link)
 
 				ServerPicture* picture = _FindPicture(token);
 				if (picture != NULL)
-					RemovePicture(picture);
+					picture->SetOwner(NULL);
 			}
 			break;
 		}
diff --git a/src/servers/app/ServerPicture.cpp b/src/servers/app/ServerPicture.cpp
index 8d41ad80bc..05746503f7 100644
--- a/src/servers/app/ServerPicture.cpp
+++ b/src/servers/app/ServerPicture.cpp
@@ -931,16 +931,27 @@ ServerPicture::~ServerPicture()
 bool
 ServerPicture::SetOwner(ServerApp* owner)
 {
+	if (owner == fOwner)
+		return true;
+
+	// Acquire an extra reference, since calling RemovePicture()
+	// May remove the last reference and then we will self-destruct right then.
+	// Setting fOwner to NULL would access free'd memory. If owner is another
+	// ServerApp, it's expected to already have a reference of course.
+	Reference<ServerPicture> _(this);
+
 	if (fOwner != NULL)
 		fOwner->RemovePicture(this);
 
-	if (owner != NULL && owner->AddPicture(this)) {
-		fOwner = owner;
-		return true;
-	}
-
 	fOwner = NULL;
-	return false;
+	if (owner == NULL)
+		return true;
+
+	if (!owner->AddPicture(this))
+		return false;
+
+	fOwner = owner;
+	return true;
 }
 
 
@@ -1107,6 +1118,7 @@ ServerPicture::NestPicture(ServerPicture* picture)
 	if (fPictures == NULL || !fPictures->AddItem(picture))
 		return false;
 
+	picture->AcquireReference();
 	return true;
 }
 
diff --git a/src/servers/app/ServerPicture.h b/src/servers/app/ServerPicture.h
index 7ad2225f02..92416202e9 100644
--- a/src/servers/app/ServerPicture.h
+++ b/src/servers/app/ServerPicture.h
@@ -38,6 +38,7 @@ public:
 
 			int32				Token() { return fToken; }
 			bool				SetOwner(ServerApp* owner);
+			ServerApp*			Owner() const { return fOwner; }
 
 			bool				ReleaseClientReference();