Crash with DockBuilder altering the state of already submitted windows

[hsimyu]

Version/Branch of Dear ImGui:

Version: 1.76 WIP
Branch: docking (c142540)

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_glfw.cpp + imgui_impl_opengl3.cpp (using example_glfw_opengl3 for testing)
Compiler: MSVC
Operating System: Windows10, 64 bit (1903)

My Issue/Question:
I'm testing docking branch and working on the feature to serialize and load the current docking layouts with DockBuilder APIs.
(I recognize these APIs are in beta, but am investigating its possibility)

Before restoring a saved layout, I tried to remove all existing DockNodes with the code like:

// Remove All Active DockNode Trees
ImGuiContext& g = *GImGui;

for (auto* window : g.Windows)
{
    if (!window->Active) continue;
    if (!window->DockNode) continue;

    auto* rootNode = ImGui::DockNodeGetRootNode(window->DockNode);
    ImGui::DockBuilderRemoveNode(rootNode->ID);
}

When ImGui::DockBuilderRemoveNode() called, my application crashed in this function at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L14123, throwing read access violation exception.

I found that node has already freed in calling DockBuilderRemoveNodeDockedWindows() so that accessing node is invalid after calling the function.
Is it inappropriate to call ImGui::DockBuilderRemoveNode() with RootNode ID? or is it a bug of DockBuilderRemoveNode()?

Detailed traces how node was freed:

• In DockBuilderRemoveNodeDockedWindows(), DockContextProcessUndockWindow() is called. (at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L14221)
• In DockContextProcessUndockWindow(), DockNodeRemoveWindow() is called when window->DockNode is not NULL. (at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L12097)
• In DockNodeRemoveWindow(), DockContextRemoveNode() is called. (at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L12325)
• In DockContextRemoveNode(), IM_DELETE() is called for the node pointer. (at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L11771)

Note:
In my app, DockSpaces are not created explicitly.
The function below is added to main.cpp in example_glfw_opengl3.
-> There are 4 active windows ("Dear ImGui Demo", "Hello, world!", "saved_dock_state", "window_list").
Also, minimal example for reproduce this issue is provide at the bottom.

void RestoreDockTest()
{
    static std::unordered_map<uint32_t, SerializableDockTree> dockTrees;

    DrawMainMenu(dockTrees);

    {
        ImGui::Begin("saved_dock_state");

        for (const auto& pair : dockTrees)
        {
            PrintDockTree(pair.second);
            ImGui::Separator();
        }

        ImGui::End();
    }

    {
        ImGui::Begin("window_list");
        ImGui::TextUnformatted("Windows");
        ListWindows();
        ImGui::End();
    }
}

Screenshot
Docking state is like this:

Stack Trace

example_glfw_opengl3.exe!ImGui::DockBuilderRemoveNode(unsigned int node_id) 行 14123 C++
example_glfw_opengl3.exe!ApplySavedDockTree(const std::unordered_map<unsigned int,SerializableDockTree,std::hash,std::equal_to,std::allocator<std::pair<unsigned int const ,SerializableDockTree> > > & dockTreeMap) 行 197 C++
example_glfw_opengl3.exe!DrawMainMenu(std::unordered_map<unsigned int,SerializableDockTree,std::hash,std::equal_to,std::allocator<std::pair<unsigned int const ,SerializableDockTree> > > & dockTree) 行 278 C++
example_glfw_opengl3.exe!DockTest() 行 337 C++
example_glfw_opengl3.exe!main(int __formal, char * * __formal) 行 512 C++

Standalone, minimal, complete and verifiable example:

void ReproduceCrash()
{
    ImGui::Begin("window1");

    if (ImGui::Button("Remove All DockNodes"))
    {
        // Remove All DockNodes
        ImGuiContext& g = *GImGui;

        for (auto* window : g.Windows)
        {
            if (!window->Active) continue;
            if (!window->DockNode) continue;

            auto* rootNode = ImGui::DockNodeGetRootNode(window->DockNode);
            ImGui::DockBuilderRemoveNode(rootNode->ID);
        }
    }

    ImGui::End();
}

[ocornut]

Hello,

I may not have time to look into this in depth.

Adding extra code before the crashing line would technically fix the crash:

    node = DockContextFindNodeByID(ctx, node_id);
    if (node == NULL)
        return;

However with that setup I'm noticing that

gets turned into

(I moved dear imgui demo slightly to show its pos/size)
Which sounds incorrect to me in the first place.

If the problem you are trying to solve is:

working on the feature to serialize and load the current docking layouts with DockBuilder APIs.

I think we should leverage the .ini logic further. Currently it's not possible to reload .ini data or merge .ini data into active one, but I would like to allow this and I believe it may be a better route onward?

[hsimyu]

Adding the code

    node = DockContextFindNodeByID(ctx, node_id);
    if (node == NULL)
        return;

before the crashing line fixes read access violation exception.
However, on my setup, the assertion Must call EndChild() and not End()! is invoked (at https://github.com/ocornut/imgui/blob/docking/imgui.cpp#L6653) at ImGui::End() in ReproduceCrash() function.

(The assertion failed after clicked Remove All DockNodes Button.)

When the assertion failed,

• window->Flags == 0x01010002
• window->DockIsActive == false
• g.WithInEndChild == false

so that IM_ASSERT_USER_ERROR() failed.
I will investigate why window->Flags has ImGuiWindowFlags_ChildWindow (0x01000000) without using ImGui::BeginChild()...

---

I think we should leverage the .ini logic further. Currently it's not possible to reload .ini data or merge .ini data into active one, but I would like to allow this and I believe it may be a better route onward?

I agree it's better if such a feature provided.
In my case, I would like to save and load several layouts with its names (EditGraphics, EditUI, Debug, etc.).
If reload (and merge) will be available, there would be no need to inspect DockNode state with internal APIs for restoring.

[ocornut]

It's invalid to alter the docking state of a window while inside the window, so you can't call that code mid-frame from a docked window.

(Docked windows get turned into child, that's why the flag is set)

[hsimyu]

@ocornut

It's invalid to alter the docking state of a window while inside the window, so you can't call that code mid-frame from a docked window.

Thanks! I modified the ReproduceCrash() to

void ReproduceCrash()
{
    static bool requestRemoveNodes = false;

    ImGui::Begin("window1");

    if (ImGui::Button("Remove All DockNodes"))
    {
        requestRemoveNodes = true;
    }

    ImGui::End();

    if (requestRemoveNodes)
    {
        // Remove All DockNodes
        ImGuiContext& g = *GImGui;

        for (auto* window : g.Windows)
        {
            if (!window->Active) continue;
            if (!window->DockNode) continue;

            auto* rootNode = ImGui::DockNodeGetRootNode(window->DockNode);
            ImGui::DockBuilderRemoveNode(rootNode->ID);
        }

        requestRemoveNodes = false;
    }
}

This works fine.

---

If you do not have much time, I would like to create a pull request that inserts the extra codes before the crashing line.

[ocornut]

Below is a different test case from #3203.
Notice different behavior if "DockBuilder Test" is already docked or not when pressing the button.

I believe merging #3179 is misleading - it will work in SOME cases but not all and we should strive to clarify the exact requirements and needs for this to work reliably in every situation, instead of adding "silently fail" behaviors.

ImGui::Begin("Profiler##profiler1"); ImGui::End();
ImGui::Begin("Scene##scene1"); ImGui::End();
ImGui::Begin("Editor##editor1"); ImGui::End();
ImGui::Begin("Console##console1"); ImGui::End();
ImGui::Begin("Log##logger1"); ImGui::End();

ImGui::Begin("DockBuilder Test");
ImGuiID mDockspaceID = ImGui::GetID("blah");
if (ImGui::MenuItem("Default Layout"))
{
    ImGui::DockBuilderRemoveNode(mDockspaceID);
    ImGui::DockBuilderAddNode(mDockspaceID, ImGuiDockNodeFlags_None);
    ImGui::DockBuilderSetNodeSize(mDockspaceID, ImGui::GetMainViewport()->Size);

    ImGuiID dock_main = mDockspaceID;
    ImGuiID dock_right = ImGui::DockBuilderSplitNode(dock_main, ImGuiDir_Right, 0.3f, nullptr, &dock_main);
    ImGuiID dock_down = ImGui::DockBuilderSplitNode(dock_main, ImGuiDir_Down, 0.3f, nullptr, &dock_main);

    ImGui::DockBuilderDockWindow("DockBuilder Test", dock_main);
    ImGui::DockBuilderDockWindow("Profiler##profiler1", dock_main);
    ImGui::DockBuilderDockWindow("Scene##scene1", dock_main);
    ImGui::DockBuilderDockWindow("Editor##editor1", dock_right);
    ImGui::DockBuilderDockWindow("Console##console1", dock_down);
    ImGui::DockBuilderDockWindow("Log##logger1", dock_down);
    ImGui::DockBuilderFinish(mDockspaceID);
}
ImGui::End();

Also note that technically we support multiple Begin()/End() pairs to append to a same window.
We don't have a neat consistent double-buffering of every values but I imagine should make DockBuilder alter values that will only be picked at the next -first call of the frame to Begin()- for a given window.

[hsimyu]

we should strive to clarify the exact requirements and needs for this to work reliably in every situation, instead of adding "silently fail" behaviors.

I think the same as you.
I'll investigate this issue in detail and modify #3179 (or decline & create new one) to fix this.

[hsimyu]

I digged this and might find the source.

I think this issue is from "automatic dock node delete" in DockNodeRemoveWindow(). (the codes below, at imgui.cpp:L12386)

if (node->Windows.Size == 0 && !node->IsCentralNode() && !node->IsDockSpace() && window->DockId != node->ID)
{
    // Automatic dock node delete themselves if they are not holding at least one tab
    DockContextRemoveNode(&g, node, true);
    return;
}

If a DockNode('s id), which has no parent and are not either CentralNode or DockSpace, is passed to DockBuilderRemoveNode(), the "automatic deletion" occurs for the node.
DockBuilderRemoveDockedWindows() uses DockBuilderRemoveNode() through DockContextProcessUndockWindow(), so DockBuilderRemoveDockedWindows() deletes DockNode (unintentionally?) if such DockNode's id is passed as root_id
As a result, node is deleted at 5th line in DockBuilderRemoveNode() and the access violation is caused.

---

I'm not sure an appropriate solution for this problem at present, but I think To fix DockBuilderRemoveDockedWindows() or DockNodeRemoveWindow() to remove windows pointer of a DockNode, not to remove DockNode itself is one of possible solutions, because it seems unintended behavior.

Candidates:

• Remove "automatic deletion" feature in DockNodeRemoveWindow().
• Disable "automatic deletion" in DockNodeRemoveWindow() if called from DockBuilder functions.
• Replace the use of DockNodeRemoveWindow() in DockContextProcessUndockWindow().

@ocornut
Do you think what is an appropriate solution for this issue?

[ocornut]

I believe merging #3179 is misleading - it will work in SOME cases but not all and we should strive to clarify the exact requirements

Merged essentially #3179 today, I retract my comment above as that change is a definitive required fix.
The muddled waters here is your code and some of the discussion in #3203 (but the code link there is not accessible) suggested other issues related to working with DockBuilder with an open window, I believe those issues might still exist.

The "automatic" deletion you justly described is correct in that situation. That node is not "protected" unless there's there's a central node in the tree. I think it is right to remove the node and your fix was correct.

If it i am not misunderstanding the situation I think this issue is fixed now.

What I am confused about is that #3203 and #4295 are referring to another callstack where there's a dangling pointer and I don't know how to repro this. It is possible that that "other" bug is a side effect of the first one we just fixed, when nodes are merged node pointers would be invalidated and even the root node was not deleted (because there was a central node in the tree) maybe it still had moved, and the bottom lines of DockBuilderRemoveNode() could have accidentally affected the wrong node.

If anyone reading those threads still gets the crash mentioned in #3203 and #4295 (DockNodeUpdate() -> Begin() -> ... -> TitleBarHeight() crashing) please let me know!