Skip to content

Commit

Permalink
Don't drop the GPUProcess media assertion too promptly after playback…
Browse files Browse the repository at this point in the history
… ends

https://bugs.webkit.org/show_bug.cgi?id=259219
rdar://112003621

Reviewed by Jer Noble and Eric Carlson.

Don't drop the GPUProcess media assertion too promptly after playback ends,
instead, drop it on a timer.

There is a race where both the GPUProcess and the UIProcess get notified of a
WebProcess exiting in parallel. When notified, the UIProcess would drop the
media assertion on the GPUProcess, which could get it to suspend while the
GPUProcess is STILL shutting down the media stack.

Dropping the media assertion on a timer makes it so that the GPUProcess is now
extremely unlikely to lose the race and will have properly shut down the media
stack by the time it gets suspended.

* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::clearAudibleActivity):
(WebKit::WebProcessPool::updateAudibleMediaAssertions):
* Source/WebKit/UIProcess/WebProcessPool.h:

Canonical link: https://commits.webkit.org/266069@main
  • Loading branch information
cdumez committed Jul 14, 2023
1 parent 99ff083 commit e89c9b4
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
15 changes: 14 additions & 1 deletion Source/WebKit/UIProcess/WebProcessPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ constexpr unsigned maximumGPUProcessRelaunchAttemptsBeforeKillingWebProcesses {
bool WebProcessPool::s_shouldCrashWhenCreatingWebProcess = false;
#endif

static constexpr Seconds audibleActivityClearDelay = 5_s;

bool WebProcessPool::s_didGlobalStaticInitialization = false;

Ref<WebProcessPool> WebProcessPool::create(API::ProcessPoolConfiguration& configuration)
Expand Down Expand Up @@ -233,6 +235,7 @@ WebProcessPool::WebProcessPool(API::ProcessPoolConfiguration& configuration)
, m_backForwardCache(makeUniqueRef<WebBackForwardCache>(*this))
, m_webProcessCache(makeUniqueRef<WebProcessCache>(*this))
, m_webProcessWithAudibleMediaCounter([this](RefCounterEvent) { updateAudibleMediaAssertions(); })
, m_audibleActivityTimer(RunLoop::main(), this, &WebProcessPool::clearAudibleActivity)
, m_webProcessWithMediaStreamingCounter([this](RefCounterEvent) { updateMediaStreamingActivity(); })
{
if (!s_didGlobalStaticInitialization) {
Expand Down Expand Up @@ -2156,14 +2159,24 @@ WebProcessWithAudibleMediaToken WebProcessPool::webProcessWithAudibleMediaToken(
return m_webProcessWithAudibleMediaCounter.count();
}

void WebProcessPool::clearAudibleActivity()
{
ASSERT(!m_webProcessWithAudibleMediaCounter.value());
m_audibleMediaActivity = std::nullopt;
}

void WebProcessPool::updateAudibleMediaAssertions()
{
if (!m_webProcessWithAudibleMediaCounter.value()) {
WEBPROCESSPOOL_RELEASE_LOG(ProcessSuspension, "updateAudibleMediaAssertions: The number of processes playing audible media now zero. Releasing UI process assertion.");
m_audibleMediaActivity = std::nullopt;
// We clear the audible activity on a timer for 2 reasons:
// 1. Media may start playing shortly after (e.g. switching from one track to another)
// 2. It minimizes the risk of the GPUProcess getting suspended while shutting down the media stack.
m_audibleActivityTimer.startOneShot(audibleActivityClearDelay);
return;
}

m_audibleActivityTimer.stop();
if (m_audibleMediaActivity)
return;

Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/UIProcess/WebProcessPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ class WebProcessPool final
#if HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK)
void setMediaAccessibilityPreferences(WebProcessProxy&);
#endif
void clearAudibleActivity();

Ref<API::ProcessPoolConfiguration> m_configuration;

Expand Down Expand Up @@ -803,6 +804,7 @@ class WebProcessPool final
#endif
};
std::optional<AudibleMediaActivity> m_audibleMediaActivity;
RunLoop::Timer m_audibleActivityTimer;

WebProcessWithMediaStreamingCounter m_webProcessWithMediaStreamingCounter;
bool m_mediaStreamingActivity { false };
Expand Down

0 comments on commit e89c9b4

Please sign in to comment.