Use ObjectID in ProgressDialog Window list by KoBeWi · Pull Request #113642 · godotengine/godot

KoBeWi · 2025-12-05T18:04:39Z

Progress dialog has a LocalVector of pointers to Windows (nodes, not the OS lol). The window is a child of WindowWrapper. The condition in WindowWrapper's destructor was always false, because the Window is freed before the wrapper, and so the ObjectID was invalid. For the purpose of removing a pointer from a vector, it does not matter if it's valid or not, so that check just didn't make sense.

Funny how ObjectID is normally supposed to prevent crashes 🤦‍♂️

kitbdev · 2025-12-06T05:22:33Z

It looks like the ObjectID was preventing a heap-use-after-free.
I think we could instead move it to predelete notification, or make Progress dialog use ObjectID.

KoBeWi · 2025-12-06T11:38:35Z

heap-use-after-free.

What. How exactly it's "used"? Does passing the pointer as an argument trigger it? How it can be ignored?

AThousandShips · 2025-12-06T12:30:54Z

It's used to detect if the pointer is valid, it's not preventing it but the ObjectDB call validates the pointer

KoBeWi · 2025-12-06T18:01:47Z

Well, I know that, but pointer validity is not relevant here.

AThousandShips · 2025-12-06T19:03:33Z

Yes because the method called performs a null check which triggers use after free:

godot/editor/gui/progress_dialog.cpp

Lines 270 to 273 in dec5a37

    
           void ProgressDialog::remove_host_window(Window *p_window) { 
        
           	ERR_FAIL_NULL(p_window); 
        
           	host_windows.erase(p_window); 
        
           }

KoBeWi · 2025-12-06T19:41:01Z

This error is just stupid. Why does it care whether the pointer is freed or not? I only want to use the address, not whatever it's pointing to. How to tell the sanitizer that I know that the pointer is invalid and it doesn't matter here? I could just convert it to integer or whatever, but that probably also counts as "use".

kitbdev · 2025-12-13T17:16:39Z

The address sanitizer checks when passing into a function apparently. There are function attributes like __attribute__((no_sanitize("address"))) that can be used but they depend on compiler and disable it for the whole function.

It should work if it is moved to predelete or enter/exit tree, right?

KoBeWi · 2025-12-13T20:02:37Z

Predelete is too late, not sure about exit tree.
But it would be nice if I could avoid making the changes more complex just to "fix" some false positive in CI :/

bruvzg

This check was not doing anything useful, host_window store pointers, and it doesn't matter if pointer is valid. And removing invalid pointers is actually necessary.

So change looks good. But since sanitizer is not happy, I would change host_window and related functions to store ObjectIDs instead of pointers. It's also safer in case some methods accessing host_window content is called after Window is freed but before pointer is removed.

diff --git a/editor/gui/progress_dialog.cpp b/editor/gui/progress_dialog.cpp
index b82b3549fc..bf3c571fe0 100644
--- a/editor/gui/progress_dialog.cpp
+++ b/editor/gui/progress_dialog.cpp
@@ -156,8 +156,11 @@ void ProgressDialog::_popup() {
        // will discard every key input.
        EditorNode::get_singleton()->set_process_input(true);
        // Disable all other windows to prevent interaction with them.
-       for (Window *w : host_windows) {
-               w->set_process_mode(PROCESS_MODE_DISABLED);
+       for (ObjectID wid : host_windows) {
+               Window *w = ObjectDB::get_instance<Window>(wid);
+               if (w) {
+                       w->set_process_mode(PROCESS_MODE_DISABLED);
+               }
        }

        Size2 ms = main->get_combined_minimum_size();
@@ -254,21 +257,22 @@ void ProgressDialog::end_task(const String &p_task) {
        if (tasks.is_empty()) {
                hide();
                EditorNode::get_singleton()->set_process_input(false);
-               for (Window *w : host_windows) {
-                       w->set_process_mode(PROCESS_MODE_INHERIT);
+               for (ObjectID wid : host_windows) {
+                       Window *w = ObjectDB::get_instance<Window>(wid);
+                       if (w) {
+                               w->set_process_mode(PROCESS_MODE_INHERIT);
+                       }
                }
        } else {
                _popup();
        }
 }

-void ProgressDialog::add_host_window(Window *p_window) {
-       ERR_FAIL_NULL(p_window);
+void ProgressDialog::add_host_window(ObjectID p_window) {
        host_windows.push_back(p_window);
 }

-void ProgressDialog::remove_host_window(Window *p_window) {
-       ERR_FAIL_NULL(p_window);
+void ProgressDialog::remove_host_window(ObjectID p_window) {
        host_windows.erase(p_window);
 }

diff --git a/editor/gui/progress_dialog.h b/editor/gui/progress_dialog.h
index 17deeac79c..4e51f9e3bf 100644
--- a/editor/gui/progress_dialog.h
+++ b/editor/gui/progress_dialog.h
@@ -79,7 +79,7 @@ class ProgressDialog : public CenterContainer {
        PanelContainer *center_panel = nullptr;
        VBoxContainer *main = nullptr;

-       LocalVector<Window *> host_windows;
+       LocalVector<ObjectID> host_windows;

        Size2 main_border_size;

@@ -101,8 +101,8 @@ public:
        bool task_step(const String &p_task, const String &p_state, int p_step = -1, bool p_force_redraw = true);
        void end_task(const String &p_task);

-       void add_host_window(Window *p_window);
-       void remove_host_window(Window *p_window);
+       void add_host_window(ObjectID p_window);
+       void remove_host_window(ObjectID p_window);

        ProgressDialog();
 };
diff --git a/editor/gui/window_wrapper.cpp b/editor/gui/window_wrapper.cpp
index 055a4361f0..e97780280d 100644
--- a/editor/gui/window_wrapper.cpp
+++ b/editor/gui/window_wrapper.cpp
@@ -383,13 +383,11 @@ WindowWrapper::WindowWrapper() {
        window_background->set_anchors_and_offsets_preset(PRESET_FULL_RECT);
        window->add_child(window_background);

-       ProgressDialog::get_singleton()->add_host_window(window);
+       ProgressDialog::get_singleton()->add_host_window(window_id);
 }

 WindowWrapper::~WindowWrapper() {
-       if (ObjectDB::get_instance(window_id)) {
-               ProgressDialog::get_singleton()->remove_host_window(window);
-       }
+       ProgressDialog::get_singleton()->remove_host_window(window_id);
 }

 // ScreenSelect

kitbdev · 2025-12-19T23:48:17Z

It is still happening, so it must be the ProgressDialog itself that is used after it is freed.

editor/gui/window_wrapper.cpp

kitbdev · 2026-01-05T20:58:01Z

The ProgressDialog singleton is never set to null, it just gets deleted.
So setting it to null in the ProgressDialog destructor together with the ProgressDialog::get_singleton() != nullptr check should fix it.

~ProgressDialog() {
	singleton = nullptr;
}

Just setting it to null makes also it easier to debug, since a debugger will break when it is used without needing the sanitizer.

Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>

akien-mga

Seems to pass CI now!

(Android builds failed due to Maven repos being flaky, should be restarted later.)

Repiteo · 2026-01-06T16:02:26Z

Thanks!

Use ObjectID in ProgressDialog Window list

KoBeWi added this to the 4.x milestone Dec 5, 2025

KoBeWi requested review from a team as code owners December 5, 2025 18:04

KoBeWi added bug topic:editor crash labels Dec 5, 2025

KoBeWi force-pushed the uno_reverse branch from de4ecef to 416060e Compare December 6, 2025 19:20

akien-mga requested a review from bruvzg December 17, 2025 12:55

bruvzg reviewed Dec 17, 2025

View reviewed changes

KoBeWi force-pushed the uno_reverse branch from 416060e to 22845d2 Compare December 17, 2025 21:07

KoBeWi changed the title ~~Remove unnecessary ObjectID from WindowWrapper~~ Use ObjectID in ProgressDialog Window list Dec 17, 2025

akien-mga reviewed Jan 5, 2026

View reviewed changes

editor/gui/window_wrapper.cpp Outdated Show resolved Hide resolved

akien-mga modified the milestones: 4.x, 4.6 Jan 5, 2026

KoBeWi force-pushed the uno_reverse branch from 22845d2 to 8a38caa Compare January 5, 2026 13:15

Use ObjectID in ProgressDialog Window list

51348a2

Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>

KoBeWi force-pushed the uno_reverse branch from 8a38caa to 51348a2 Compare January 6, 2026 13:56

akien-mga approved these changes Jan 6, 2026

View reviewed changes

Repiteo merged commit 46eb6e9 into godotengine:master Jan 6, 2026
56 of 60 checks passed

KoBeWi deleted the uno_reverse🔁 branch January 6, 2026 16:16

kitbdev mentioned this pull request Feb 6, 2026

Editor crash #113481

Open

rivie13 pushed a commit to rivie13/Phoenix-Agentic-Engine that referenced this pull request Feb 16, 2026

Merge pull request godotengine#113642 from KoBeWi/uno_reverse🔁

3620a03

Use ObjectID in ProgressDialog Window list

Uh oh!

Conversation

KoBeWi commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitbdev commented Dec 6, 2025

Uh oh!

KoBeWi commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AThousandShips commented Dec 6, 2025

Uh oh!

KoBeWi commented Dec 6, 2025

Uh oh!

AThousandShips commented Dec 6, 2025

Uh oh!

KoBeWi commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitbdev commented Dec 13, 2025

Uh oh!

KoBeWi commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruvzg left a comment

Choose a reason for hiding this comment

Uh oh!

kitbdev commented Dec 19, 2025

Uh oh!

Uh oh!

kitbdev commented Jan 5, 2026

Uh oh!

akien-mga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Repiteo commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

KoBeWi commented Dec 5, 2025 •

edited

Loading

KoBeWi commented Dec 6, 2025 •

edited

Loading

KoBeWi commented Dec 6, 2025 •

edited

Loading

KoBeWi commented Dec 13, 2025 •

edited

Loading