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

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

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

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) {

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

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

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) {

src/servers/app/drawing/HWInterface.h

@@ -9,7 +9,9 @@
 #define HW_INTERFACE_H
 
 
+#include "IntRect.h"
 #include "MultiLocker.h"
+#include "ServerCursor.h"
 
 #include <video_overlay.h>
 
@@ -20,13 +22,10 @@
 #include <OS.h>
 #include <Region.h>
 
-#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();