Bug 146554

Summary: Ensure media playback is stopped during page close
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, bfulgham, commit-queue, dbates, eric.carlson, jer.noble, sam, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 146573, 146668, 147116    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch (correct test failure)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch beidson: review+

Description Brent Fulgham 2015-07-02 12:00:20 PDT
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.
Comment 1 Brent Fulgham 2015-07-02 12:00:55 PDT
<rdar://problem/18033944>
Comment 2 Brent Fulgham 2015-07-02 14:05:53 PDT
Created attachment 256035 [details]
Patch
Comment 3 Brent Fulgham 2015-07-02 14:53:39 PDT
Eric Carlson reviewed the media aspects of this in person. We just need a WK2 review now.
Comment 4 Brent Fulgham 2015-07-02 15:07:46 PDT
WK2 suggested just doing everything in the WebCore::Page destructor instead.
Comment 5 Brent Fulgham 2015-07-02 15:10:07 PDT
Created attachment 256039 [details]
Patch
Comment 6 Brent Fulgham 2015-07-02 15:10:50 PDT
Created attachment 256040 [details]
Patch
Comment 7 Simon Fraser (smfr) 2015-07-02 15:29:09 PDT
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.
Comment 8 Brent Fulgham 2015-07-02 17:10:40 PDT
Created attachment 256052 [details]
Patch
Comment 9 Brent Fulgham 2015-07-02 17:38:19 PDT
Created attachment 256054 [details]
Patch
Comment 10 Brent Fulgham 2015-07-02 18:45:37 PDT
Committed r186251: <http://trac.webkit.org/changeset/186251>
Comment 11 WebKit Commit Bot 2015-07-02 19:39:43 PDT
Re-opened since this is blocked by bug 146573
Comment 12 Brent Fulgham 2015-07-02 22:42:57 PDT
Created attachment 256073 [details]
Patch
Comment 13 Brent Fulgham 2015-07-02 22:52:34 PDT
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.
Comment 14 Brent Fulgham 2015-07-02 22:53:36 PDT
Created attachment 256076 [details]
Patch (correct test failure)
Comment 15 Brent Fulgham 2015-07-02 23:05:56 PDT
Created attachment 256077 [details]
Patch
Comment 16 Brent Fulgham 2015-07-02 23:08:01 PDT
I think the latest patch actually creates a progression in 'webaudio/mediastreamaudiodestinationnode.html'.
Comment 17 Brent Fulgham 2015-07-03 12:45:31 PDT
Created attachment 256116 [details]
Patch
Comment 18 Brent Fulgham 2015-07-03 12:46:25 PDT
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.
Comment 19 Daniel Bates 2015-07-03 12:57:29 PDT
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.
Comment 20 Daniel Bates 2015-07-03 13:00:19 PDT
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?
Comment 21 Daniel Bates 2015-07-03 13:01:22 PDT
(In reply to comment #20)
> Can we return a const pointer?

*a pointer to a const Document.
Comment 22 Brent Fulgham 2015-07-03 17:53:34 PDT
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.
Comment 23 Brent Fulgham 2015-07-04 19:21:36 PDT
Created attachment 256166 [details]
Patch
Comment 24 Brent Fulgham 2015-07-04 21:04:37 PDT
Created attachment 256169 [details]
Patch
Comment 25 Sam Weinig 2015-07-05 10:27:06 PDT
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?
Comment 26 Brent Fulgham 2015-07-05 11:24:55 PDT
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.
Comment 27 Brent Fulgham 2015-07-05 13:56:48 PDT
Created attachment 256184 [details]
Patch
Comment 28 Brent Fulgham 2015-07-05 14:01:22 PDT
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.
Comment 29 Eric Carlson 2015-07-06 10:17:05 PDT
Comment on attachment 256184 [details]
Patch

This looks good to me, thanks Brent!
Comment 30 Sam Weinig 2015-07-06 11:52:45 PDT
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.
Comment 31 Brent Fulgham 2015-07-06 15:57:41 PDT
Created attachment 256248 [details]
Patch
Comment 32 Brady Eidson 2015-07-06 16:02:01 PDT
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
Comment 33 Brent Fulgham 2015-07-06 16:42:25 PDT
Committed r186385: <http://trac.webkit.org/changeset/186385>
Comment 34 WebKit Commit Bot 2015-07-06 18:38:49 PDT
Re-opened since this is blocked by bug 146668
Comment 35 Brent Fulgham 2015-07-06 19:54:41 PDT
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.
Comment 36 Brent Fulgham 2015-07-06 20:41:21 PDT
Committed r186396: <http://trac.webkit.org/changeset/186396>