Bug 186971 – [GStreamer] Media player does not properly inhibit, uninhibit sleep

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Bastien Nocera]

webkit2gtk3-2.20.2-1.fc28.x86_64

1. Open a page like https://www.twitch.tv/videos/276566160
2. Press play, idle is inhibited
3. Press pause, idle is still inhibited
4. Open a new tab, idle stop being inhibited

You can check the inhibited actions with:
gdbus introspect --session --dest org.gnome.SessionManager --object-path /org/gnome/SessionManager | grep InhibitedActions

After 3, idle should be uninhibited.

[Michael Catanzaro]

I see at the very top of HTMLMediaElement::shouldDisableSleep:

    if (!m_player || m_player->paused() || loop())
        return SleepType::None;

which should turn off the SleepDisabler. So some debugging will be required to investigate why this code is not working as intended.

[Philippe Normand]

The problem is two-fold:

- the visibility of the media element changed but HTMLMediaElement::shouldDisableSleep() returns too early the None result, before checking the m_elementIsHidden variable.

- Even if the m_elementIsHidden test is moved up, a System sleep disabler will be created, but our SleepDisablerGlib doesn't support that type.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #2)
> - Even if the m_elementIsHidden test is moved up, a System sleep disabler
> will be created, but our SleepDisablerGlib doesn't support that type.

Umm...

I see I wrote this comment when writing SleepDisablerGlib:

    // We don't support suspend ("System") inhibitors, only idle inhibitors.
    // To get suspend inhibitors, we'd need to use the fancy GNOME
    // SessionManager API, which requires registering as a client application,
    // which is not practical from the web process. Secondly, because the only
    // current use of a suspend inhibitor in WebKit,
    // HTMLMediaElement::shouldDisableSleep, is suspicious. There's really no
    // valid reason for WebKit to ever block suspend, only idle.

There are three things wrong with this comment:

 * Nowadays, GNOME suspends the computer automatically on inactivity, and we actually do want to prevent that.
 * Inhibiting idle does affect the autosuspend countdown. So the current code should prevent automatic suspend, just not manual suspend. (I think I stand by my comment that we do not want to inhibit normal suspend, since that would just be annoying.)
 * I don't know why I thought HTMLMediaElement::shouldDisableSleep was suspicious.

Anyway, I guess the behavior we want is to ignore the Type argument and just always inhibit idle. That is, treat type=System the same way we current treat type=Display. I couldn't decide which to do when I wrote this code, but in retrospect that would have been a better option.

[Philippe Normand]

(In reply to Philippe Normand from comment #2)
> The problem is two-fold:
> 
> - the visibility of the media element changed but
> HTMLMediaElement::shouldDisableSleep() returns too early the None result,
> before checking the m_elementIsHidden variable.
> 

Eric, would it be acceptable to move the the m_elementIsHidden test up in HTMLMediaElement::shouldDisableSleep() ?

[Eric Carlson]

(In reply to Philippe Normand from comment #4)
> (In reply to Philippe Normand from comment #2)
> > The problem is two-fold:
> > 
> > - the visibility of the media element changed but
> > HTMLMediaElement::shouldDisableSleep() returns too early the None result,
> > before checking the m_elementIsHidden variable.
> > 
> 
> Eric, would it be acceptable to move the the m_elementIsHidden test up in
> HTMLMediaElement::shouldDisableSleep() ?

We don't want to enable sleep for a "playing" that doesn't have audio and video, so where would we move the m_elementIsHidden test?

[Philippe Normand]

I started debugging this again.

In shouldDisableSleep(), both hasVideo() and hasAudio() calls return false because a null MediaPlayerPrivate has been created meanwhile. I don't know why yet... So this shouldn't happen, the video is paused and should have both audio and video... So shouldBeAbleToSleep becomes true there, instead of false.

[Philippe Normand]

I meant, So shouldBeAbleToSleep becomes false there, instead of true. :)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> There are three things wrong with this comment:

Split all of this out to bug #194500. Leaving this bug for the HTMLMediaElement problems.

[Michael Catanzaro]

Well this bug as reported no longer occurs, because nowadays we never disable sleep. In bug #192530, I found HTMLMediaElement::updateSleepDisabling is not being called when paused/unpaused.

[Michael Catanzaro]

This continues to be pretty annoying.

Conclusion from bug #194500 is that SleepDisabler will inhibit idle now, as expected. So that's solved.

Conclusion from bug #192530: "Now, HTMLMediaElement::updateSleepDisabling is not being called when paused/unpaused, which is bad, but I believe we have other bug reports for that." This should be solved here.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #6)
> I started debugging this again.
> 
> In shouldDisableSleep(), both hasVideo() and hasAudio() calls return false
> because a null MediaPlayerPrivate has been created meanwhile. I don't know
> why yet... So this shouldn't happen, the video is paused and should have
> both audio and video... So shouldBeAbleToSleep becomes true there, instead
> of false.

I wasted a bunch of time debugging this today because I didn't read your comment. Conclusion: this is exactly what's still happening. When starting a YouTube video, m_mediaPlayer is either valid and paused() (during a brief window before playback begins), or else it's NULL (it seems to always be NULL during media playback). So next question to debug is: why is media playing when MediaPlayer is NULL?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> So next question to debug is: why is media playing when MediaPlayer is NULL?

[Xabier Rodríguez Calvar]

(In reply to Michael Catanzaro from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > So next question to debug is: why is media playing when MediaPlayer is NULL?

The media player is null in some other HTMLMediaElement, not in the one taking care of the main playback.

[Xabier Rodríguez Calvar]

Created attachment 413683 [details]
Patch

We were not updating the playback state in the MSE player and therefore, when getting to PLAYING the HTMLMediaElement didn't know.

[Michael Catanzaro]

I tested this today. I see an inhibitor is now acquired when I begin playing a YouTube video, which is an improvement. However, the inhibitor is not released when I stop the video. Debug patch:

diff --git a/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp b/Source/WebCore/PAL/pal/system/glib/SleepDisabl
erGLib.cpp
index 6f922e118afd..2fc2d15487a4 100644
--- a/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp
+++ b/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp
@@ -104,6 +104,7 @@ SleepDisablerGLib::~SleepDisablerGLib()
 
 void SleepDisablerGLib::acquireInhibitor()
 {
+WTFLogAlways("%s this=%p", __FUNCTION__, this);
     if (m_screenSaverProxy) {
         ASSERT(!m_inhibitPortalProxy);
         acquireInhibitorViaScreenSaverProxy();
@@ -158,6 +159,7 @@ void SleepDisablerGLib::acquireInhibitorViaInhibitPortalProxy()
 
 void SleepDisablerGLib::releaseInhibitor()
 {
+WTFLogAlways("%s this=%p", __FUNCTION__, this);
     if (m_screenSaverProxy) {
         ASSERT(!m_inhibitPortalProxy);
         releaseInhibitorViaScreenSaverProxy();

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #15)
> I tested this today. I see an inhibitor is now acquired when I begin playing
> a YouTube video, which is an improvement. However, the inhibitor is not
> released when I stop the video.

The inhibitor is also not released when I close the browser while a video is playing. I guess the SleepDisabler is leaked, which I presume will cause the inhibit to last forever? Not sure.

I've also noticed the inhibitor is sometimes not acquired when starting a video, but I haven't figured out how to reproduce that.

Finally, Calvaris noticed that SleepDisabler doesn't actually work. There are two problems: first, the ScreenSaver D-Bus API is not available inside the sandbox. (OK.) Second, the portal API fails because we try using g_get_prgname() instead of a valid app ID. These are problems in SleepDisabler itself, which I will address in a forthcoming bug report. Let's use this bug only for the original problem in HTMLMediaElement that Calvaris has investigated.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #16)
> I've also noticed the inhibitor is sometimes not acquired when starting a
> video, but I haven't figured out how to reproduce that.

This often -- but not always -- happens when starting a video by clicking the "Skip Ads" button.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> This often -- but not always -- happens when starting a video by clicking
> the "Skip Ads" button.

Hm, actually it seems like it always happens. So summary of remaining media-level problems:

 * SleepDisabler not created when "Skip Ads" is used to start the video
 * SleepDisabler not destroyed when stopping the video
 * SleepDisabler leaked when closing tab (though it is destroyed when refreshing the tab!)

(In reply to Michael Catanzaro from comment #16)
> Finally, Calvaris noticed that SleepDisabler doesn't actually work. There
> are two problems: first, the ScreenSaver D-Bus API is not available inside
> the sandbox. (OK.) Second, the portal API fails because we try using
> g_get_prgname() instead of a valid app ID.

I'm continuing to investigate these.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #16)
> Second, the portal API fails because we try using
> g_get_prgname() instead of a valid app ID.

That's not what's happening. We only pass our own prgname to the ScreenSaver D-Bus API, which is blocked in the sandbox because it's only for use by trusted applications. We don't pass our own app ID to the portal API, because we are sandboxed and not trusted to not lie about it. We can continue this thread in bug #219010.

[Bastien Nocera]

(In reply to Michael Catanzaro from comment #19)
> (In reply to Michael Catanzaro from comment #16)
> > Second, the portal API fails because we try using
> > g_get_prgname() instead of a valid app ID.
> 
> That's not what's happening. We only pass our own prgname to the ScreenSaver
> D-Bus API, which is blocked in the sandbox because it's only for use by
> trusted applications. We don't pass our own app ID to the portal API,
> because we are sandboxed and not trusted to not lie about it. We can
> continue this thread in bug #219010.

The org.freedesktop.ScreenSaver API is implemented in gnome-settings-daemon, and doesn't filter clients:
https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/screensaver-proxy/gsd-screensaver-proxy-manager.c
https://github.com/WebKit/webkit/blob/master/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp#L54

[Xabier Rodríguez Calvar]

The playback part is done. I think this can/should land regardless the problem with the flatpak. If there's something else I can help with, I guess another bug can be filed.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #18)
> (In reply to Michael Catanzaro from comment #17)
> > This often -- but not always -- happens when starting a video by clicking
> > the "Skip Ads" button.
> 
> Hm, actually it seems like it always happens. So summary of remaining
> media-level problems:
> 
>  * SleepDisabler not created when "Skip Ads" is used to start the video
>  * SleepDisabler not destroyed when stopping the video
>  * SleepDisabler leaked when closing tab (though it is destroyed when
> refreshing the tab!)

These problems are not bugs in SleepDisabler itself. At least the first two seem very likely to be media playback bugs, yes? I suppose the third one could be caused by something else leaking the entire MediaPlayer?

[Michael Catanzaro]

Comment on attachment 413683 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:588
> +        // Emit play state change notification only when going to PLAYING so that
> +        // the media element gets a chance to enable its page sleep disabler.
> +        // Emitting this notification in more cases triggers unwanted code paths
> +        // and test timeouts.

But is this really correct? What is the plan to ensure the SleepDisabler gets released when playback stops?

[Xabier Rodríguez Calvar]

Comment on attachment 413683 [details]
Patch

(In reply to Michael Catanzaro from comment #23)
> Comment on attachment 413683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413683&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:588
> > +        // Emit play state change notification only when going to PLAYING so that
> > +        // the media element gets a chance to enable its page sleep disabler.
> > +        // Emitting this notification in more cases triggers unwanted code paths
> > +        // and test timeouts.
> 
> But is this really correct? What is the plan to ensure the SleepDisabler
> gets released when playback stops?

Once the sandboxing issues are fixed, you can test it and file a new bug if there is such a problem.

[EWS]

Committed r270235: <https://trac.webkit.org/changeset/270235>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413683 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71798034>

[Michael Catanzaro]

>  * SleepDisabler not created when "Skip Ads" is used to start the video
>  * SleepDisabler not destroyed when stopping the video
>  * SleepDisabler leaked when closing tab (though it is destroyed when
> refreshing the tab!)

Issuse with SleepDisabler creation/destruction are not related to the sandboxing issues, so I'll report new bugs now.

[Michael Catanzaro]

Reported: bug #219353, bug #219354, bug #219355.