Opened 10 years ago

Closed 6 years ago

#4260 closed bug (fixed)

[ScreenSaver] freezes when switching screensavers

Reported by: diver Owned by: axeld
Priority: normal Milestone: R1
Component: Preferences/ScreenSaver Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #10125
Has a Patch: no Platform: All

Description

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

Attachments (5)

screensaver (11.2 KB ) - added by diver 8 years ago.
ScreenSaver DeadLocked.png (57.8 KB ) - added by jscipione 6 years ago.
Debugger output when locked up
Screen Saver Locked up.png (50.1 KB ) - added by jscipione 6 years ago.
The previous picture isn't dead locked, just catching up, this is locked up
ScreenSaver-1032-debug-04-09-2013-16-59-59.report.txt (16.0 KB ) - added by jscipione 6 years ago.
Full debug report when deadlocked
ScreenSaver-735-debug-15-11-2013-20-22-59.report.txt (163.7 KB ) - added by jscipione 6 years ago.
Crash quickly going through items on hrev46368

Download all attachments as: .zip

Change History (24)

comment:1 by diver, 10 years ago

Still here in hrev35570.

comment:2 by diver, 10 years ago

Version: R1/pre-alpha1R1/Development

comment:3 by diver, 9 years ago

Still here in hrev38300.

comment:4 by diver, 8 years ago

Still here in hrev43984.

by diver, 8 years ago

Attachment: screensaver added

by jscipione, 6 years ago

Attachment: ScreenSaver DeadLocked.png added

Debugger output when locked up

by jscipione, 6 years ago

Attachment: Screen Saver Locked up.png added

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

comment:5 by jscipione, 6 years ago

Resolution: fixed
Status: newclosed

Fixed in hrev46012

by jscipione, 6 years ago

Full debug report when deadlocked

comment:6 by anevilyak, 6 years ago

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.

comment:7 by jscipione, 6 years ago

Resolution: fixed
Status: closedreopened

Re-opening to fix properly

comment:8 by axeld, 6 years ago

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()).

in reply to:  8 comment:9 by jscipione, 6 years ago

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().

comment:10 by axeld, 6 years ago

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.

comment:11 by jscipione, 6 years ago

Blocking: 10125 added

comment:12 by diver, 6 years ago

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

comment:13 by diver, 6 years ago

Correction, now it just crashes.

by jscipione, 6 years ago

Crash quickly going through items on hrev46368

comment:14 by jscipione, 6 years ago

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.

comment:15 by anevilyak, 6 years ago

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

comment:16 by jscipione, 6 years ago

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.

comment:17 by axeld, 6 years ago

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?

comment:18 by jscipione, 6 years ago

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.

comment:19 by jscipione, 6 years ago

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.