Context recovery / graceful handling of bad state

[sherief]

I'm using vanilla Dear Imgui v1.60 WIP.

I have an app that exposes imgui API to plugins using a scripting language. Now, in my own code I can be sure that I am "well-behaved" - for example, I match calls to ImGui::Begin() with calls to ImGui::End(). But I can't be sure that plugins will be the same. I also have live reloading of plugin scripts, and sometimes I end up trying to load plugin code that won't compile (I can keep using the last-known-good version in this case) or worse: plugin code that isn't well-behaved with respect to imgui API. Consider a plugin that consists entirely of [the scrip equivalent of] the following:

ImGui::Begin("This won't end well.");

I end up hitting this assertion:

IM_ASSERT(g.CurrentWindowStack.Size == 1);    // Mismatched Begin()/End() calls

As a feature request, is it possible to consider supporting more graceful failures? I don't have a solid idea in mind, but you can see my use case above. It could be something like D3D12's command lists, where the CloseCommandList() function returns a status code indicating whether this was a valid or malformed command list - maybe ImGui::EndFrame() could return a similar value and just drop the frame altogether, or render as much of it as it can [and I realize how this is a fuzzy concept - I'm just thinking out loud here].

Is there some other way to do this that I'm missing? I wouldn't mind running the plugin code in its own imgui context (multiple context support rocks!), but currently that doesn't seem to solve the problem I have.

[ocornut]

I agree this is desirable, I'll have to think about how we can do it but there's definitively improvements to make.

One of the problem is that several of those issues ideally wants some reporting, and for more local push/pop it is nice to be able to notify the programmer locally.

I'm also not really sure we want imgui to be one of those library that spams you with warnings and log (whether they are printf or a specific window for that purpose!). Even if that's hidden behind some sort of flag, it typically end up being either enabled on all machines and spammy, or disabled and unnecessary, so it's a little tricky to find the right balance. If you have any suggestion..

[sherief]

I'm not a fan of having the library spam output anywhere - how about having End() take a custom user function like this:

using handler = HandlerResponse (*)(Context* context, /* args for error type and description */, void* userData);

ImGui::End(handler CustomHandler = ImGui::DefaultHandler, void* userData /*passed as-is to handler */ = nullptr);

This way, code is backwards-compatible. If you don't specify a custom handler, the default handler can call IM_ASSERT(). The code also won't have to implement writing anything anywhere - that's the handler's job. And HandlerResponse can communicate info back to imgui (maybe what to do on failure - drop entire frame vs attempt to recover as much UI as you can?).

What do you think?

[ocornut]

I'll have to experiment with Begin/End, Push/Pop variants and first see how we can reliably detect and recover.

Another kind of imgui issue that may be interesting to detect are ID collisions, which can happen anywhere. For those if we want a lightweight check we'll have to do it when either widget is active.

This might end up getting similar to just rewiring IM_ASSERT() when you are in the context of your scripting language, assuming imgui can recover after said assert.

[sherief]

Would it be possible to save / load the context's state to a chunk of memory? I'm thinking of something like

void* buffer = malloc();
ImGui::SaveState(buffer, bufferSize);
bool success = runScriptUICode();
if(!success)
{
ImGui::LoadState(buffer, bufferSize); //Back where we started
}
ImGui::EndFrame();

[ocornut]

Not really, but just destroying and recreating a context generally yield good result because the windows position/size are saved. You lose things like tree node open/close state but otherwise it's not really visible.

[sherief]

That's workable. Ideally though, being able to save the state once the UI code gets "borked" then returning to it once it's unborked would be great. Some of the plugin code operates on a DCC-like app's object hierarchy and tree node state specifically is valuable to me.

Not to mention, this might also help with the case of restoring UI state across launches which is another issue I was planning to bring up at one point...

[ocornut]

Not to mention, this might also help with the case of restoring UI state across launches which is another issue I was planning to bring up at one point...

Depend what you mean by UI state:

Tree nodes/Collapsing header open/close state
Technically you can use SetStateStorage()/GetStateStorage() (per-window) and backup/restore that content however it REALLY wasn't designed for persistence: there's no type info/safety, no handling of hash collision, no clean up/garbage collection, So while it works well enough for typical working session (and nobody reported an issue) if you were to store and restore that data long term it would probably build-up into a problem over time.

I don't think you really want to be preserving most of tree node state but it may be useful to certain certain nodes for saving. If we end up reorganizing the state storage field we could add enough info. Currently the struct is:

u32 pair
void* value (enum)

We could stick in a few things, e.g.

u32 active_this_session : 1
u32 persistence_desirable : 1
u32 type : remaining bits?

Haven't really thought about it more than that.

Z-order
I agree it's a problem that it's not preserved for now.

What else?
Most of the data is on your side. It would be nice if we provided im-style very basic serialization primitives, but with persistence comes a bunch of new responsibility and I don't see it being a near-term priority.

I hope in the upcoming few months I can tackle more graceful error handling tho.

[sherief]

Man I don't even know what all state involved in a restore is, but I'm setting out to find out now.

[nikki93]

@ocornut -- Did you get a chance to think about / tackle this more? I'm exposing ImGui in a live scripting context so it's common that there are mismatches when the code a user is writing isn't 'complete' yet due to try/catch style behavior in the scripting language. I have a workaround for now by having my own utilities that take and call a closure and make sure to do the .end*()/.pop*() calls, allowing code like:

imgui.inWindow(..., function()
  imgui.selectable(...)
  -- Can do stuff throwing errors here, will still call `.end()` after
  error('oops!')
end)

Again, sorry for using a non-C++ language to show this example but the point was to show the utility allowing safe calls that I wrote in the scripting language. :)

Because I have these utilities it's not super urgent for me, but wondering if you've thought further.

[sherief]

That's exactly the case I'm running into. I have a live reload system in place and sometimes a script is written to disk with a logic error somewhere, and on the next live reload my UI state goes FUBAR.

I've resorted to some state tracking similar to what you have, but it's fragile, doesn't cover all cases, and feels hacky af.

[ocornut]

Sorry I haven't had time to look at that yet. It looks sufficiently simple and useful that I may. I think it possibly might make more sense as an explicit helper to be called optionally (and generally by user of scripting languages) rather than being hard-wired by default.

The reason being that we can't just silently ignore those errors. The expectation is that the code running script would poll an error flag and turn this into end-user feedback, however at the library/C++ level we have no way of providing this feedback to the user. Making this explicit also solve the issue that scope are not easy to define (e.g. the issue with PushStyleCol/Begin/PopStyleCol/End patterns, see #1767).

Even though they are less common, we would nice to also protect from excessive calls to PopXXX/EndXXX functions. Please understand that this never will be a 100% sandboxing, are we in agreement that this would be designed to minimize the side-effect and cost of programmer errors, but not to protect from malicious users?

[nikki93]

@ocornut — I’m in full agreement with all of this! Yeah, not meant to be a security feature at all, just convenience at most (in my case). One little question: Would the error polling just show most recent error or collect errors since the last poll? For GL calls for example it only collects last one which makes one need to call glError(...) nearly after every call in a user-friendly wrapper. Accumulating errors would remove this overhead. In any case, having anything at all in this realm would be a great improvement so I’m open either way.

[sherief]

Yes, this shouldn't be intended to protect from malice, just misuse. If you're dealing with a malicious user you'll need to have bindings that apply enough mitigation to have some sort of measurable impact, and that doesn't belong in imgui - people who want such mitigations can implement them on top of the public imgui API just fine.

[ejulien]

Hello,

Context: Our editor can be extended using plugins. Plugin hot-reload+ImGui is extremely fast to iterate with.

---

Problem: Unfortunately API misuses are really ruining our party. These come in two flavors:

1. Genuine errors (eg. wrongly scoped End or EndChild call).
2. Script errors interrupting an otherwise valid sequence of ImGui calls.

Both usually end with a C++ assertion being thrown which is less than ideal.

---

Solution?: Both scenarios can be adressed in the same manner but solving the first one might be more work.

Handling scenario 2. could look something like:

ImGui::EnterUntrustedSection();

bool success = call_plugin_issuing_ImGui_calls());
if (!success)
    ImGui::RollbackUntrustedSection();

ImGui::ExitUntrustedSection();

if (!success)
    ImGui::Text("So sad");

Script execution failure was reported by the client code but that doesn't cover API misuse.

---

The ideal version would validate the untrusted section on exit and roll it back if it did not pass.

ImGui::EnterUntrustedSection();

call_plugin_issuing_ImGui_calls(); // alea jacta est

if (!ImGui::ExitUntrustedSection())
    ImGui::Text("So sad");

Obviously logic already executed in the client code can not be rolled back (eg. if (ImGui::Button) ++my_var;) but that really doesn't matter. As soon as the error is repaired the UI will correctly reflect those changes anyway.

---

While not a critical feature to have I wonder how hard it would be to implement on top of the current design?

[ejulien]

Seeing that #2096 is cross-referenced here might I add that I would be much more interested in completely rolling back a broken declaration rather than having a partial declaration automatically rendered valid.

I'd rather present the user a known-to-be-working UI of my choosing in case of a failure.

[ocornut]

There's no magic "rolling back" we have to address specific issues one by one, clarify the fail cases and what we want to do out of them. It probably boils down to clearing most stacks.

E.g. We could merely aim at making EndFrame() fail proof and then if you have an issue on your side you just call some form of EndFrame() that is safe to call and move on to the next frame (you may choose to not render this frame or not).

The problem is that genuine error needs to be reported somehow, we can't have a fully silent form of error handling.

[ejulien]

Precise error reporting would be nice to have and overriding IM_ASSERT to report errors seems good. This together with a safe EndFrame would solve most issues a script writer can face if not the cosmetic ones.

However, I don't really understand your objection to the magic "roll back", is it on a technical standpoint? It looks like the most foolproof approach to me.

[ocornut]

However, I don't really understand your objection to the magic "roll back", is it on a technical standpoint? It looks like the most foolproof approach to me.

I don't know what "rollback" means other than taking a snapshot of the entire memory used by dear imgui and restoring that, complete with pointers provided by the user-allocator (so yeah, it is a technical problem to implement).

[ocornut]

I made some changes so that mismatched Begin/End calls should not lead to crashes or other side effects any more. However note that they will still trigger IM_ASSERT() ( and I have improved the formulation and location of those asserts). For scripting language it is expected that you teach your assert handler to play nice with the app (e.g. error/log/abort script?).

While this doesn't cover everything I believe this should cover MOST errors, and would be interested to know if it works better for you. @sherief , @ejulien , etc.

[rokups]

Note for the future: just found out it does not cover ImGui::TreePop().

[ocornut]

Note that I've been experimenting with a more general solution to this in the testing framework (as by default most checks can trigger an early out from a test function, leaving things un-ended/un-popup) so hopefully once it is matured we can provide more tools to handle that.

[codecat]

Running into the same problems here, a script error stopping a script in the middle of valid ImGui calls, causing ImGui to throw a failed assertion. Was looking for a way to "unroll" or "end all" or "pop all" the BeginX calls and found this thread. Good to know I'm not the only one.. :D

[ocornut]

FYI this is what I've been using in the TestEngine, with quite some success:

void    ImGuiTestContext::RecoverFromUiContextErrors(bool verbose)
{
    ImGuiContext& g = *UiContext;

    while (g.CurrentWindowStack.Size > 0)
    {
#ifdef IMGUI_HAS_TABLE
        while (g.CurrentTable && (g.CurrentTable->OuterWindow == g.CurrentWindow || g.CurrentTable->InnerWindow == g.CurrentWindow))
        {
            if (verbose) LogWarning("Recovered from missing EndTable() call.");
            ImGui::EndTable();
        }
#endif

        while (g.CurrentTabBar != NULL)
        {
            if (verbose) LogWarning("Recovered from missing EndTabBar() call.");
            ImGui::EndTabBar();
        }

        while (g.CurrentWindow->DC.TreeDepth > 0)
        {
            if (verbose) LogWarning("Recovered from missing TreePop() call.");
            ImGui::TreePop();
        }

        while (g.GroupStack.Size > g.CurrentWindow->DC.StackSizesOnBegin.SizeOfGroupStack)
        {
            if (verbose) LogWarning("Recovered from missing EndGroup() call.");
            ImGui::EndGroup();
        }

        while (g.CurrentWindow->IDStack.Size > 1)
        {
            if (verbose) LogWarning("Recovered from missing PopID() call.");
            ImGui::PopID();
        }

        while (g.ColorStack.Size > g.CurrentWindow->DC.StackSizesOnBegin.SizeOfColorStack)
        {
            if (verbose) LogWarning("Recovered from missing PopStyleColor() for '%s'", ImGui::GetStyleColorName(g.ColorStack.back().Col));
            ImGui::PopStyleColor();
        }
        while (g.StyleVarStack.Size > g.CurrentWindow->DC.StackSizesOnBegin.SizeOfStyleVarStack)
        {
            if (verbose) LogWarning("Recovered from missing PopStyleVar().");
            ImGui::PopStyleVar();
        }
        while (g.FocusScopeStack.Size > g.CurrentWindow->DC.StackSizesOnBegin.SizeOfFocusScopeStack)
        {
            if (verbose) LogWarning("Recovered from missing PopFocusScope().");
            ImGui::PopFocusScope();
        }

        if (g.CurrentWindowStack.Size == 1)
        {
            IM_ASSERT(g.CurrentWindow->IsFallbackWindow);
            break;
        }

        if (g.CurrentWindow->Flags & ImGuiWindowFlags_ChildWindow)
        {
            if (verbose) LogWarning("Recovered from missing EndChild() call.");
            ImGui::EndChild();
        }
        else
        {
            if (verbose) LogWarning("Recovered from missing End() call.");
            ImGui::End();
        }
    }
}

It ends to save the day lots of the time, but technically speaking lots of that is done incorrectly as multiple stacks won't be unpopped in the right order.

I could try moving this function to becoming an internal inside ImGui:: and provide a callback or something to report on errors.
cc @fLindahl

[fLindahl]

That would be neat. I'll use this snippet for now, and report any problems, if any, that occurs here.

Thanks!

[ocornut]

Pushed this in master as 9712bff
Note this is technically undocumented/internals api so come with little guarantee and won't be widely advertised but we've been using it in the TestEngine.

[frink]

It might be a good idea to add the str_id to the Log Message so that you can quickly find the offending item in a more complex code base...

[Web-eWorks]

The way we've been handling stack unrolling on error in Pioneer has been to save a copy of the stack sizes, including the CurrentWindowStackSize, when entering a protected call (try {} catch-equivalent) and to then handle the necessary End/Pops if an error occurred during the protected call. Looking at ErrorCheckEndFrameRecover, this appears to be very similar to what we are doing; however ImGui's function unwinds the entire stack instead of supporting mid-frame unrolling.

Would you be interested in a pull request that allows unrolling to a "known state" (implemented with ImGuiStackSizes) mid-frame? It would need a few minutes to detach from Pioneer's logging system but is otherwise based on the existing ImGui functionality with a few extra variables to enable unrolling to a "mid-window" state.

[Mellnik]

Could we get some more insights on how to expose Dear ImGui to "unsafe" environments.
Imagine we expose the ImGui API to Lua and load external scripts.

One could throw inside of IM_ASSERT and catch the error but not all applications support exception handling. Especially games.

For instance see the function ImGui::EndTabItem:

    IM_ASSERT(tab_bar->LastTabItemIdx >= 0);
    ImGuiTabItem* tab = &tab_bar->Tabs[tab_bar->LastTabItemIdx];
    if (!(tab->Flags & ImGuiTabItemFlags_NoPushId))
        PopID();

You can easily crash the application by calling EndTabItem without a prior call to BeginTabItem.

[sherief]

Could we get some more insights on how to expose Dear ImGui to "unsafe" environments. Imagine we expose the ImGui API to Lua and load external scripts.

One could throw inside of IM_ASSERT and catch the error but not all applications support exception handling. Especially games.

For instance see the function ImGui::EndTabItem:

    IM_ASSERT(tab_bar->LastTabItemIdx >= 0);
    ImGuiTabItem* tab = &tab_bar->Tabs[tab_bar->LastTabItemIdx];
    if (!(tab->Flags & ImGuiTabItemFlags_NoPushId))
        PopID();

You can easily crash the application by calling EndTabItem without a prior call to BeginTabItem.

Your bindings can't just be automated C++<-->scripting (Lua or otherwise) bridges, you need to have validation in your bindings that are exposed to untrusted code.

[Mellnik]

That means you would have to go through every single ImGui function and check whether it does sanity checks or not? And that every ImGui update because stuff could change.

Sounds insane. Sanity checks could be done by default being opt-in with macros to maintain performance for who don't need it.

In the case of EndTabItem I would need to check whether BeginTabItem was called beforehand? That would require a whole logic layer on top of ImGui again?

Edit:
At this point it would just make more sense to gracefully shutdown the production-ready application with a message in case of an assert. Assuming all sanity checks are covered by IM_ASSERT.

[codecat]

I went through this "insanity" myself, I ended up making bindings for Angelscript this way. Every function that pushes/pops to the stack has a secondary stack with some information attached to it as to where in the scripts the stack was pushed from.

This allows me to have any kind of "broken" ImGui stack unrolled correctly as well as making sure you can't pop something off the stack in the wrong order (like calling EndTabItem without calling BeginTabItem). It also has the added benefit of being able to show helpful hints as to where the script programmer might've made a mistake or forgot to pop something off the stack.

My bindings end up looking something like this:

static bool ScriptUI_BeginTabItem(const string& label, bool& open, ImGuiTabItemFlags flags)
{
	bool opened = true;
	bool ret = ImGui::BeginTabItem(label, &opened, flags);
	open = opened;
	if (ret) {
		PushToPlugin(ScriptPlugin::uist_TabItem);
	}
	return ret;
}

static void ScriptUI_EndTabItem()
{
	if (PopFromPlugin(ScriptPlugin::uist_TabItem)) {
		ImGui::EndTabItem();
	}
}

Most of the "magic" happens in PopFromPlugin, which checks if the state is valid, throws a script exception if it's not, and then returns false if ImGui's function should not be called.

I then have a loop to unroll through my secondary stack:

switch (item.m_type) {
case ScriptPlugin::uist_Window: ImGui::End(); break;
case ScriptPlugin::uist_Group: ImGui::EndGroup(); break;
case ScriptPlugin::uist_StyleColor: ImGui::PopStyleColor(); break;
case ScriptPlugin::uist_StyleVar: ImGui::PopStyleVar(); break;
case ScriptPlugin::uist_ID: ImGui::PopID(); break;
case ScriptPlugin::uist_Font: ImGui::PopFont(); break;
case ScriptPlugin::uist_FontNull: break; // Do nothing because we didn't *really* push anything
case ScriptPlugin::uist_MenuBar: ImGui::EndMenuBar(); break;
case ScriptPlugin::uist_Menu: ImGui::EndMenu(); break;
case ScriptPlugin::uist_Tree: ImGui::TreePop(); break;
case ScriptPlugin::uist_Tooltip: ImGui::EndTooltip(); break;
case ScriptPlugin::uist_Popup: ImGui::EndPopup(); break;
case ScriptPlugin::uist_Combo: ImGui::EndCombo(); break;
case ScriptPlugin::uist_Child: ImGui::EndChild(); break;
case ScriptPlugin::uist_TabBar: ImGui::EndTabBar(); break;
case ScriptPlugin::uist_TabItem: ImGui::EndTabItem(); break;
case ScriptPlugin::uist_Table: ImGui::EndTable(); break;
case ScriptPlugin::uist_ListBox: ImGui::EndListBox(); break;
case ScriptPlugin::uist_TextWrap: ImGui::PopTextWrapPos(); break;
case ScriptPlugin::uist_ItemWidth: ImGui::PopItemWidth(); break;
case ScriptPlugin::uist_Disabled: ImGui::EndDisabled(); break;
}

It's a bit more work, but for my case it was well worth it, and I already have quite a large coverage of the API manually done like this. Perhaps something like that can be done in a more automatic code generation way, too.. 🤷‍♀️

[ocornut]

Sanity checks could be done by default

Sanity checks ARE done by default except our priority are native users, notifying of usage mistakes instead of silently failing, and maintaining high performances.

I am open to increase coverage of “failing without not crash” which is the purpose of IM_ASSERT_USER_ERROR() but I am not expecting this coverage will ever get anywhere 100%, rather we can aim to cover the most common cases. And I would correct them on a case by case basic, eg ok to make EndTabItem() non-crashing.

[Mellnik]

@ocornut Sounds good, it would be great to have one of the assert types to cover as many cases as possible. And wouldn't you agree that any library should do assert sanity checks in their public API functions? For maximum performance you disable the assert macros anyway.
Of course I also agree that you can't cover 100%. When someone exposes Dear ImGui to untrusted scripts they have to take additional care to inputs to the ImGui API. For instance in ImGui::Combo ensuring only valid array indexes are passed.

@codecat Thank you. Seems like a good additional way to safeguard without too much effort. Do you also call ImGui::ErrorCheckEndFrameRecover before EndFrame?

[codecat]

No, I am not calling that function. Judging by the comments above it, the function doesn't sound super reliable, but I haven't played around with it.

[ocornut]

We’ve been using that function for a long time in the test engine. It’s not perfect but doesn’t make things worse, and recovers many cases.