Author: axeld Date: 2010-02-15 15:50:53 +0100 (Mon, 15 Feb 2010) New Revision: 35472 Changeset: http://dev.haiku-os.org/changeset/35472/haiku Ticket: http://dev.haiku-os.org/ticket/4709 Modified: haiku/trunk/src/servers/app/ServerApp.cpp haiku/trunk/src/servers/app/ServerApp.h haiku/trunk/src/servers/app/ServerBitmap.cpp haiku/trunk/src/servers/app/ServerBitmap.h haiku/trunk/src/servers/app/ServerPicture.cpp haiku/trunk/src/servers/app/ServerPicture.h haiku/trunk/src/servers/app/ServerWindow.cpp Log: * Fixed race conditions in the server's bitmap/picture handling: the objects are now removed from the maps as soon as the client deletes them. This also makes the "client reference" mechanism superfluous I introduced earlier. * ServerApp::SetCurrentCursor() must always call Desktop::SetCursor(), since it is also called whenever the current application changes. This fixes the cursor almost never changing. * Renamed ServerPicture::Usurp()/StepDown() to PushPicture(), and PopPicture(). * Also, they now acquire a reference to the picture in question (ie. the picture you get from PopPicture() also owns a reference you need to free). * ServerApp::CreatePicture() may fail, too. This case is now handled in the code that calls it. * Previously, the ServerWindow tried to process up to 70 messages in one go. That obviously caused bug #4709. Now, we have the additional requirement to not hold the desktop lock for longer than 25 ms. I haven't tested it with Kaleidoscope yet, though. Modified: haiku/trunk/src/servers/app/ServerApp.cpp =================================================================== --- haiku/trunk/src/servers/app/ServerApp.cpp 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerApp.cpp 2010-02-15 14:50:53 UTC (rev 35472) @@ -191,20 +191,12 @@ fMapLocker.Lock(); - while (!fBitmapMap.empty()) { - ServerBitmap* bitmap = fBitmapMap.begin()->second; + while (!fBitmapMap.empty()) + _DeleteBitmap(fBitmapMap.begin()->second); - fBitmapMap.erase(fBitmapMap.begin()); - bitmap->ReleaseReference(); - } + while (!fPictureMap.empty()) + RemovePicture(fPictureMap.begin()->second); - while (!fPictureMap.empty()) { - ServerPicture* picture = fPictureMap.begin()->second; - - fPictureMap.erase(fPictureMap.begin()); - picture->ReleaseReference(); - } - fDesktop->GetCursorManager().DeleteCursors(fClientTeam); STRACE(("ServerApp %s::~ServerApp(): Exiting\n", Signature())); @@ -298,17 +290,16 @@ void ServerApp::SetCurrentCursor(ServerCursor* cursor) { - if (fViewCursor == cursor) - return; + if (fViewCursor != cursor) { + if (fViewCursor) + fViewCursor->ReleaseReference(); - if (fViewCursor) - fViewCursor->ReleaseReference(); + fViewCursor = cursor; - fViewCursor = cursor; + if (fViewCursor) + fViewCursor->AcquireReference(); + } - if (fViewCursor) - fViewCursor->AcquireReference(); - fDesktop->SetCursor(CurrentCursor()); } @@ -419,29 +410,6 @@ } -bool -ServerApp::BitmapAdded(ServerBitmap* bitmap) -{ - BAutolock _(fMapLocker); - - try { - fBitmapMap.insert(std::make_pair(bitmap->Token(), bitmap)); - } catch (std::bad_alloc& exception) { - return false; - } - - return true; -} - - -void -ServerApp::BitmapRemoved(ServerBitmap* bitmap) -{ - BAutolock _(fMapLocker); - fBitmapMap.erase(bitmap->Token()); -} - - ServerPicture* ServerApp::CreatePicture(const ServerPicture* original) { @@ -477,7 +445,7 @@ bool -ServerApp::PictureAdded(ServerPicture* picture) +ServerApp::AddPicture(ServerPicture* picture) { BAutolock _(fMapLocker); @@ -492,10 +460,12 @@ void -ServerApp::PictureRemoved(ServerPicture* picture) +ServerApp::RemovePicture(ServerPicture* picture) { BAutolock _(fMapLocker); + fPictureMap.erase(picture->Token()); + picture->ReleaseReference(); } @@ -743,7 +713,7 @@ STRACE(("ServerApp %s: Create Bitmap (%.1fx%.1f)\n", Signature(), frame.Width() + 1, frame.Height() + 1)); - if (bitmap != NULL && bitmap->SetOwner(this)) { + if (bitmap != NULL && _AddBitmap(bitmap)) { fLink.StartMessage(B_OK); fLink.Attach<int32>(bitmap->Token()); fLink.Attach<uint8>(allocationFlags); @@ -770,8 +740,6 @@ STRACE(("ServerApp %s: received BBitmap delete request\n", Signature())); - // Delete a bitmap's allocated memory - // Attached Data: // 1) int32 token int32 token; @@ -784,7 +752,7 @@ STRACE(("ServerApp %s: Deleting Bitmap %ld\n", Signature(), token)); - bitmap->ReleaseClientReference(); + _DeleteBitmap(bitmap); } fMapLocker.Unlock(); @@ -833,6 +801,8 @@ break; } + // Picture ops + case AS_CREATE_PICTURE: { // TODO: Maybe rename this to AS_UPLOAD_PICTURE ? @@ -872,7 +842,7 @@ ServerPicture* picture = _FindPicture(token); if (picture != NULL) - picture->ReleaseClientReference(); + RemovePicture(picture); } break; } @@ -3199,6 +3169,34 @@ } +bool +ServerApp::_AddBitmap(ServerBitmap* bitmap) +{ + BAutolock _(fMapLocker); + + try { + fBitmapMap.insert(std::make_pair(bitmap->Token(), bitmap)); + } catch (std::bad_alloc& exception) { + return false; + } + + bitmap->SetOwner(this); + return true; +} + + +void +ServerApp::_DeleteBitmap(ServerBitmap* bitmap) +{ + ASSERT(fMapLock.IsLocked()); + + gBitmapManager->BitmapRemoved(bitmap); + fBitmapMap.erase(bitmap->Token()); + + bitmap->ReleaseReference(); +} + + ServerBitmap* ServerApp::_FindBitmap(int32 token) const { Modified: haiku/trunk/src/servers/app/ServerApp.h =================================================================== --- haiku/trunk/src/servers/app/ServerApp.h 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerApp.h 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -79,14 +79,12 @@ { return fInitialWorkspace; } ServerBitmap* GetBitmap(int32 token) const; - bool BitmapAdded(ServerBitmap* bitmap); - void BitmapRemoved(ServerBitmap* bitmap); ServerPicture* CreatePicture( const ServerPicture* original = NULL); ServerPicture* GetPicture(int32 token) const; - bool PictureAdded(ServerPicture* picture); - void PictureRemoved(ServerPicture* picture); + bool AddPicture(ServerPicture* picture); + void RemovePicture(ServerPicture* picture); Desktop* GetDesktop() const { return fDesktop; } @@ -105,7 +103,10 @@ bool _HasWindowUnderMouse(); + bool _AddBitmap(ServerBitmap* bitmap); + void _DeleteBitmap(ServerBitmap* bitmap); ServerBitmap* _FindBitmap(int32 token) const; + ServerPicture* _FindPicture(int32 token) const; private: Modified: haiku/trunk/src/servers/app/ServerBitmap.cpp =================================================================== --- haiku/trunk/src/servers/app/ServerBitmap.cpp 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerBitmap.cpp 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -28,8 +28,7 @@ using namespace BPrivate; -/*! - A word about memory housekeeping and why it's implemented this way: +/*! A word about memory housekeeping and why it's implemented this way: The reason why this looks so complicated is to optimize the most common path (bitmap creation from the application), and don't cause any further @@ -47,8 +46,7 @@ */ -/*! - \brief Constructor called by the BitmapManager (only). +/*! \brief Constructor called by the BitmapManager (only). \param rect Size of the bitmap. \param space Color space of the bitmap \param flags Various bitmap flags to tweak the bitmap as defined in Bitmap.h @@ -65,7 +63,6 @@ fAllocationCookie(NULL), fOverlay(NULL), fBuffer(NULL), - fReferenceCount(1), // WARNING: '1' is added to the width and height. // Same is done in FBBitmap subclass, so if you // modify here make sure to do the same under @@ -75,8 +72,7 @@ fBytesPerRow(0), fSpace(space), fFlags(flags), - fOwner(NULL), - fHasClientReference(true) + fOwner(NULL) // fToken is initialized (if used) by the BitmapManager { int32 minBytesPerRow = get_bytes_per_row(space, fWidth); @@ -92,8 +88,7 @@ fAllocationCookie(NULL), fOverlay(NULL), fBuffer(NULL), - fReferenceCount(1), - fHasClientReference(false) + fOwner(NULL) { if (bitmap) { fWidth = bitmap->fWidth; @@ -101,14 +96,12 @@ fBytesPerRow = bitmap->fBytesPerRow; fSpace = bitmap->fSpace; fFlags = bitmap->fFlags; - fOwner = bitmap->fOwner; } else { fWidth = 0; fHeight = 0; fBytesPerRow = 0; fSpace = B_NO_COLOR_SPACE; fFlags = 0; - fOwner = NULL; } } @@ -200,18 +193,10 @@ } -bool +void ServerBitmap::SetOwner(ServerApp* owner) { - if (fOwner != NULL) - fOwner->BitmapRemoved(this); - - if (owner != NULL && owner->BitmapAdded(this)) { - fOwner = owner; - return true; - } - - return false; + fOwner = owner; } @@ -222,18 +207,6 @@ } -bool -ServerBitmap::ReleaseClientReference() -{ - if (!fHasClientReference) - return false; - - fHasClientReference = false; - ReleaseReference(); - return true; -} - - void ServerBitmap::PrintToStream() { @@ -242,30 +215,21 @@ } -void -ServerBitmap::LastReferenceReleased() -{ - if (fOwner != NULL) - fOwner->BitmapRemoved(this); - - gBitmapManager->BitmapRemoved(this); - delete this; -} - - // #pragma mark - UtilityBitmap::UtilityBitmap(BRect rect, color_space space, uint32 flags, - int32 bytesperline, screen_id screen) - : ServerBitmap(rect, space, flags, bytesperline, screen) + int32 bytesPerRow, screen_id screen) + : + ServerBitmap(rect, space, flags, bytesPerRow, screen) { AllocateBuffer(); } UtilityBitmap::UtilityBitmap(const ServerBitmap* bitmap) - : ServerBitmap(bitmap) + : + ServerBitmap(bitmap) { AllocateBuffer(); @@ -276,7 +240,8 @@ UtilityBitmap::UtilityBitmap(const uint8* alreadyPaddedData, uint32 width, uint32 height, color_space format) - : ServerBitmap(BRect(0, 0, width - 1, height - 1), format, 0) + : + ServerBitmap(BRect(0, 0, width - 1, height - 1), format, 0) { AllocateBuffer(); if (Bits()) Modified: haiku/trunk/src/servers/app/ServerBitmap.h =================================================================== --- haiku/trunk/src/servers/app/ServerBitmap.h 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerBitmap.h 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -69,11 +69,9 @@ void SetOverlay(::Overlay* overlay); ::Overlay* Overlay() const; - bool SetOwner(ServerApp* owner); + void SetOwner(ServerApp* owner); ServerApp* Owner() const; - bool ReleaseClientReference(); - //! Does a shallow copy of the bitmap passed to it inline void ShallowCopy(const ServerBitmap *from); @@ -95,8 +93,6 @@ ServerBitmap(const ServerBitmap* bmp); virtual ~ServerBitmap(); - virtual void LastReferenceReleased(); - void AllocateBuffer(); protected: @@ -104,7 +100,6 @@ void* fAllocationCookie; ::Overlay* fOverlay; uint8* fBuffer; - int32 fReferenceCount; int32 fWidth; int32 fHeight; @@ -114,7 +109,6 @@ ServerApp* fOwner; int32 fToken; - bool fHasClientReference; }; class UtilityBitmap : public ServerBitmap { Modified: haiku/trunk/src/servers/app/ServerPicture.cpp =================================================================== --- haiku/trunk/src/servers/app/ServerPicture.cpp 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerPicture.cpp 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -783,9 +783,8 @@ : fFile(NULL), fPictures(NULL), - fUsurped(NULL), - fOwner(NULL), - fHasClientReference(true) + fPushed(NULL), + fOwner(NULL) { fToken = gTokenSpace.NewToken(kPictureToken, this); fData = new(std::nothrow) BMallocIO(); @@ -799,9 +798,8 @@ fFile(NULL), fData(NULL), fPictures(NULL), - fUsurped(NULL), - fOwner(NULL), - fHasClientReference(false) + fPushed(NULL), + fOwner(NULL) { fToken = gTokenSpace.NewToken(kPictureToken, this); @@ -827,9 +825,8 @@ fFile(NULL), fData(NULL), fPictures(NULL), - fUsurped(NULL), - fOwner(NULL), - fHasClientReference(true) + fPushed(NULL), + fOwner(NULL) { fToken = gTokenSpace.NewToken(kPictureToken, this); @@ -852,16 +849,26 @@ ServerPicture::~ServerPicture() { + ASSERT(fOwner == NULL); + delete fData; delete fFile; gTokenSpace.RemoveToken(fToken); if (fPictures != NULL) { - for (int32 i = fPictures->CountItems(); i-- > 0;) - fPictures->ItemAt(i)->ReleaseReference(); + for (int32 i = fPictures->CountItems(); i-- > 0;) { + ServerPicture* picture = fPictures->ItemAt(i); + picture->SetOwner(NULL); + picture->ReleaseReference(); + } delete fPictures; } + + if (fPushed != NULL) { + fPushed->SetOwner(NULL); + fPushed->ReleaseReference(); + } } @@ -869,29 +876,18 @@ ServerPicture::SetOwner(ServerApp* owner) { if (fOwner != NULL) - fOwner->PictureRemoved(this); + fOwner->RemovePicture(this); - if (owner != NULL && owner->PictureAdded(this)) { + if (owner != NULL && owner->AddPicture(this)) { fOwner = owner; return true; } + fOwner = NULL; return false; } -bool -ServerPicture::ReleaseClientReference() -{ - if (!fHasClientReference) - return false; - - fHasClientReference = false; - ReleaseReference(); - return true; -} - - void ServerPicture::EnterStateChange() { @@ -1014,22 +1010,38 @@ } +/*! Acquires a reference to the pushed picture. +*/ void -ServerPicture::Usurp(ServerPicture* picture) +ServerPicture::PushPicture(ServerPicture* picture) { - fUsurped = picture; + if (fPushed != NULL) + debugger("already pushed a picture"); + + fPushed = picture; + fPushed->AcquireReference(); } +/*! Returns a reference with the popped picture. +*/ ServerPicture* -ServerPicture::StepDown() +ServerPicture::PopPicture() { - ServerPicture* old = fUsurped; - fUsurped = NULL; + ServerPicture* old = fPushed; + fPushed = NULL; return old; } +void +ServerPicture::AppendPicture(ServerPicture* picture) +{ + // A pushed picture is the same as an appended one + PushPicture(picture); +} + + bool ServerPicture::NestPicture(ServerPicture* picture) { @@ -1119,13 +1131,3 @@ fData->Seek(oldPosition, SEEK_SET); return status; } - - -void -ServerPicture::LastReferenceReleased() -{ - if (fOwner != NULL) - fOwner->PictureRemoved(this); - - delete this; -} Modified: haiku/trunk/src/servers/app/ServerPicture.h =================================================================== --- haiku/trunk/src/servers/app/ServerPicture.h 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerPicture.h 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -49,9 +49,10 @@ void Play(View* view); - void Usurp(ServerPicture* newPicture); - ServerPicture* StepDown(); + void PushPicture(ServerPicture* picture); + ServerPicture* PopPicture(); + void AppendPicture(ServerPicture* picture); bool NestPicture(ServerPicture* picture); off_t DataLength() const; @@ -59,19 +60,16 @@ status_t ImportData(BPrivate::LinkReceiver& link); status_t ExportData(BPrivate::PortLink& link); -protected: - virtual void LastReferenceReleased(); - private: - typedef BObjectList<ServerPicture> PictureList; + typedef BObjectList<ServerPicture> PictureList; int32 fToken; BFile* fFile; BPositionIO* fData; PictureList* fPictures; - ServerPicture* fUsurped; + ServerPicture* fPushed; ServerApp* fOwner; - bool fHasClientReference; }; + #endif // SERVER_PICTURE_H Modified: haiku/trunk/src/servers/app/ServerWindow.cpp =================================================================== --- haiku/trunk/src/servers/app/ServerWindow.cpp 2010-02-15 14:43:09 UTC (rev 35471) +++ haiku/trunk/src/servers/app/ServerWindow.cpp 2010-02-15 14:50:53 UTC (rev 35472) @@ -1,5 +1,5 @@ /* - * Copyright 2001-2009, Haiku. + * Copyright 2001-2010, Haiku. * Distributed under the terms of the MIT License. * * Authors: @@ -2066,8 +2066,10 @@ DTRACE(("ServerWindow %s: Message AS_VIEW_BEGIN_PICTURE\n", Title())); ServerPicture* picture = App()->CreatePicture(); - picture->SyncState(fCurrentView); - fCurrentView->SetPicture(picture); + if (picture != NULL) { + picture->SyncState(fCurrentView); + fCurrentView->SetPicture(picture); + } break; } @@ -2756,9 +2758,9 @@ bool -ServerWindow::_DispatchPictureMessage(int32 code, BPrivate::LinkReceiver &link) +ServerWindow::_DispatchPictureMessage(int32 code, BPrivate::LinkReceiver& link) { - ServerPicture *picture = fCurrentView->Picture(); + ServerPicture* picture = fCurrentView->Picture(); if (picture == NULL) return false; @@ -2778,7 +2780,6 @@ BRect rect; link.Read<BRect>(&rect); picture->WriteInvertRect(rect); - break; } @@ -3092,13 +3093,12 @@ link.Read<int32>(&opCount); link.Read<int32>(&ptCount); - uint32 *opList = new(nothrow) uint32[opCount]; - BPoint *ptList = new(nothrow) BPoint[ptCount]; + uint32* opList = new(std::nothrow) uint32[opCount]; + BPoint* ptList = new(std::nothrow) BPoint[ptCount]; if (opList != NULL && ptList != NULL && link.Read(opList, opCount * sizeof(uint32)) >= B_OK && link.Read(ptList, ptCount * sizeof(BPoint)) >= B_OK) { - - // This might seem a bit weird, but under R5, the shapes + // This might seem a bit weird, but under BeOS, the shapes // are always offset by the current pen location BPoint penLocation = fCurrentView->CurrentState()->PenLocation(); @@ -3108,9 +3108,9 @@ const bool fill = (code == AS_FILL_SHAPE); picture->WriteDrawShape(opCount, opList, ptCount, ptList, fill); } + delete[] opList; delete[] ptList; - break; } @@ -3175,16 +3175,17 @@ // we are supposed to clear the clipping region picture->WriteClearClipping(); } - break; } + case AS_VIEW_BEGIN_PICTURE: { - ServerPicture *newPicture = App()->CreatePicture(); - newPicture->Usurp(picture); - newPicture->SyncState(fCurrentView); - fCurrentView->SetPicture(newPicture); - + ServerPicture* newPicture = App()->CreatePicture(); + if (newPicture != NULL) { + newPicture->PushPicture(picture); + newPicture->SyncState(fCurrentView); + fCurrentView->SetPicture(newPicture); + } break; } @@ -3196,7 +3197,7 @@ ServerPicture* appendPicture = App()->GetPicture(token); if (appendPicture != NULL) { //picture->SyncState(fCurrentView); - appendPicture->Usurp(picture); + appendPicture->AppendPicture(picture); } fCurrentView->SetPicture(appendPicture); @@ -3208,9 +3209,11 @@ case AS_VIEW_END_PICTURE: { - ServerPicture* steppedDown = picture->StepDown(); + ServerPicture* poppedPicture = picture->PopPicture(); + fCurrentView->SetPicture(poppedPicture); + if (poppedPicture != NULL) + poppedPicture->ReleaseReference(); - fCurrentView->SetPicture(steppedDown); fLink.StartMessage(B_OK); fLink.Attach<int32>(picture->Token()); fLink.Flush(); @@ -3301,6 +3304,7 @@ #endif int32 messagesProcessed = 0; + bigtime_t processingStart = system_time(); bool lockedDesktop = false; bool needsAllWindowsLocked = false; @@ -3384,8 +3388,9 @@ fDesktop->UnlockAllWindows(); // Only process up to 70 waiting messages at once (we have the - // Desktop locked) - if (!receiver.HasMessages() || ++messagesProcessed > 70) { + // Desktop locked), but don't hold the lock longer than 25 ms + if (!receiver.HasMessages() || ++messagesProcessed > 70 + || system_time() - processingStart > 25000) { if (lockedDesktop) fDesktop->UnlockSingleWindow(); break;