Bug 146554 – Ensure media playback is stopped during page close

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

Depending on how a user closes a WebKit page, the media elements are not always stopped immediately. This can lead to audio playback continuing for a few seconds after the tab is closed.

The Page::close method should instruct the media elements to stop playback.

[Brent Fulgham]

<rdar://problem/18033944>

[Brent Fulgham]

Created attachment 256035 [details]
Patch

[Brent Fulgham]

Eric Carlson reviewed the media aspects of this in person. We just need a WK2 review now.

[Brent Fulgham]

WK2 suggested just doing everything in the WebCore::Page destructor instead.

[Brent Fulgham]

Created attachment 256039 [details]
Patch

[Brent Fulgham]

Created attachment 256040 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 256040 [details]
Patch

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

> Source/WebCore/page/Page.cpp:252
> +    PlatformMediaSessionManager::sharedManager().stopAllMediaPlayback();

This isn't right. You can have > 1 Page in a process.

[Brent Fulgham]

Created attachment 256052 [details]
Patch

[Brent Fulgham]

Created attachment 256054 [details]
Patch

[Brent Fulgham]

Committed r186251: <http://trac.webkit.org/changeset/186251>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 146573

[Brent Fulgham]

Created attachment 256073 [details]
Patch

[Brent Fulgham]

Testing revealed a crash when attempting to suspend certain WebAudio session. This was because the DestinationAudioNodes logic returns a nullptr for its current execution context when the node is being stopped. This pointer is not null-checked before being used.

In the test case, although the WebAudio session is still marked as Playing, the DestinationAudioNode is marked as 'stopping'. When this happens, the (null) pointer is dereferenced resulting in a crash.

[Brent Fulgham]

Created attachment 256076 [details]
Patch (correct test failure)

[Brent Fulgham]

Created attachment 256077 [details]
Patch

[Brent Fulgham]

I think the latest patch actually creates a progression in 'webaudio/mediastreamaudiodestinationnode.html'.

[Brent Fulgham]

Created attachment 256116 [details]
Patch

[Brent Fulgham]

Revised patch again. Media cleanup must happen at Page destruction time. If we do it when the Document is destroyed, the document has already been cleared out of we lose the ability to use that as the key to find the relevant media sessions.

[Daniel Bates]

Comment on attachment 256116 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by Zalan Bujtas.

Please take care to update this line with the name of the actual reviewer before you land this change.

> Source/WebCore/ChangeLog:13
> +        (WebCore::WebAudio::hostingDocument): Added.

Is "hosting document" existing jargon in the area of media sessions? If not then I suggest we name this function to according to the purpose of its return value, which is to be a key to identify a media session. Maybe mediaSessionKey?

> Source/WebCore/Modules/webaudio/AudioContext.h:93
> +    Document* hostingDocument() const override { return document(); }

Although not written in the WebKit Code Style Guidelines, I thought we were moving towards making methods be non-const when returning a non-const pointer from the email thread that started from <https://lists.webkit.org/pipermail/webkit-dev/2011-May/016872.html>. This can addressed in a follow up patch if needed.

As aforementioned, I suggest we come up with a more descriptive name for this function (or at least add a comment) so as to better describe the purpose of this function from document(). I am assuming we only want to use hostingDocument() as a way to identify a media session and use document() for all other purposes. That is, I assume we do not want to advertise to people to use hostingDocument() as a means to circumvent the assertion check in document().

> Source/WebCore/html/HTMLMediaElement.h:151
> +    Document* hostingDocument() const override { return &document(); }

Ditto.

> Source/WebCore/page/Page.cpp:1772
> +    for (auto* frame = static_cast<Frame*>(&mainFrame()); frame; frame = frame->tree().traverseNext())

I don't see the value of using auto here given that we need to perform a static_cast<Frame*> to hint to the compiler that auto refer to data type Frame* instead of MainFrame*. I suggest we substitute "Frame*" for "auto*". Then we can remove the static_cast<Frame*>().

> Source/WebCore/platform/audio/PlatformMediaSession.h:183
> +    virtual Document* hostingDocument() const = 0;

See my remark for AudioContext::hostingDocument().

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:335
> +    for (auto session : m_sessions) {

For your consideration, I suggest we use "auto*" instead of "auto" so as to make it explicit to a reader that it's acceptable to perform a copy of each element in m_sessions (since m_sessions is a collection of pointers) among other benefits.

[Daniel Bates]

Comment on attachment 256116 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioContext.h:93
>> +    Document* hostingDocument() const override { return document(); }
> 
> Although not written in the WebKit Code Style Guidelines, I thought we were moving towards making methods be non-const when returning a non-const pointer from the email thread that started from <https://lists.webkit.org/pipermail/webkit-dev/2011-May/016872.html>. This can addressed in a follow up patch if needed.
> 
> As aforementioned, I suggest we come up with a more descriptive name for this function (or at least add a comment) so as to better describe the purpose of this function from document(). I am assuming we only want to use hostingDocument() as a way to identify a media session and use document() for all other purposes. That is, I assume we do not want to advertise to people to use hostingDocument() as a means to circumvent the assertion check in document().

Can we return a const pointer?

[Daniel Bates]

(In reply to comment #20)
> Can we return a const pointer?

*a pointer to a const Document.

[Brent Fulgham]

Comment on attachment 256116 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        (WebCore::WebAudio::hostingDocument): Added.
> 
> Is "hosting document" existing jargon in the area of media sessions? If not then I suggest we name this function to according to the purpose of its return value, which is to be a key to identify a media session. Maybe mediaSessionKey?

I guess it is a key, in the sense that we want to find all media sessions that match a particular document. But the MediaSessions are not stored uniquely by this "key"; rather -- some set of MediaSessions are part of a document. In that sense, I think "hostingDocument" seems reasonable, though I'm far from an expert in good API design or naming schemes.

>>> Source/WebCore/Modules/webaudio/AudioContext.h:93
>>> +    Document* hostingDocument() const override { return document(); }
>> 
>> Although not written in the WebKit Code Style Guidelines, I thought we were moving towards making methods be non-const when returning a non-const pointer from the email thread that started from <https://lists.webkit.org/pipermail/webkit-dev/2011-May/016872.html>. This can addressed in a follow up patch if needed.
>> 
>> As aforementioned, I suggest we come up with a more descriptive name for this function (or at least add a comment) so as to better describe the purpose of this function from document(). I am assuming we only want to use hostingDocument() as a way to identify a media session and use document() for all other purposes. That is, I assume we do not want to advertise to people to use hostingDocument() as a means to circumvent the assertion check in document().
> 
> Can we return a const pointer?

Yes -- this can return a const pointer.

I'm less enamored of changing the name of "hostingDocument" to "...Key" or similar; conceptually, we want to only pause playback on those items that are part of a given Document. I needed to distinguish from the default "document()" because HTMLMediaElements return Document&, and this function returns Document*. The Document* member of AudioContext can be nullptr in some cases, so it's not safe to return as reference.

Maybe it could be a private method, and let PlatformMediaSession be a friend.

>> Source/WebCore/page/Page.cpp:1772
>> +    for (auto* frame = static_cast<Frame*>(&mainFrame()); frame; frame = frame->tree().traverseNext())
> 
> I don't see the value of using auto here given that we need to perform a static_cast<Frame*> to hint to the compiler that auto refer to data type Frame* instead of MainFrame*. I suggest we substitute "Frame*" for "auto*". Then we can remove the static_cast<Frame*>().

Seems reasonable. I'll switch to Frame*.

>> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:335
>> +    for (auto session : m_sessions) {
> 
> For your consideration, I suggest we use "auto*" instead of "auto" so as to make it explicit to a reader that it's acceptable to perform a copy of each element in m_sessions (since m_sessions is a collection of pointers) among other benefits.

Ok -- seems like a reasonable change.

[Brent Fulgham]

Created attachment 256166 [details]
Patch

[Brent Fulgham]

Created attachment 256169 [details]
Patch

[Sam Weinig]

Comment on attachment 256169 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1773
> +        PlatformMediaSessionManager::sharedManager().stopAllMediaPlaybackForDocument(*frame->document());

What guarantees all the documents are non-null?

[Brent Fulgham]

Comment on attachment 256169 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:1773
>> +        PlatformMediaSessionManager::sharedManager().stopAllMediaPlaybackForDocument(*frame->document());
> 
> What guarantees all the documents are non-null?

Hmmm. Good point.

[Brent Fulgham]

Created attachment 256184 [details]
Patch

[Brent Fulgham]

Debugging (and logging) shows that the WebProcess can be disconnected from the UIProcess *before* the WebPage::Close message is sent. When this happens, the WebProcess chugs merrily along, playing video content until the WebProcess realizes it's orphaned and shuts down.

I added logic to the "didClose" handler in WebProcess to deal with the disconnected case, so that we pause media when this happens. This addresses the remaining testing failures I encountered.

[Eric Carlson]

Comment on attachment 256184 [details]
Patch

This looks good to me, thanks Brent!

[Sam Weinig]

Comment on attachment 256184 [details]
Patch

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

> Source/WebKit2/WebProcess/WebProcess.cpp:662
> +    PlatformMediaSessionManager::sharedManager().stopAllMediaPlaybackForProcess();

As I mentioned in person, this seems wrong.  The line below should be shutting down the process.  If that is not happening, that is the bug.

[Brent Fulgham]

Created attachment 256248 [details]
Patch

[Brady Eidson]

Comment on attachment 256248 [details]
Patch

I looked at this in person with Brent a bit. The issue with the process not shutting down as quickly as we'd expect is bizarre and can't be figured out right now.

As suggested in person earlier, he filed a bug and referenced it in a FIXME near the necessary shutdown code.

An unfortunate r+ from me

[Brent Fulgham]

Committed r186385: <http://trac.webkit.org/changeset/186385>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 146668

[Brent Fulgham]

This was rolled out, in r186394: <http://trac.webkit.org/changeset/186394>.

Three tests were failing because part of an earlier patch was improperly lost when doing the final checking. I'll correct and re-land.

[Brent Fulgham]

Committed r186396: <http://trac.webkit.org/changeset/186396>