* 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

@@ -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;
 		}

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;
 }
 

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();