From ace2d5ee376ad7d772cd313781ea2f912409f299 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Wed, 2 Apr 2008 11:12:39 +0000 Subject: [PATCH] HWInterface::Cursor() and therefor Desktop::Cursor() accessed the current cursor without locking, and did not add a reference while using the cursor. I have tried to solve both problems by introducing a simple ServerCursorReference class, which makes sure that the reference count is properly maintained. There are only two places where this code was even used, from within ServerApp and when taking screenshots. Axel, you mentioned in #837 that the code is unsafe, is this what you meant? This hopefully fixes #837, but it is very hard to reproduce in the first place, I will close the ticket, but it should just be reopened if ever encountered again. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@24741 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- src/servers/app/Desktop.cpp | 6 +-- src/servers/app/Desktop.h | 7 +-- src/servers/app/ServerApp.cpp | 2 - src/servers/app/ServerCursor.h | 52 ++++++++++++++++++++++- src/servers/app/drawing/DrawingEngine.cpp | 27 ++++++------ src/servers/app/drawing/HWInterface.cpp | 20 ++++++--- src/servers/app/drawing/HWInterface.h | 7 ++- 7 files changed, 91 insertions(+), 30 deletions(-) diff --git a/src/servers/app/Desktop.cpp b/src/servers/app/Desktop.cpp index 46848fe069..1ee6e9b084 100644 --- a/src/servers/app/Desktop.cpp +++ b/src/servers/app/Desktop.cpp @@ -714,15 +714,15 @@ Desktop::SetCursor(ServerCursor* newCursor) if (newCursor == NULL) newCursor = fCursorManager.GetCursor(B_CURSOR_DEFAULT); - ServerCursor* oldCursor = Cursor(); - if (newCursor == oldCursor) + ServerCursorReference oldCursor = Cursor(); + if (newCursor == oldCursor.Cursor()) return; HWInterface()->SetCursor(newCursor); } -ServerCursor* +ServerCursorReference Desktop::Cursor() const { return HWInterface()->Cursor(); diff --git a/src/servers/app/Desktop.h b/src/servers/app/Desktop.h index fc9b3fcebc..7c35f08cd1 100644 --- a/src/servers/app/Desktop.h +++ b/src/servers/app/Desktop.h @@ -12,12 +12,13 @@ #include "CursorManager.h" +#include "DesktopSettings.h" #include "EventDispatcher.h" +#include "MessageLooper.h" #include "Screen.h" #include "ScreenManager.h" +#include "ServerCursor.h" #include "VirtualScreen.h" -#include "DesktopSettings.h" -#include "MessageLooper.h" #include "WindowList.h" #include "Workspace.h" #include "WorkspacePrivate.h" @@ -78,7 +79,7 @@ class Desktop : public MessageLooper, public ScreenOwner { CursorManager& GetCursorManager() { return fCursorManager; } void SetCursor(ServerCursor* cursor); - ServerCursor* Cursor() const; + ServerCursorReference Cursor() const; void SetLastMouseState(const BPoint& position, int32 buttons); // for use by the mouse filter only diff --git a/src/servers/app/ServerApp.cpp b/src/servers/app/ServerApp.cpp index 61e3040a55..db29ffe785 100644 --- a/src/servers/app/ServerApp.cpp +++ b/src/servers/app/ServerApp.cpp @@ -291,8 +291,6 @@ ServerApp::Activate(bool value) if (fIsActive == value) return; - // TODO: send some message to the client?!? - fIsActive = value; if (fIsActive) { diff --git a/src/servers/app/ServerCursor.h b/src/servers/app/ServerCursor.h index 80c22a1607..70493224c8 100644 --- a/src/servers/app/ServerCursor.h +++ b/src/servers/app/ServerCursor.h @@ -33,7 +33,7 @@ class ServerCursor : public ServerBitmap { ServerCursor(const ServerCursor* cursor); virtual ~ServerCursor(); - + //! Returns the cursor's hot spot void SetHotSpot(BPoint pt); BPoint GetHotSpot() const @@ -70,4 +70,54 @@ class ServerCursor : public ServerBitmap { vint32 fPendingViewCursor; }; + +class ServerCursorReference { +public: + ServerCursorReference() + : fCursor(NULL) + { + } + ServerCursorReference(ServerCursor* cursor) + : fCursor(cursor) + { + if (fCursor) + fCursor->Acquire(); + } + ServerCursorReference(const ServerCursorReference& other) + : fCursor(other.fCursor) + { + if (fCursor) + fCursor->Acquire(); + } + virtual ~ServerCursorReference() + { + if (fCursor) + fCursor->Release(); + } + + ServerCursorReference& operator=(const ServerCursorReference& other) + { + SetCursor(other.fCursor); + return *this; + } + + void SetCursor(ServerCursor* cursor) + { + if (fCursor == cursor) + return; + if (fCursor) + fCursor->Release(); + fCursor = cursor; + if (fCursor) + fCursor->Acquire(); + } + ServerCursor* Cursor() const + { + return fCursor; + } +private: + ServerCursor* fCursor; +}; + + #endif // SERVER_CURSOR_H diff --git a/src/servers/app/drawing/DrawingEngine.cpp b/src/servers/app/drawing/DrawingEngine.cpp index 4514df2efa..25e30f474c 100644 --- a/src/servers/app/drawing/DrawingEngine.cpp +++ b/src/servers/app/drawing/DrawingEngine.cpp @@ -62,7 +62,7 @@ class AutoFloatingOverlaysHider { , fHidden(interface->HideFloatingOverlays(area)) { } - + AutoFloatingOverlaysHider(HWInterface* interface) : fInterface(interface) , fHidden(fInterface->HideFloatingOverlays()) @@ -83,7 +83,7 @@ class AutoFloatingOverlaysHider { private: HWInterface* fInterface; bool fHidden; - + }; @@ -127,7 +127,7 @@ DrawingEngine::UnlockParallelAccess() bool DrawingEngine::LockExclusiveAccess() -{ +{ return fGraphicsCard->LockExclusiveAccess(); } @@ -453,7 +453,7 @@ DrawingEngine::CopyRegion(/*const*/ BRegion* region, // compare vertically if (yOffset > 0) { if (is_above(a, b)) { - cmp -= 1; + cmp -= 1; } else if (is_above(b, a)) { cmp += 1; } @@ -541,7 +541,7 @@ DrawingEngine::InvertRect(BRect r) BRegion region(r); region.IntersectWith(fPainter->ClippingRegion()); fGraphicsCard->InvertRegion(region); - } else { + } else { fPainter->InvertRect(r); _CopyToFront(r); @@ -586,7 +586,7 @@ DrawingEngine::DrawArc(BRect r, const float &angle, BPoint center(r.left + xRadius, r.top + yRadius); - if (filled) + if (filled) fPainter->FillArc(center, xRadius, yRadius, angle, span); else fPainter->StrokeArc(center, xRadius, yRadius, angle, span); @@ -730,7 +730,7 @@ DrawingEngine::FillRect(BRect r, const rgb_color& color) || overlaysHider.WasHidden()); } else { fPainter->FillRect(r, color); - + _CopyToFront(r); } } @@ -1069,7 +1069,7 @@ DrawingEngine::DrawString(const char* string, int32 length, //bigtime_t now = system_time(); // TODO: BoundingBox is quite slow!! Optimizing it will be beneficial. // Cursiously, the DrawString after it is actually faster!?! -// TODO: make the availability of the hardware cursor part of the +// TODO: make the availability of the hardware cursor part of the // HW acceleration flags and skip all calculations for HideFloatingOverlays // in case we don't have one. // TODO: Watch out about penLocation and use Painter::PenLocation() when @@ -1151,11 +1151,14 @@ DrawingEngine::ReadBitmap(ServerBitmap *bitmap, bool drawCursor, BRect bounds) status_t result = bitmap->ImportBits(buffer->Bits(), buffer->BitsLength(), buffer->BytesPerRow(), buffer->ColorSpace(), - bounds.LeftTop(), BPoint(0, 0), + bounds.LeftTop(), BPoint(0, 0), bounds.IntegerWidth() + 1, bounds.IntegerHeight() + 1); if (drawCursor) { - ServerCursor *cursor = fGraphicsCard->Cursor(); + ServerCursorReference cursorRef = fGraphicsCard->Cursor(); + ServerCursor* cursor = cursorRef.Cursor(); + if (!cursor) + return result; int32 cursorWidth = cursor->Width(); int32 cursorHeight = cursor->Height(); @@ -1164,7 +1167,7 @@ DrawingEngine::ReadBitmap(ServerBitmap *bitmap, bool drawCursor, BRect bounds) BBitmap cursorArea(BRect(0, 0, cursorWidth - 1, cursorHeight - 1), B_BITMAP_NO_SERVER_LINK, B_RGBA32); - + cursorArea.ImportBits(bitmap->Bits(), bitmap->BitsLength(), bitmap->BytesPerRow(), bitmap->ColorSpace(), cursorPosition, BPoint(0, 0), @@ -1185,7 +1188,7 @@ DrawingEngine::ReadBitmap(ServerBitmap *bitmap, bool drawCursor, BRect bounds) bitmap->ImportBits(cursorArea.Bits(), cursorArea.BitsLength(), cursorArea.BytesPerRow(), cursorArea.ColorSpace(), - BPoint(0, 0), cursorPosition, + BPoint(0, 0), cursorPosition, cursorWidth, cursorHeight); } diff --git a/src/servers/app/drawing/HWInterface.cpp b/src/servers/app/drawing/HWInterface.cpp index 80677b4ce9..64ae40681e 100644 --- a/src/servers/app/drawing/HWInterface.cpp +++ b/src/servers/app/drawing/HWInterface.cpp @@ -12,7 +12,6 @@ #include "drawing_support.h" #include "RenderingBuffer.h" -#include "ServerCursor.h" #include "SystemPalette.h" #include "UpdateQueue.h" @@ -148,6 +147,17 @@ HWInterface::SetCursor(ServerCursor* cursor) fFloatingOverlaysLock.Unlock(); } +// Cursor +ServerCursorReference +HWInterface::Cursor() const +{ + if (!fFloatingOverlaysLock.Lock()) + return ServerCursorReference(NULL); + ServerCursorReference reference(fCursor); + fFloatingOverlaysLock.Unlock(); + return reference; +} + // SetCursorVisible void HWInterface::SetCursorVisible(bool visible) @@ -529,7 +539,7 @@ HWInterface::_DrawCursor(IntRect area) const uint8* c = crs; uint8* d = dst; uint8* b = bup; - + for (int32 x = area.left; x <= area.right; x++) { *(uint32*)b = *(uint32*)s; // assumes backbuffer alpha = 255 @@ -601,7 +611,7 @@ HWInterface::_CopyToFront(uint8* src, uint32 srcBPR, case B_RGB32: case B_RGBA32: { int32 bytes = (right - x + 1) * 4; - + if (bytes > 0) { // offset to left top pixel in dest buffer dst += y * dstBPR + x * 4; @@ -657,7 +667,7 @@ HWInterface::_CopyToFront(uint8* src, uint32 srcBPR, } break; } - case B_RGB15: + case B_RGB15: case B_RGBA15: { // offset to left top pixel in dest buffer dst += y * dstBPR + x * 2; @@ -702,7 +712,7 @@ HWInterface::_CopyToFront(uint8* src, uint32 srcBPR, break; } - case B_GRAY8: + case B_GRAY8: if (frontBuffer->Width() > dstBPR) { // VGA 16 color grayscale planar mode if (fVGADevice > 0) { diff --git a/src/servers/app/drawing/HWInterface.h b/src/servers/app/drawing/HWInterface.h index e3235e5633..73660e6615 100644 --- a/src/servers/app/drawing/HWInterface.h +++ b/src/servers/app/drawing/HWInterface.h @@ -9,7 +9,9 @@ #define HW_INTERFACE_H +#include "IntRect.h" #include "MultiLocker.h" +#include "ServerCursor.h" #include @@ -20,13 +22,10 @@ #include #include -#include "IntRect.h" - class Overlay; class RenderingBuffer; class ServerBitmap; -class ServerCursor; class UpdateQueue; class BString; @@ -108,7 +107,7 @@ class HWInterface : protected MultiLocker { virtual void Sync() {} // cursor handling (these do their own Read/Write locking) - ServerCursor* Cursor() const { return fCursor; } + ServerCursorReference Cursor() const; virtual void SetCursor(ServerCursor* cursor); virtual void SetCursorVisible(bool visible); bool IsCursorVisible();