From d34a680c043c72bae01662cc730ca81cb4cd2615 Mon Sep 17 00:00:00 2001 From: John Scipione Date: Thu, 14 Nov 2013 16:44:15 -0500 Subject: [PATCH] Screen Saver: fix race condition Start the screensaver in the window thread instead of the runner thread so that there is no lock contention for the window lock in the runner thread when the saver starts. The view that gets drawn into is assumed to have been prepared before being passed to the runner thread, and this assumption has been made true for the screensaver preview and screen_blanker apps. Eliminate fHasStarted and the corresponding HasStarted() method in ScreenSaverRunner as they are no longer needed. Drawing still happens in the runner thread, and still needs to lock the window thread potentially causing contention, yet, there is a timeout here so the contention won't freeze the screensaver window, only delay drawing the screensaver. Drawing could be moved to the window thread via message passing to avoid lock contention with the window but this would defeat a big part of the purpose of having a separate rendering thread. This fixes #10125 and #4260 --- .../private/screen_saver/ScreenSaverRunner.h | 23 +++----- src/bin/screen_blanker/ScreenBlanker.cpp | 29 +++++---- src/bin/screen_blanker/ScreenBlanker.h | 54 ++++++++--------- src/bin/screen_blanker/ScreenSaverWindow.cpp | 38 +++++++----- src/bin/screen_blanker/ScreenSaverWindow.h | 14 +++-- src/kits/screensaver/ScreenSaverRunner.cpp | 59 +++++++------------ .../screensaver/ScreenSaverWindow.cpp | 12 +++- .../screensaver/ScreenSaverWindow.h | 12 ++-- 8 files changed, 117 insertions(+), 124 deletions(-) diff --git a/headers/private/screen_saver/ScreenSaverRunner.h b/headers/private/screen_saver/ScreenSaverRunner.h index ec6239f9f7..f3f717e98e 100644 --- a/headers/private/screen_saver/ScreenSaverRunner.h +++ b/headers/private/screen_saver/ScreenSaverRunner.h @@ -12,24 +12,19 @@ #define SCREEN_SAVER_RUNNER_H -#include -#include +#include +#include - -class BScreenSaver; -class BView; -class ScreenSaverSettings; +#include "ScreenSaverSettings.h" class ScreenSaverRunner { public: - ScreenSaverRunner(BWindow* window, BView* view, - bool preview, + ScreenSaverRunner(BView* view, ScreenSaverSettings& settings); ~ScreenSaverRunner(); - BScreenSaver* ScreenSaver() const; - bool HasStarted() const; + BScreenSaver* ScreenSaver() const { return fSaver; }; status_t Run(); void Quit(); @@ -43,13 +38,11 @@ private: static status_t _ThreadFunc(void* data); status_t _Run(); - BScreenSaver* fSaver; - BWindow* fWindow; - BDirectWindow* fDirectWindow; BView* fView; + bool fIsDirectDraw; ScreenSaverSettings& fSettings; - bool fPreview; - bool fHasStarted; + + BScreenSaver* fSaver; image_id fAddonImage; thread_id fThread; diff --git a/src/bin/screen_blanker/ScreenBlanker.cpp b/src/bin/screen_blanker/ScreenBlanker.cpp index b03f9444e9..2b6a43c07e 100644 --- a/src/bin/screen_blanker/ScreenBlanker.cpp +++ b/src/bin/screen_blanker/ScreenBlanker.cpp @@ -40,8 +40,7 @@ ScreenBlanker::ScreenBlanker() : BApplication(SCREEN_BLANKER_SIG), fWindow(NULL), - fSaver(NULL), - fRunner(NULL), + fSaverRunner(NULL), fPasswordWindow(NULL), fResumeRunner(NULL), fStandByScreenRunner(NULL), @@ -70,15 +69,18 @@ ScreenBlanker::ReadyToRun() BScreen screen(B_MAIN_SCREEN_ID); fWindow = new ScreenSaverWindow(screen.Frame()); fPasswordWindow = new PasswordWindow(); - fRunner = new ScreenSaverRunner(fWindow, fWindow->ChildAt(0), false, fSettings); - fSaver = fRunner->ScreenSaver(); - if (fSaver) { - fWindow->SetSaver(fSaver); - fRunner->Run(); - } else { + BView* view = fWindow->ChildAt(0); + fSaverRunner = new ScreenSaverRunner(view, fSettings); + fWindow->SetSaverRunner(fSaverRunner); + + BScreenSaver* saver = fSaverRunner->ScreenSaver(); + if (saver != NULL && saver->StartSaver(view, false) == B_OK) + fSaverRunner->Run(); + else { fprintf(stderr, "could not load the screensaver addon\n"); - fWindow->ChildAt(0)->SetViewColor(0, 0, 0); + view->SetViewColor(0, 0, 0); + // needed for Blackness saver } fWindow->SetFullScreen(true); @@ -110,7 +112,7 @@ ScreenBlanker::_SetDPMSMode(uint32 mode) screen.SetDPMS(mode); if (fWindow->Lock()) { - fRunner->Suspend(); + fSaverRunner->Suspend(); fWindow->Unlock(); } } @@ -122,7 +124,7 @@ ScreenBlanker::_ShowPasswordWindow() _TurnOnScreen(); if (fWindow->Lock()) { - fRunner->Suspend(); + fSaverRunner->Suspend(); fWindow->Sync(); // TODO: is that needed? @@ -239,7 +241,7 @@ ScreenBlanker::MessageReceived(BMessage* message) fPasswordWindow->SetPassword(""); fPasswordWindow->Hide(); - fRunner->Resume(); + fSaverRunner->Resume(); fWindow->Unlock(); } @@ -301,7 +303,8 @@ ScreenBlanker::_Shutdown() fWindow->Quit(); } - delete fRunner; + delete fSaverRunner; + fSaverRunner = NULL; } diff --git a/src/bin/screen_blanker/ScreenBlanker.h b/src/bin/screen_blanker/ScreenBlanker.h index 9f4af7fea4..b190e73772 100644 --- a/src/bin/screen_blanker/ScreenBlanker.h +++ b/src/bin/screen_blanker/ScreenBlanker.h @@ -24,37 +24,37 @@ const static uint32 kMsgResumeSaver = 'RSSV'; class ScreenBlanker : public BApplication { - public: - ScreenBlanker(); - ~ScreenBlanker(); +public: + ScreenBlanker(); + ~ScreenBlanker(); - virtual void ReadyToRun(); + virtual void ReadyToRun(); - virtual bool QuitRequested(); - virtual void MessageReceived(BMessage* message); + virtual bool QuitRequested(); + virtual void MessageReceived(BMessage* message); - private: - bool _LoadAddOn(); - void _ShowPasswordWindow(); - void _QueueResumeScreenSaver(); - void _TurnOnScreen(); - void _SetDPMSMode(uint32 mode); - void _QueueTurnOffScreen(); - void _Shutdown(); - - ScreenSaverSettings fSettings; - ScreenSaverWindow *fWindow; - BScreenSaver *fSaver; - ScreenSaverRunner *fRunner; - PasswordWindow *fPasswordWindow; - - bigtime_t fBlankTime; - BMessageRunner* fResumeRunner; - - BMessageRunner* fStandByScreenRunner; - BMessageRunner* fSuspendScreenRunner; - BMessageRunner* fTurnOffScreenRunner; bool IsPasswordWindowShown() const; + +private: + bool _LoadAddOn(); + void _ShowPasswordWindow(); + void _QueueResumeScreenSaver(); + void _TurnOnScreen(); + void _SetDPMSMode(uint32 mode); + void _QueueTurnOffScreen(); + void _Shutdown(); + + ScreenSaverSettings fSettings; + ScreenSaverWindow* fWindow; + ScreenSaverRunner* fSaverRunner; + PasswordWindow* fPasswordWindow; + + bigtime_t fBlankTime; + BMessageRunner* fResumeRunner; + + BMessageRunner* fStandByScreenRunner; + BMessageRunner* fSuspendScreenRunner; + BMessageRunner* fTurnOffScreenRunner; }; #endif // SCREEN_SAVER_APP_H diff --git a/src/bin/screen_blanker/ScreenSaverWindow.cpp b/src/bin/screen_blanker/ScreenSaverWindow.cpp index eed8025411..60a2d7ae22 100644 --- a/src/bin/screen_blanker/ScreenSaverWindow.cpp +++ b/src/bin/screen_blanker/ScreenSaverWindow.cpp @@ -71,11 +71,7 @@ ScreenSaverFilter::Filter(BMessage* message, BHandler** target) } -void -ScreenSaverFilter::SetEnabled(bool enabled) -{ - fEnabled = enabled; -} +// #pragma mark - ScreenSaverWindow /*! @@ -88,7 +84,9 @@ ScreenSaverWindow::ScreenSaverWindow(BRect frame) B_NO_BORDER_WINDOW_LOOK, kWindowScreenFeel, B_NOT_RESIZABLE | B_NOT_MOVABLE | B_NOT_MINIMIZABLE | B_NOT_ZOOMABLE | B_NOT_CLOSABLE, B_ALL_WORKSPACES), - fSaver(NULL) + fTopView(NULL), + fSaverRunner(NULL), + fFilter(NULL) { frame.OffsetTo(0, 0); fTopView = new BView(frame, "ScreenSaver View", B_FOLLOW_ALL, B_WILL_DRAW); @@ -99,9 +97,10 @@ ScreenSaverWindow::ScreenSaverWindow(BRect frame) AddChild(fTopView); - // Ensure that this view receives keyboard input + // Ensure that this view receives keyboard and mouse input fTopView->MakeFocus(true); - fTopView->SetEventMask(B_KEYBOARD_EVENTS, 0); + fTopView->SetEventMask(B_KEYBOARD_EVENTS | B_POINTER_EVENTS, + B_NO_POINTER_HISTORY); } @@ -111,13 +110,6 @@ ScreenSaverWindow::~ScreenSaverWindow() } -void -ScreenSaverWindow::SetSaver(BScreenSaver *saver) -{ - fSaver = saver; -} - - void ScreenSaverWindow::MessageReceived(BMessage* message) { @@ -148,3 +140,19 @@ ScreenSaverWindow::DirectConnected(direct_buffer_info* info) saver->DirectConnected(info); } + +void +ScreenSaverWindow::SetSaverRunner(ScreenSaverRunner* runner) +{ + fSaverRunner = runner; +} + + +BScreenSaver* +ScreenSaverWindow::_ScreenSaver() +{ + if (fSaverRunner != NULL) + return fSaverRunner->ScreenSaver(); + + return NULL; +} diff --git a/src/bin/screen_blanker/ScreenSaverWindow.h b/src/bin/screen_blanker/ScreenSaverWindow.h index 9c0c9c0fca..0151a9ed29 100644 --- a/src/bin/screen_blanker/ScreenSaverWindow.h +++ b/src/bin/screen_blanker/ScreenSaverWindow.h @@ -11,6 +11,8 @@ #include #include +#include "ScreenSaverRunner.h" + const static uint32 kMsgEnableFilter = 'eflt'; @@ -37,16 +39,18 @@ public: ScreenSaverWindow(BRect frame); ~ScreenSaverWindow(); - void SetSaver(BScreenSaver *saver); virtual void MessageReceived(BMessage* message); virtual bool QuitRequested(); virtual void DirectConnected(direct_buffer_info* info); + void SetSaverRunner(ScreenSaverRunner* runner); + BScreenSaver* _ScreenSaver(); - private: - BView *fTopView; - BScreenSaver *fSaver; - ScreenSaverFilter *fFilter; +private: + BView* fTopView; + ScreenSaverRunner* fSaverRunner; + ScreenSaverFilter* fFilter; }; + #endif // SCREEN_SAVER_WINDOW_H diff --git a/src/kits/screensaver/ScreenSaverRunner.cpp b/src/kits/screensaver/ScreenSaverRunner.cpp index 2e4bc8ef0f..799ac65df8 100644 --- a/src/kits/screensaver/ScreenSaverRunner.cpp +++ b/src/kits/screensaver/ScreenSaverRunner.cpp @@ -11,29 +11,27 @@ #include "ScreenSaverRunner.h" -#include "ScreenSaverSettings.h" - -#include -#include -#include -#include #include +#include +#include +#include +#include -ScreenSaverRunner::ScreenSaverRunner(BWindow* window, BView* view, - bool preview, ScreenSaverSettings& settings) + +ScreenSaverRunner::ScreenSaverRunner(BView* view, + ScreenSaverSettings& settings) : - fSaver(NULL), - fWindow(window), fView(view), + fIsDirectDraw(view != NULL + && dynamic_cast(view->Window()) != NULL), fSettings(settings), - fPreview(preview), + fSaver(NULL), fAddonImage(-1), fThread(-1), fQuitting(false) { - fDirectWindow = dynamic_cast(fWindow); _LoadAddOn(); } @@ -47,20 +45,6 @@ ScreenSaverRunner::~ScreenSaverRunner() } -BScreenSaver* -ScreenSaverRunner::ScreenSaver() const -{ - return fSaver; -} - - -bool -ScreenSaverRunner::HasStarted() const -{ - return fHasStarted; -} - - status_t ScreenSaverRunner::Run() { @@ -184,13 +168,12 @@ ScreenSaverRunner::_Run() { static const uint32 kInitialTickRate = 50000; - if (fWindow->Lock()) { - fView->SetViewColor(0, 0, 0); - fView->SetLowColor(0, 0, 0); + if (fView == NULL || fView->Window() == NULL) { + // view is NULL or not connected to app server, bail out if (fSaver != NULL) - fHasStarted = fSaver->StartSaver(fView, fPreview) == B_OK; + fSaver->StopSaver(); - fWindow->Unlock(); + return B_BAD_VALUE; } // TODO: This code is getting awfully complicated and should @@ -199,7 +182,7 @@ ScreenSaverRunner::_Run() int32 snoozeCount = 0; int32 frame = 0; bigtime_t lastTickTime = 0; - bigtime_t tick = fSaver ? fSaver->TickSize() : tickBase; + bigtime_t tick = fSaver != NULL ? fSaver->TickSize() : tickBase; while (!fQuitting) { // break the idle time up into ticks so that we can evaluate @@ -214,7 +197,7 @@ ScreenSaverRunner::_Run() // re-evaluate the tick time after each successful wakeup // screensavers can adjust it on the fly, and we must be // prepared to accomodate that - tick = fSaver ? fSaver->TickSize() : tickBase; + tick = fSaver != NULL ? fSaver->TickSize() : tickBase; if (tick < tickBase) { if (tick < 0) @@ -229,22 +212,22 @@ ScreenSaverRunner::_Run() if (snoozeCount) { // if we are sleeping, do nothing snoozeCount--; - } else if (fSaver != NULL && fHasStarted) { + } else if (fSaver != NULL) { if (fSaver->LoopOnCount() && frame >= fSaver->LoopOnCount()) { // Time to nap frame = 0; snoozeCount = fSaver->LoopOffCount(); - } else if (fWindow->LockWithTimeout(5000LL) == B_OK) { + } else if (fView->Window()->LockWithTimeout(5000LL) == B_OK) { if (!fQuitting) { - // NOTE: R5 really calls DirectDraw() + // NOTE: BeOS R5 really calls DirectDraw() // and then Draw() for the same frame - if (fDirectWindow) + if (fIsDirectDraw) fSaver->DirectDraw(frame); fSaver->Draw(fView, frame); fView->Sync(); frame++; } - fWindow->Unlock(); + fView->Window()->Unlock(); } } else snoozeCount = 1000; diff --git a/src/preferences/screensaver/ScreenSaverWindow.cpp b/src/preferences/screensaver/ScreenSaverWindow.cpp index 41efa8a4ba..1762907d2e 100644 --- a/src/preferences/screensaver/ScreenSaverWindow.cpp +++ b/src/preferences/screensaver/ScreenSaverWindow.cpp @@ -19,11 +19,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -31,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -807,8 +810,7 @@ ModulesView::_OpenSaver() BView* view = fPreviewView->AddPreview(); fCurrentName = fSettings.ModuleName(); - fSaverRunner = new ScreenSaverRunner(Window(), view, true, fSettings); - BScreenSaver* saver = _ScreenSaver(); + fSaverRunner = new ScreenSaverRunner(view, fSettings); #ifdef __HAIKU__ BRect rect = fSettingsBox->InnerFrame().InsetByCopy(4, 4); @@ -821,8 +823,12 @@ ModulesView::_OpenSaver() fSettingsView->SetViewColor(ui_color(B_PANEL_BACKGROUND_COLOR)); fSettingsBox->AddChild(fSettingsView); - if (saver != NULL && fSaverRunner->Run() == B_OK) + BScreenSaver* saver = _ScreenSaver(); + if (saver != NULL && fSettingsView != NULL) { saver->StartConfig(fSettingsView); + if (saver->StartSaver(view, false) == B_OK) + fSaverRunner->Run(); + } if (fSettingsView->ChildAt(0) == NULL) { // There are no settings at all, we add the module name here to diff --git a/src/preferences/screensaver/ScreenSaverWindow.h b/src/preferences/screensaver/ScreenSaverWindow.h index 1206bf3a31..c3d70d68b9 100644 --- a/src/preferences/screensaver/ScreenSaverWindow.h +++ b/src/preferences/screensaver/ScreenSaverWindow.h @@ -12,18 +12,14 @@ #define SCREEN_SAVER_WINDOW_H +#include + #include "PasswordWindow.h" - -#include -#include -#include -#include -#include - #include "ScreenSaverSettings.h" -class BButton; +class BMessage; +class BRect; class BTabView; class FadeView;