From d363234129484733c55f961215bcb1343d84392e Mon Sep 17 00:00:00 2001
From: Colin Snover
Date: Mon, 18 Sep 2017 22:13:44 -0500
Subject: SCI32: Fix GfxFrameout::addPlane from causing possible leaks

---
 engines/sci/graphics/controls32.cpp |  2 +-
 engines/sci/graphics/frameout.cpp   | 22 +++++++++++-----------
 engines/sci/graphics/frameout.h     |  8 +++-----
 engines/sci/graphics/video32.cpp    |  6 +++---
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/engines/sci/graphics/controls32.cpp b/engines/sci/graphics/controls32.cpp
index 7080c108b3..77dfdc20bb 100644
--- a/engines/sci/graphics/controls32.cpp
+++ b/engines/sci/graphics/controls32.cpp
@@ -128,7 +128,7 @@ reg_t GfxControls32::kernelEditText(const reg_t controlObject) {
 
 	Plane *plane = new Plane(editorPlaneRect, kPlanePicTransparent);
 	plane->changePic();
-	g_sci->_gfxFrameout->addPlane(*plane);
+	g_sci->_gfxFrameout->addPlane(plane);
 
 	CelInfo32 celInfo;
 	celInfo.type = kCelTypeMem;
diff --git a/engines/sci/graphics/frameout.cpp b/engines/sci/graphics/frameout.cpp
index 9a7bfc804a..d3e08bf1c1 100644
--- a/engines/sci/graphics/frameout.cpp
+++ b/engines/sci/graphics/frameout.cpp
@@ -275,7 +275,7 @@ void GfxFrameout::kernelAddPlane(const reg_t object) {
 		updatePlane(*plane);
 	} else {
 		plane = new Plane(object);
-		addPlane(*plane);
+		addPlane(plane);
 	}
 }
 
@@ -351,17 +351,17 @@ int16 GfxFrameout::kernelGetHighPlanePri() {
 	return _planes.getTopSciPlanePriority();
 }
 
-void GfxFrameout::addPlane(Plane &plane) {
-	if (_planes.findByObject(plane._object) == nullptr) {
-		plane.clipScreenRect(_screenRect);
-		_planes.add(&plane);
-	} else {
-		plane._deleted = 0;
-		if (plane._created == 0) {
-			plane._moved = g_sci->_gfxFrameout->getScreenCount();
-		}
-		_planes.sort();
+void GfxFrameout::addPlane(Plane *plane) {
+	// In SSCI, if a plane with the same object ID already existed, this call
+	// would cancel deletion and update an already-existing plane, but callers
+	// expect the passed plane object to become memory-managed by GfxFrameout,
+	// so doing what SSCI did would end up leaking the Plane objects
+	if (_planes.findByObject(plane->_object) != nullptr) {
+		error("Plane %04x:%04x already exists", PRINT_REG(plane->_object));
 	}
+
+	plane->clipScreenRect(_screenRect);
+	_planes.add(plane);
 }
 
 void GfxFrameout::updatePlane(Plane &plane) {
diff --git a/engines/sci/graphics/frameout.h b/engines/sci/graphics/frameout.h
index ddaa17ca3d..cd48c78fdd 100644
--- a/engines/sci/graphics/frameout.h
+++ b/engines/sci/graphics/frameout.h
@@ -120,15 +120,13 @@ private:
 
 public:
 	/**
-	 * Creates and adds a new plane to the plane list, or
-	 * cancels deletion and updates an already-existing
-	 * plane if a plane matching the given plane VM object
-	 * already exists within the current plane list.
+	 * Creates and adds a new plane to the plane list. Ownership of the passed
+	 * object is transferred to GfxFrameout.
 	 *
 	 * @note This method is on Screen in SCI engine, but it
 	 * is only ever called on `GraphicsMgr.screen`.
 	 */
-	void addPlane(Plane &plane);
+	void addPlane(Plane *plane);
 
 	/**
 	 * Deletes a plane within the current plane list.
diff --git a/engines/sci/graphics/video32.cpp b/engines/sci/graphics/video32.cpp
index 301092bf4e..91e48612d8 100644
--- a/engines/sci/graphics/video32.cpp
+++ b/engines/sci/graphics/video32.cpp
@@ -696,7 +696,7 @@ VMDPlayer::EventFlags VMDPlayer::playUntilEvent(const EventFlags flags, const ui
 
 		if (!_blackoutRect.isEmpty() && _planeIsOwned) {
 			_blackoutPlane = new Plane(_blackoutRect);
-			g_sci->_gfxFrameout->addPlane(*_blackoutPlane);
+			g_sci->_gfxFrameout->addPlane(_blackoutPlane);
 		}
 
 		if (shouldUseCompositing()) {
@@ -897,7 +897,7 @@ void VMDPlayer::initComposited() {
 		if (_priority) {
 			_plane->_priority = _priority;
 		}
-		g_sci->_gfxFrameout->addPlane(*_plane);
+		g_sci->_gfxFrameout->addPlane(_plane);
 		_screenItem = new ScreenItem(_plane->_object, vmdCelInfo, Common::Point(), vmdScaleInfo);
 	} else {
 		_screenItem = new ScreenItem(_plane->_object, vmdCelInfo, Common::Point(_drawRect.left, _drawRect.top), vmdScaleInfo);
@@ -1040,7 +1040,7 @@ void DuckPlayer::open(const GuiResourceId resourceId, const int displayMode, con
 
 	if (_doFrameOut) {
 		_plane = new Plane(_drawRect, kPlanePicColored);
-		g_sci->_gfxFrameout->addPlane(*_plane);
+		g_sci->_gfxFrameout->addPlane(_plane);
 		g_sci->_gfxFrameout->frameOut(true);
 	}
 
-- 
cgit v1.2.3