[haiku-commits] r38484 - haiku/trunk/src/servers/app

From: superstippi@xxxxxx
To: haiku-commits@xxxxxxxxxxxxx
Date: Wed, 1 Sep 2010 00:07:50 +0200 (CEST)
Author: stippi
Date: 2010-09-01 00:07:50 +0200 (Wed, 01 Sep 2010)
New Revision: 38484
Changeset: http://dev.haiku-os.org/changeset/38484

Modified:
   haiku/trunk/src/servers/app/ServerApp.cpp
   haiku/trunk/src/servers/app/ServerPicture.cpp
   haiku/trunk/src/servers/app/ServerPicture.h
Log:
* 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.


Modified: haiku/trunk/src/servers/app/ServerApp.cpp
===================================================================
--- haiku/trunk/src/servers/app/ServerApp.cpp   2010-08-31 20:32:50 UTC (rev 
38483)
+++ haiku/trunk/src/servers/app/ServerApp.cpp   2010-08-31 22:07:50 UTC (rev 
38484)
@@ -197,7 +197,7 @@
                _DeleteBitmap(fBitmapMap.begin()->second);
 
        while (!fPictureMap.empty())
-               RemovePicture(fPictureMap.begin()->second);
+               fPictureMap.begin()->second->SetOwner(NULL);
 
        fDesktop->GetCursorManager().DeleteCursors(fClientTeam);
 
@@ -453,11 +453,14 @@
 }
 
 
+/*! 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 @@
 }
 
 
+/*! 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 @@
                                        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 @@
 
                                ServerPicture* picture = _FindPicture(token);
                                if (picture != NULL)
-                                       RemovePicture(picture);
+                                       picture->SetOwner(NULL);
                        }
                        break;
                }

Modified: haiku/trunk/src/servers/app/ServerPicture.cpp
===================================================================
--- haiku/trunk/src/servers/app/ServerPicture.cpp       2010-08-31 20:32:50 UTC 
(rev 38483)
+++ haiku/trunk/src/servers/app/ServerPicture.cpp       2010-08-31 22:07:50 UTC 
(rev 38484)
@@ -931,16 +931,27 @@
 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;
+       fOwner = NULL;
+       if (owner == NULL)
                return true;
-       }
 
-       fOwner = NULL;
-       return false;
+       if (!owner->AddPicture(this))
+               return false;
+
+       fOwner = owner;
+       return true;
 }
 
 
@@ -1107,6 +1118,7 @@
        if (fPictures == NULL || !fPictures->AddItem(picture))
                return false;
 
+       picture->AcquireReference();
        return true;
 }
 

Modified: haiku/trunk/src/servers/app/ServerPicture.h
===================================================================
--- haiku/trunk/src/servers/app/ServerPicture.h 2010-08-31 20:32:50 UTC (rev 
38483)
+++ haiku/trunk/src/servers/app/ServerPicture.h 2010-08-31 22:07:50 UTC (rev 
38484)
@@ -38,6 +38,7 @@
 
                        int32                           Token() { return 
fToken; }
                        bool                            SetOwner(ServerApp* 
owner);
+                       ServerApp*                      Owner() const { return 
fOwner; }
 
                        bool                            
ReleaseClientReference();
[haiku-commits] r38484 - haiku/trunk/src/servers/app

Other related posts:
