[ScreenSaver] freezes when switching screensavers

[diver]

Start ScreenSaver and select any screensaver
Now using only up/down arrows on your keybord go from the first screensaver to the last one and back several times.

Tested with hrev32451 in VirtualBox 3.0.4

[diver]

Still here in hrev35570.

[diver]

Still here in hrev38300.

[diver]

Still here in hrev43984.

[jscipione]

Debugger output when locked up

[jscipione]

The previous picture isn't dead locked, just catching up, this is locked up

[jscipione]

Fixed in hrev46012

[jscipione]

Full debug report when deadlocked

[anevilyak]

That report shows a pretty straightforward reason for the deadlock: the window thread is in MessageReceived() waiting for the renderer thread to exit. Being in MessageReceived implies that thread holds the window lock. Meanwhile, the renderer thread tries to grab the window lock, which it won't be able to get because of the aforementioned thread state. Without looking more closely, two possible resolutions immediately come to mind:

a) the window doesn't wait for the renderer thread, and instead the renderer sends the window a message indicating it's terminated right before exiting, at which point the window takes the actions it would've taken after the thread exited,

or b) the renderer thread simply doesn't touch the window directly at all, and instead sends it messages to ask it to do things on its behalf, so it never has to do the aforementioned locking at all.

[jscipione]

Re-opening to fix properly

[axeld]

I actually see little reason why the ModulesView has to wait until Quit() returns, anyway -- once that call is made, the screen saver cannot access the window anymore (well, not visually, that is).

Not that I'm deep into the code, but I think I would add a ScreenSaverRunner::QuitAsync() that just sets fQuitting to true (and call Resume()), and Quit() would just call this one, and wait until the thread returns. ModulesView could then call QuitAsync(), and offload the destruction of the saver into another thread (ie. only this one would then call Quit()).

[jscipione]

Replying to axeld:

I actually see little reason why the ModulesView has to wait until Quit() returns, anyway -- once that call is made, the screen saver cannot access the window anymore (well, not visually, that is).

Not that I'm deep into the code, but I think I would add a ScreenSaverRunner::QuitAsync() that just sets fQuitting to true (and call Resume()), and Quit() would just call this one, and wait until the thread returns. ModulesView could then call QuitAsync(), and offload the destruction of the saver into another thread (ie. only this one would then call Quit()).

Seems like a decent idea, I tried this, and it actually allows the screensaver preview to update live, but, I don't understand where I'm suppose to call Quit().

[axeld]

You only still need to call Quit() in the offloaded thread in order to wait for the thread to finish (you can also just wait yourself, if you know the thread_id). You have to do this to make sure the runner isn't active anymore before deleting it and the screen saver it ran.

[diver]

Still locks up in hrev46368, it's harder to trigger it tho. Sometimes it just crashes.

[diver]

Correction, now it just crashes.

[jscipione]

Crash quickly going through items on hrev46368

[jscipione]

I've managed to reproduce the crash and posted a report, can somebody help deduce the problem from this crash report? The stack trace is unintelligible.

[anevilyak]

In this particular case, it crashed in the Message screen saver's Draw().

[jscipione]

Okay, that's what I feared, I guess this means I'll have to move the drawing code to the window thread as well via message passing in order to prevent this crash.

[axeld]

Why? I'm pretty sure that Message (and any other screen saver) can Draw() from another thread, even if that particular solution is done horribly inefficient (by allocating and destroying a BBitmap in each Draw() call). What do you think happens here?

[jscipione]

With the new understanding I've come to have, please disregard the above idea. I realize now this crash is probably caused by the view being deleted before being passed to the Draw() method of the screensaver and has nothing to do with locking.

I had previously experimented with passing a message from the runner thread back to the window thread, via a handler passed into the constructor and having the window thread do the drawing instead so that the runner thread would not have to lock the window before drawing or even be aware of the windows existence in order to prevent locking issues.

But, firstly, lock contention wasn't a problem here anyway, and secondly I don't want to do the drawing in the window thread because that thread needs to be free to do other things like receive mouse and keyboard input and draw other things while the runner thread is drawing the screensaver.

[jscipione]

Fixed in hrev46368 with follow up in hrev46374 and a related bug that causes a crash in Message was fixed in hrev46570