Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: [Win32] Nasty interaction between sf::Window::pollEvent() and non-sfml windows  (Read 7750 times)

0 Members and 1 Guest are viewing this topic.

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
Houston, we've got a problem.

So I'm writing a plugin for some random program and using SFML window to render some debug stuff. The plugin has a structure like this:

struct MySystem
{
  MySystem()
  {
    window = new sf::RenderWindow(...);
  }
  ~MySystem()
  {
    delete window;
  }
  void Update()
  {
    sf::Event event;
    while(window->pollEvent(event);
  }
  sf::RenderWindow *window;
};
MySystem *mySystem;
void DLL_API PluginInit()
{
  mySystem = new MySystem();
}
void DLL_API PluginUpdate()
{
  mySystem->Update();
}
void DLL_API PluginRelease()
{
  delete mySystem;
}
 

So far, so good. Now imagine there's a button "stop plugin" in the program I'm making the plugin for. The button apparently has an event function like this:
void ProgramImMakingPluginFor::OnPluginStopButtonPressed()
{
  PluginRelease();
}
 

What happens is this: the program crashes when I hit the "StopPlugin" button and crashed stack trace points to my line with while(window->pollEvent(event)); A couple of restless nights pointed out that what actually happens is this:

During my Update() when I call window->pollEvent(event) instead of processing messages addressed to my window it processes messages to non-sfml windows as well. sf::Window::pollEvent() calls sf::Window::popEvent() that calls WindowImplWin32::processEvents() that looks like this:
void WindowImplWin32::processEvents()
{
    // We process the window events only if we own it
    if (!m_callback)
    {
        MSG message;
        while (PeekMessageW(&message, NULL/*SIC*/, 0, 0, PM_REMOVE))
        {
            TranslateMessage(&message);
            DispatchMessageW(&message);
        }
    }
}
 
And during MySystem::Update() pollEvent() calls ProgramImMakingPluginFor::OnPluginStopButtonPressed() that calls PluginRelease() and destroys mySystem. Apparently bad things happen if inside mySystem->Update() you destroy instance of mySystem.

Two questions:
1) Shouldn't it have been
while (PeekMessageW(&message, m_handle, 0, 0, PM_REMOVE))
 
instead of
while (PeekMessageW(&message, NULL, 0, 0, PM_REMOVE))
 
?
2) Which options do I have for a workaround?
« Last Edit: January 14, 2015, 03:45:49 am by Suslik »

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 10801
    • View Profile
    • development blog
    • Email
Quote from: MSDN
If hWnd is NULL, PeekMessage retrieves messages for any window that belongs to the current thread, and any messages on the current thread's message queue whose hwnd value is NULL (see the MSG structure). Therefore if hWnd is NULL, both window messages and thread messages are processed.
I'm don't know what kind of "thread" messages there are and why it was originally decided to go this way.

To help with your problem, you'd have to provide a bit more information. What kind of event does get triggered? Why would the processing of a "foreign" event crash your application? What is really going on? Can you build SFML in debug mode and debug things to find out where exactly within SFML it crashes?
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
I'm don't know what kind of "thread" messages there are and why it was originally decided to go this way.
To reformulate in an easy way what MSDN says, PeekMessage with NULL hWnd receives all window messages that belong to the thread regardless of window they came from.

To help with your problem, you'd have to provide a bit more information. What kind of event does get triggered?
As I already said, an event from pushing button "stop plugin" from the program I'm writing the plugin for is triggered.

Why would the processing of a "foreign" event crash your application?
Because the "foreign" event is used for stopping my plugin. It's instance gets invalid while processing messages from the "foreign" window.

Can you build SFML in debug mode and debug things to find out where exactly within SFML it crashes?
I think I already did this in zero post. My debugWindow->pollEvent() calls sf::Window::popEvent(), that calls WindowImplWin32::processEvents() and it's code is in zero post: PeekMessage() with NULL as the second argument receives all messages instead of messages related to debugWindow. It includes the message from a button that's supposed to kill my plugin and it gets killed while processing messages which leads to an immediate crash on the next iteration of while(debugWindow->pollEvent(event)) because this-> and this->debugWindow become invalid after calling the destructor.
« Last Edit: January 14, 2015, 05:50:25 pm by Suslik »

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
Ok, look here:
This is the commit that caused this problem.
https://github.com/SFML/SFML/commit/5d377fdb38312d2c7292451def6e2ce61541b9ec

And this is what it does:
Quote
It seems hard to find reliable documentation about this issue, so let's try something and see what good or bad effect it will have.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
The problem is that this commit "fixes" a few nasty bugs. Without a better understanding of these problems and of the fix, this is the best we can do, so we won't revert it.
Laurent Gomila - SFML developer

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
I think the problem is even if the commit fixes some things it does so in a wrong way. You're definitely not supposed to process messages directed to windows other than yours. If you were trying to fix https://github.com/SFML/SFML/issues/69 or https://github.com/SFML/SFML/issues/328 apparently this is not the right way because messages are not supposed to be processed this way and it produces 100% reproducible bugs while trying to fix something that looks more like Windows SDK bug reproducible only on few machines.

If you want to obtain thread messages instead of hWnd messages, use this:
Quote
If hWnd is -1, PeekMessage retrieves only messages on the current thread's message queue whose hwnd value is NULL, that is, thread messages as posted by PostMessage (when the hWnd parameter is NULL) or PostThreadMessage.

If you use NULL in PeekMessage you obtain other unwanted behaviour as well. For example this code:
int main()
{
  sf::RenderWindow window1(sf::VideoMode(800, 600), "Window1");
  sf::RenderWindow window2(sf::VideoMode(500, 500), "Window2");
  while(1)
  {
    sf::Event event;
    while(window1.pollEvent(event));
  }
  return 0;
}
 
processes messages both for window1 and window2 while there's message loop only for window1.
« Last Edit: January 14, 2015, 08:05:33 pm by Suslik »

BlueCobold

  • Full Member
  • ***
  • Posts: 105
    • View Profile
He will receive other messages if he creates multiple windows in the same thread. He can make a workaround and create them all in different threads though.
(that doesn't mean I would want to talk off a proper fix though ;) )
« Last Edit: January 14, 2015, 07:59:19 pm by BlueCobold »

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
I already made a workaround by creating every window in a separate thread to use separate message queues but that directly contradicts to:
Quote
On OS X, windows and events must be managed in the main thread
from http://www.sfml-dev.org/tutorials/2.0/window-window.php

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
You don't need to convince me that this "fix" is wrong, I think it's obvious enough ;) Nobody have an idea what caused these problems, as well as why it fixes them.

But... it does fix them. So unless you have a better fix, or a better understanding of the problems, you can discuss as much as you want, nothing is likely to change... sadly.
Laurent Gomila - SFML developer

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
I have no idea what you were trying to fix because I can't reproduce it but if processing all messages instead of those you're supposed to makes it work "better", why not just log all the "extra" messages you process and find the one responsible? Also using -1 in PeekMessage instead of NULL  was adviced multiple times, at least it's worth trying because it won't break as much.
« Last Edit: January 14, 2015, 08:24:30 pm by Suslik »

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Quote
I have no idea what you were trying to fix because I can't reproduce it
Neither do I, but many other people did. And many other libraries / codes do it this way (using NULL in PeekMessage), even SDL if I remember correctly.

Quote
if processing all messages instead of those you're supposed to makes it work "better", why not just log all the "extra" messages you process and find the one responsible?
This could be a good starting point, but "finding the one responsible" may not be trivial ;)
But feel free to investigate on your side, understanding and solving this problem cleanly would be great.

Quote
Also using -1 in PeekMessage instead of NULL  was adviced multiple times, at least it's worth trying because it won't break as much.
It's not a complete fix, since you can still have windows in different threads.
Laurent Gomila - SFML developer

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
From SDL:
void
WIN_PumpEvents(_THIS)
{
    const Uint8 *keystate;
    MSG msg;
    DWORD start_ticks = GetTickCount();

    if (g_WindowsEnableMessageLoop) {
        while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
            /* Always translate the message in case it's a non-SDL window (e.g. with Qt integration) */
            TranslateMessage(&msg);
            DispatchMessage(&msg);

            /* Make sure we don't busy loop here forever if there are lots of events coming in */
            if (SDL_TICKS_PASSED(msg.time, start_ticks)) {
                break;
            }
        }
    }

    /* Windows loses a shift KEYUP event when you have both pressed at once and let go of one.
       You won't get a KEYUP until both are released, and that keyup will only be for the second
       key you released. Take heroic measures and check the keystate as of the last handled event,
       and if we think a key is pressed when Windows doesn't, unstick it in SDL's state. */

    keystate = SDL_GetKeyboardState(NULL);
    if ((keystate[SDL_SCANCODE_LSHIFT] == SDL_PRESSED) && !(GetKeyState(VK_LSHIFT) & 0x8000)) {
        SDL_SendKeyboardKey(SDL_RELEASED, SDL_SCANCODE_LSHIFT);
    }
    if ((keystate[SDL_SCANCODE_RSHIFT] == SDL_PRESSED) && !(GetKeyState(VK_RSHIFT) & 0x8000)) {
        SDL_SendKeyboardKey(SDL_RELEASED, SDL_SCANCODE_RSHIFT);
    }
}
 
I've asked around our coding community. Looks like the correct way would probably be to make a loop of PeekMessage(.., hWnd, ..) to process messages directed to this particular window, then make additional loop with PeekMessage(.., -1, ..). If it works(it's only a suggestion though), it still won't be totally awesome because that PeekMessage(.., -1, ..) loop should be either in MFC/Qt(external gui library) OR in sfml, but this way it'll still cause way less bugs than using NULL as hWnd and should process global messages not related to any particular window as well.

Reason why you can't just use PeekMessage(.., hWnd, ..) is because of messages not directed to any particular window. Like this one for example: http://msdn.microsoft.com/ru-ru/library/windows/desktop/ms632641%28v=vs.85%29.aspx
« Last Edit: January 14, 2015, 08:58:07 pm by Suslik »

BlueCobold

  • Full Member
  • ***
  • Posts: 105
    • View Profile
You can't first peek all hWnd-related messages and then peek all others, because in the meantime there may flow in new hWnd-related messages which then would be skipped/discarded by the other peeks. That would cause other nasty bugs.

Suslik

  • Newbie
  • *
  • Posts: 33
    • View Profile
You can't first peek all hWnd-related messages and then peek all others, because in the meantime there may flow in new hWnd-related messages which then would be skipped/discarded by the other peeks. That would cause other nasty bugs.
Unfortunately I don't understand why one peeks would screw the others because as far as I understand peeks just remove requested types of messages while leaving all others intact. So I see no reason why the queue would overflow if you peek hWnd-related messages first and then all others.

Meanwile I did some testing.
#include <Windows.h>
int main()
{
  sf::RenderWindow window1(sf::VideoMode(800, 600), "Window1");
  sf::RenderWindow window2(sf::VideoMode(500, 500), "Window2");
  while(1)
  {
    MSG msg;
    while(PeekMessage(&msg, window1.getSystemHandle(), 0, 0, PM_REMOVE))
    {
      TranslateMessage(&msg);
      DispatchMessage(&msg);
    }
    while(PeekMessage(&msg, window2.getSystemHandle(), 0, 0, PM_REMOVE))
    {
      TranslateMessage(&msg);
      DispatchMessage(&msg);
    }
    while(PeekMessage(&msg, (HWND)-1, 0, 0, PM_REMOVE))
    {
      printf("Received message %d\n", msg.message);
      TranslateMessage(&msg);
      DispatchMessage(&msg);
    }
    /*sf::Event event;
    while(window1.pollEvent(event));*/

  }
  return 0;
}
 
This unfortunately does not work. The loop with -1 simply does not get any messages, looks like I'm either using this feature wrong or it's broken.

Also I did some research on how it's done in GUI libraries. For example merging Qt and MFC is done via a plugin QtMfcApp: http://doc.qt.digia.com/solutions/4/qtwinmigrate/winmigrate-walkthrough.html
Considerable amount of pain should be suffered in order to mix wxWidgets and MFC as well: http://svn.wxwidgets.org/viewvc/wx/wxWidgets/trunk/samples/mfc/mfctest.cpp?view=markup

Most GUI libraries use a message loop with PeekMessage(.., NULL, ...) and yes most libraries indeed screw each other's message loops so additional steps have to be undertaken to merge them together under windows if message order is important for your app(for most apps it is). I suppose the workaround with a separate thread for every window is not that much of a trouble compared to other solutions lol.