From e837ee8bc6de62d1b98dfe2a3097f4f740dd98b4 Mon Sep 17 00:00:00 2001
From: Michael Lotz <mmlr@mlotz.ch>
Date: Tue, 14 Apr 2015 23:54:14 +0200
Subject: [PATCH] BView: Fix destruction order of layout items.

Because of the virtual hooks a BLayout must never be destroyed while it
still has layout items. If these items are only removed from the layout
in its destructor, the subclass version of hooks like ItemRemoved() are
not called anymore. This lead to leaks because many BLayout subclasses
use the ItemRemoved() hook to clean up associated data (as is suggested
explicitly in the BLayout documentation).

In the same line of thought, a BLayoutItem must never be deleted when it
is still attached to a layout, as it similarly has virtual hooks like
DetachedFromLayout() that can not be called at this point anymore.

The destructors of BLayout and BLayoutItem now have debugger calls in
case these conditions are not met which should help to avoid
accidentally introducing such hard to debug issues.

To ensure the correct destruction order the sequence is now:

* Destroy the child views first. This cleans up their layout items while
  the layout tree is still intact.
* Unset the view layout before removing layout items so it can properly
  detach from the layout instead of just deleting it.
---
 src/kits/interface/Layout.cpp     |  7 ++++---
 src/kits/interface/LayoutItem.cpp |  6 ++++--
 src/kits/interface/View.cpp       | 13 +++++++------
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/kits/interface/Layout.cpp b/src/kits/interface/Layout.cpp
index 98cffc5b1a..a066f36115 100644
--- a/src/kits/interface/Layout.cpp
+++ b/src/kits/interface/Layout.cpp
@@ -94,9 +94,10 @@ BLayout::~BLayout()
 	if (fOwner && this == fOwner->GetLayout())
 		fOwner->_LayoutLeft(this);
 
-	// removes and deletes all items
-	if (fTarget)
-		SetTarget(NULL);
+	if (CountItems() > 0) {
+		debugger("Deleting a BLayout that still has items. Subclass hooks "
+			"will not be called");
+	}
 }
 
 
diff --git a/src/kits/interface/LayoutItem.cpp b/src/kits/interface/LayoutItem.cpp
index 4cfc01e2c3..040d885573 100644
--- a/src/kits/interface/LayoutItem.cpp
+++ b/src/kits/interface/LayoutItem.cpp
@@ -35,8 +35,10 @@ BLayoutItem::BLayoutItem(BMessage* from)
 
 BLayoutItem::~BLayoutItem()
 {
-	if (fLayout)
-		fLayout->RemoveItem(this);
+	if (fLayout != NULL) {
+		debugger("Deleting a BLayoutItem that is still attached to a layout. "
+			"Call RemoveSelf first.");
+	}
 }
 
 
diff --git a/src/kits/interface/View.cpp b/src/kits/interface/View.cpp
index 460c838c51..5472778e66 100644
--- a/src/kits/interface/View.cpp
+++ b/src/kits/interface/View.cpp
@@ -704,9 +704,6 @@ BView::~BView()
 			"Call RemoveSelf first.");
 	}
 
-	_RemoveLayoutItemsFromLayout(true);
-	_RemoveSelf();
-
 	if (fToolTip != NULL)
 		fToolTip->ReleaseReference();
 
@@ -720,10 +717,13 @@ BView::~BView()
 		child = nextChild;
 	}
 
-	// delete the layout and the layout data
-	delete fLayoutData->fLayout;
+	SetLayout(NULL);
+	_RemoveLayoutItemsFromLayout(true);
+
 	delete fLayoutData;
 
+	_RemoveSelf();
+
 	if (fVerScroller)
 		fVerScroller->SetTarget((BView*)NULL);
 	if (fHorScroller)
@@ -4198,7 +4198,7 @@ BView::_RemoveLayoutItemsFromLayout(bool deleteItems)
 	int32 index = fLayoutData->fLayoutItems.CountItems();
 	while (index-- > 0) {
 		BLayoutItem* item = fLayoutData->fLayoutItems.ItemAt(index);
-		item->Layout()->RemoveItem(item);
+		item->RemoveSelf();
 			// Removes item from fLayoutItems list
 		if (deleteItems)
 			delete item;
@@ -4802,6 +4802,7 @@ BView::SetLayout(BLayout* layout)
 
 	// unset and delete the old layout
 	if (fLayoutData->fLayout) {
+		fLayoutData->fLayout->RemoveSelf();
 		fLayoutData->fLayout->SetOwner(NULL);
 		delete fLayoutData->fLayout;
 	}