Backends: Win32: Uninitialized context crash fix

[MennoVink]

Ignoring the window procedure handler for contexts that havn't been initialized yet will prevent this from crashing when the backend data is being accessed, for example on line 526.

In my situation contexts(2) are being created before the window. Then when the window is created and starts sending messages i havn't had a chance yet to initialize each context. I'm thinking this early out will just discard messages for such contexts, similar to as-if there isn't an active context yet.

[ocornut]

I believe this is the same as #6222 (specific comment linked).
You are solely responsible for calling ImGui_ImplWin32_WndProcHandler() so you could perfectly decide to not call until ready?

[MennoVink]

I dont think it's really similar, as this code already protects itself from 'misuse'. it just doesn't fully cover it. It checks against context being available, but not against context being valid.
If design is to crash-on-misuse then I'd argue that the ImGui::GetCurrentContext() == NULL check is out of place there as well. One could say that since the app calls this function, it needs to make sure that there is a current context active.
Personally i prefer code to be robust and guard itself against misuse (especially with early-out input validation like this), but that might just be because i operate in an environment where any behaviour is better than crashing.

Design wise this check not being there i would have to go from:

• On construction hook up contexts with event dispatch.
• On context update when not inited yet, init with first window.
• On window event, forward to each context's WndProcHandler.

to

• On construction do nothing.
• On context update when not inited yet, hook up with dispatch as well as init with first window.
• On window event, forward to only initialized context's WndProcHandler.

It'll work, but hooking up with event dispatch in update rather than constructor is not as nice.

[ocornut]

It seems a little odd that you don't init backend at the same spot you create the imgui context, what is the reason for doing that?
Why not simply init backend along imgui context together at the same point you are hooking event dispatch?

[ocornut]

Been meaning to settle this as it is a low-hanging fruit,

If design is to crash-on-misuse then I'd argue that the ImGui::GetCurrentContext() == NULL check is out of place there as well. One could say that since the app calls this function, it needs to make sure that there is a current context active.

I actually agree. For the record the check was added in dd89c9e (#1565) but I don't have enough details about the context. I don't like it too much and removed it now d15574c, I also standardized some similar checks for other backends. It strikes me as odd that the Win32 backend was the only backend with such a check. I added a specific comment near the assert to facilitate it if anyone stumbles on this.

Personally i prefer code to be robust and guard itself against misuse (especially with early-out input validation like this), but that might just be because i operate in an environment where any behaviour is better than crashing.

For this specific matter, the crashing would happen immediately in your dev environment (aka it wouldn't be a "only crash in some setups/instances or for some users" crash) therefore it seems saner that you catch it.

Design wise this check not being there i would have to go from:

It seems like the only notable difference is adding that same one-line check before your call to ImGui_ImplWin32_WndProcHandler()?

Sorry to be bike-shedding about such a minor thing but code calling that stuff some lives for decades and I'd rather enforce user-correct code. If I notice that d15574c causes trouble to too many people I might revert to honor legacy of this quirk.

[ocornut]

🤦‍♂️ facepalm, i was so busy tweaking the comments/asserts for other backends that I didn't realize d15574c broke our example, and gives us the answer as this extra check that existed since dd89c9e: win32 api CreateWindow does immediately calls WndProc.

While it is perfectly possible to add an extra check in app/example custom WndProc handler before forwarding to ImGui_ImplWin32_WndProcHandler(), there is also no doubt that this code has been copy and pasted thousands of times in the wild, and therefore removing that old silent check is not worth the breaking.

I however added (in previous commit) the missing assert that will trigger if context exists but backend is not initialized, encouraging you to add an extra check if in your code if you need to initialize backends later than context creation.

[MennoVink]

👍 no worries here, it's good to clean up old stuff. Sorry to have stirred this up to become a breaking change for something so silly.

The new assert clearly communicates that the responsibility is on the application and tells it what's required for it to work.

[ocornut]

Plot twist, I have reverted my change and applied yours: ac90e1b

What was happening before is that it was also easy to get in situation where WndProc was called BEFORE backend init but for messages we don't process, leading to no crash. Adding the assert exhibited those once harmless cases.

Simply put I had an app in this situation, I realized I could fix it with an extra check, but checking "if backend is inited" is not going to be obvious for most users, so I decided it wasn't worth the hassle because of how Win32 setup is typically structured, and included your early out. Thank you!