From daa42e0e5094b492bad3b8e635650b76c72f8346 Mon Sep 17 00:00:00 2001 From: bruvzg <7645683+bruvzg@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:34:27 +0300 Subject: [PATCH] Fix a possible race condition on popup close, that might cause multiple deletions of the same list item. --- platform/linuxbsd/display_server_x11.cpp | 33 +++++++++++++------ platform/osx/display_server_osx.mm | 36 +++++++++++++-------- platform/windows/display_server_windows.cpp | 33 ++++++++++++------- 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/platform/linuxbsd/display_server_x11.cpp b/platform/linuxbsd/display_server_x11.cpp index 8fa8c64efe1..a6f6b0c5c4f 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::Element *C = nullptr; List::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::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::Element *E = popup_list.find(p_window); while (E) { - _send_window_event(windows[E->get()], DisplayServerX11::WINDOW_EVENT_CLOSE_REQUEST); List::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::Element *C = nullptr; List::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::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 6cdadcae395..986c711fc9e 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::Element *C = nullptr; List::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::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::Element *E = popup_list.find(p_window); while (E) { - send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST); List::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::Element *E = popup_list.front(); - while (E) { + if (E) { send_window_event(windows[E->get()], DisplayServerOSX::WINDOW_EVENT_CLOSE_REQUEST); - List::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::Element *C = nullptr; List::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::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 877e82e707e..e61a0a751bc 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::Element *C = nullptr; List::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::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::Element *E = popup_list.find(p_window); while (E) { - _send_window_event(windows[E->get()], DisplayServerWindows::WINDOW_EVENT_CLOSE_REQUEST); List::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::Element *C = nullptr; List::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::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; } }