Merge pull request #59908 from bruvzg/fix_popup_close_race

Fix a possible race condition on popup close, that might cause multiple deletions of the same list item.

3 files changed, 67 insertions, 35 deletions

diff --git a/platform/linuxbsd/display_server_x11.cpp b/platform/linuxbsd/display_server_x11.cpp
index 8fa8c64efe..a6f6b0c5c4 100644
--- a/platform/linuxbsd/display_server_x11.cpp
+++ b/platform/linuxbsd/display_server_x11.cpp
@@ -3208,20 +3208,24 @@ Rect2i DisplayServerX11::window_get_popup_safe_rect(WindowID p_window) const {
 }
 
 void DisplayServerX11::popup_open(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	WindowData &wd = windows[p_window];
 	if (wd.is_popup) {
-		// Close all popups, up to current popup parent, or every popup if new window is not transient.
+		// Find current popup parent, or root popup if new window is not transient.
+		List<WindowID>::Element *C = nullptr;
 		List<WindowID>::Element *E = popup_list.back();
 		while (E) {
 			if (wd.transient_parent != E->get() || wd.transient_parent == INVALID_WINDOW_ID) {
-				_send_window_event(windows[E->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
-				List<WindowID>::Element *F = E->prev();
-				popup_list.erase(E);
-				E = F;
+				C = E;
+				E = E->prev();
 			} else {
 				break;
 			}
 		}
+		if (C) {
+			_send_window_event(windows[C->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
+		}
 
 		time_since_popup = OS::get_singleton()->get_ticks_msec();
 		popup_list.push_back(p_window);
@@ -3229,16 +3233,22 @@ void DisplayServerX11::popup_open(WindowID p_window) {
 }
 
 void DisplayServerX11::popup_close(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	List<WindowID>::Element *E = popup_list.find(p_window);
 	while (E) {
-		_send_window_event(windows[E->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
 		List<WindowID>::Element *F = E->next();
+		WindowID win_id = E->get();
 		popup_list.erase(E);
+
+		_send_window_event(windows[win_id], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
 		E = F;
 	}
 }
 
 void DisplayServerX11::mouse_process_popups() {
+	_THREAD_SAFE_METHOD_
+
 	if (popup_list.is_empty()) {
 		return;
 	}
@@ -3259,7 +3269,9 @@ void DisplayServerX11::mouse_process_popups() {
 			Vector2i pos = Vector2i(root_attrs.x + root_x, root_attrs.y + root_y);
 			if ((pos != last_mouse_monitor_pos) || (mask != last_mouse_monitor_mask)) {
 				if (((mask & Button1Mask) || (mask & Button2Mask) || (mask & Button3Mask) || (mask & Button4Mask) || (mask & Button5Mask))) {
+					List<WindowID>::Element *C = nullptr;
 					List<WindowID>::Element *E = popup_list.back();
+					// Find top popup to close.
 					while (E) {
 						// Popup window area.
 						Rect2i win_rect = Rect2i(window_get_position(E->get()), window_get_size(E->get()));
@@ -3270,12 +3282,13 @@ void DisplayServerX11::mouse_process_popups() {
 						} else if (safe_rect != Rect2i() && safe_rect.has_point(pos)) {
 							break;
 						} else {
-							_send_window_event(windows[E->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
-							List<WindowID>::Element *F = E->prev();
-							popup_list.erase(E);
-							E = F;
+							C = E;
+							E = E->prev();
 						}
 					}
+					if (C) {
+						_send_window_event(windows[C->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST);
+					}
 				}
 			}
 			last_mouse_monitor_mask = mask;
diff --git a/platform/osx/display_server_osx.mm b/platform/osx/display_server_osx.mm
index 6cdadcae39..986c711fc9 100644
--- a/platform/osx/display_server_osx.mm
+++ b/platform/osx/display_server_osx.mm
@@ -3000,21 +3000,25 @@ Rect2i DisplayServerOSX::window_get_popup_safe_rect(WindowID p_window) const {
 }
 
 void DisplayServerOSX::popup_open(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	WindowData &wd = windows[p_window];
 	if (wd.is_popup) {
 		bool was_empty = popup_list.is_empty();
-		// Close all popups, up to current popup parent, or every popup if new window is not transient.
+		// Find current popup parent, or root popup if new window is not transient.
+		List<WindowID>::Element *C = nullptr;
 		List<WindowID>::Element *E = popup_list.back();
 		while (E) {
 			if (wd.transient_parent != E->get() || wd.transient_parent == INVALID_WINDOW_ID) {
-				send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
-				List<WindowID>::Element *F = E->prev();
-				popup_list.erase(E);
-				E = F;
+				C = E;
+				E = E->prev();
 			} else {
 				break;
 			}
 		}
+		if (C) {
+			send_window_event(windows[C->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
+		}
 
 		if (was_empty && popup_list.is_empty()) {
 			// Inform OS that popup was opened, to close other native popups.
@@ -3026,12 +3030,16 @@ void DisplayServerOSX::popup_open(WindowID p_window) {
 }
 
 void DisplayServerOSX::popup_close(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	bool was_empty = popup_list.is_empty();
 	List<WindowID>::Element *E = popup_list.find(p_window);
 	while (E) {
-		send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
 		List<WindowID>::Element *F = E->next();
+		WindowID win_id = E->get();
 		popup_list.erase(E);
+
+		send_window_event(windows[win_id], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
 		E = F;
 	}
 	if (!was_empty && popup_list.is_empty()) {
@@ -3047,11 +3055,8 @@ void DisplayServerOSX::mouse_process_popups(bool p_close) {
 	if (p_close) {
 		// Close all popups.
 		List<WindowID>::Element *E = popup_list.front();
-		while (E) {
+		if (E) {
 			send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
-			List<WindowID>::Element *F = E->next();
-			popup_list.erase(E);
-			E = F;
 		}
 		if (!was_empty) {
 			// Inform OS that all popups are closed.
@@ -3064,7 +3069,9 @@ void DisplayServerOSX::mouse_process_popups(bool p_close) {
 		}
 
 		Point2i pos = mouse_get_position();
+		List<WindowID>::Element *C = nullptr;
 		List<WindowID>::Element *E = popup_list.back();
+		// Find top popup to close.
 		while (E) {
 			// Popup window area.
 			Rect2i win_rect = Rect2i(window_get_position(E->get()), window_get_size(E->get()));
@@ -3075,12 +3082,13 @@ void DisplayServerOSX::mouse_process_popups(bool p_close) {
 			} else if (safe_rect != Rect2i() && safe_rect.has_point(pos)) {
 				break;
 			} else {
-				send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
-				List<WindowID>::Element *F = E->prev();
-				popup_list.erase(E);
-				E = F;
+				C = E;
+				E = E->prev();
 			}
 		}
+		if (C) {
+			send_window_event(windows[C->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST);
+		}
 		if (!was_empty && popup_list.is_empty()) {
 			// Inform OS that all popups are closed.
 			[[NSDistributedNotificationCenter defaultCenter] postNotificationName:@"com.apple.HIToolbox.endMenuTrackingNotification" object:@"org.godotengine.godot.popup_window"];
diff --git a/platform/windows/display_server_windows.cpp b/platform/windows/display_server_windows.cpp
index 877e82e707..e61a0a751b 100644
--- a/platform/windows/display_server_windows.cpp
+++ b/platform/windows/display_server_windows.cpp
@@ -2081,20 +2081,24 @@ Rect2i DisplayServerWindows::window_get_popup_safe_rect(WindowID p_window) const
 }
 
 void DisplayServerWindows::popup_open(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	WindowData &wd = windows[p_window];
 	if (wd.is_popup) {
-		// Close all popups, up to current popup parent, or every popup if new window is not transient.
+		// Find current popup parent, or root popup if new window is not transient.
+		List<WindowID>::Element *C = nullptr;
 		List<WindowID>::Element *E = popup_list.back();
 		while (E) {
 			if (wd.transient_parent != E->get() || wd.transient_parent == INVALID_WINDOW_ID) {
-				_send_window_event(windows[E->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
-				List<WindowID>::Element *F = E->prev();
-				popup_list.erase(E);
-				E = F;
+				C = E;
+				E = E->prev();
 			} else {
 				break;
 			}
 		}
+		if (C) {
+			_send_window_event(windows[C->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
+		}
 
 		time_since_popup = OS::get_singleton()->get_ticks_msec();
 		popup_list.push_back(p_window);
@@ -2102,17 +2106,22 @@ void DisplayServerWindows::popup_open(WindowID p_window) {
 }
 
 void DisplayServerWindows::popup_close(WindowID p_window) {
+	_THREAD_SAFE_METHOD_
+
 	List<WindowID>::Element *E = popup_list.find(p_window);
 	while (E) {
-		_send_window_event(windows[E->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
 		List<WindowID>::Element *F = E->next();
+		WindowID win_id = E->get();
 		popup_list.erase(E);
+
+		_send_window_event(windows[win_id], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
 		E = F;
 	}
 }
 
 LRESULT DisplayServerWindows::MouseProc(int code, WPARAM wParam, LPARAM lParam) {
 	_THREAD_SAFE_METHOD_
+
 	uint64_t delta = OS::get_singleton()->get_ticks_msec() - time_since_popup;
 	if (delta > 250) {
 		switch (wParam) {
@@ -2123,7 +2132,9 @@ LRESULT DisplayServerWindows::MouseProc(int code, WPARAM wParam, LPARAM lParam)
 			case WM_MBUTTONDOWN: {
 				MOUSEHOOKSTRUCT *ms = (MOUSEHOOKSTRUCT *)lParam;
 				Point2i pos = Point2i(ms->pt.x, ms->pt.y);
+				List<WindowID>::Element *C = nullptr;
 				List<WindowID>::Element *E = popup_list.back();
+				// Find top popup to close.
 				while (E) {
 					// Popup window area.
 					Rect2i win_rect = Rect2i(window_get_position(E->get()), window_get_size(E->get()));
@@ -2134,13 +2145,13 @@ LRESULT DisplayServerWindows::MouseProc(int code, WPARAM wParam, LPARAM lParam)
 					} else if (safe_rect != Rect2i() && safe_rect.has_point(pos)) {
 						break;
 					} else {
-						_send_window_event(windows[E->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
-						List<WindowID>::Element *F = E->prev();
-						popup_list.erase(E);
-						E = F;
+						C = E;
+						E = E->prev();
 					}
 				}
-
+				if (C) {
+					_send_window_event(windows[C->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST);
+				}
 			} break;
 		}
 	}