Bug 199471

Summary: HTMLMediaElement can hold onto display sleep assertion while process is suspended.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric.carlson, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Jer Noble 2019-07-03 15:35:10 PDT
HTMLMediaElement can hold onto display sleep assertion while process is suspended.
Comment 1 Jer Noble 2019-07-03 15:35:39 PDT
<rdar://problem/52124320>
Comment 2 Jer Noble 2019-07-03 15:39:24 PDT
Created attachment 373424 [details]
Patch
Comment 3 Darin Adler 2019-07-03 15:48:51 PDT
Comment on attachment 373424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373424&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:6927
> +    if (PlatformMediaSessionManager::sharedManager().processIsSuspended())
> +        return SleepType::None;

Is this correct even for audio? You can’t hear audio when the process is suspended?
Comment 4 Jer Noble 2019-07-03 15:51:07 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 373424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373424&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6927
> > +    if (PlatformMediaSessionManager::sharedManager().processIsSuspended())
> > +        return SleepType::None;
> 
> Is this correct even for audio? You can’t hear audio when the process is
> suspended?

The danger is that, if the process is suspended, it will never get a chance to release it's process assertion, and so even if audio playback ends in mediaserverd (like when it reaches the end of the file), we'll keep the system from sleeping.
Comment 5 Darin Adler 2019-07-03 15:52:53 PDT
(In reply to Jer Noble from comment #4)
> The danger is that, if the process is suspended, it will never get a chance
> to release it's process assertion, and so even if audio playback ends in
> mediaserverd (like when it reaches the end of the file), we'll keep the
> system from sleeping.

OK, so we’re losing the feature where you can listen to a whole podcast without the system sleeping? And need to find some other way to implement that?
Comment 6 Jer Noble 2019-07-03 16:02:38 PDT
No. (In reply to Darin Adler from comment #5)
> (In reply to Jer Noble from comment #4)
> > The danger is that, if the process is suspended, it will never get a chance
> > to release it's process assertion, and so even if audio playback ends in
> > mediaserverd (like when it reaches the end of the file), we'll keep the
> > system from sleeping.
> 
> OK, so we’re losing the feature where you can listen to a whole podcast
> without the system sleeping? And need to find some other way to implement
> that?

No, for two reasons: 1) on iOS, mediaserverd will keep the system from sleeping while audio is being generated and 2) WebKit in the UIProcess will take out a process assertion on the WebContent when it is generating audio. So 1) makes having our own system sleep assertion a “belt and suspenders” thing, and 2) should ensure the WebCintent process is never suspended in the first place during the “podcast playback” scenario. 

But because the WebContent process could get suspended the next turn of the run loop after the processWillSuspend() call turns, we may not ever receive the mediaPlayerRateDidChange() callback before suspending. This patch just protects against the situation where a suspend breaks our ability to release our system sleep assertion.
Comment 7 WebKit Commit Bot 2019-07-03 17:08:08 PDT
Comment on attachment 373424 [details]
Patch

Clearing flags on attachment: 373424

Committed r247118: <https://trac.webkit.org/changeset/247118>
Comment 8 WebKit Commit Bot 2019-07-03 17:08:09 PDT
All reviewed patches have been landed.  Closing bug.