Bug 197798 - Take out MediaPlayback UI assertion when any WebProcess is playing audible media
Summary: Take out MediaPlayback UI assertion when any WebProcess is playing audible media
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-10 14:05 PDT by Jer Noble
Modified: 2019-05-13 15:02 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2019-05-10 14:16 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.05 KB, patch)
2019-05-10 15:06 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2019-05-10 16:12 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (10.77 KB, patch)
2019-05-13 13:26 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (10.77 KB, patch)
2019-05-13 13:33 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (10.77 KB, patch)
2019-05-13 13:49 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2019-05-10 14:05:01 PDT
Take out MediaPlayback UI assertion when any WebProcess is playing audible media
Comment 1 Jer Noble 2019-05-10 14:16:34 PDT
Created attachment 369597 [details]
Patch
Comment 2 Build Bot 2019-05-10 14:18:47 PDT
Attachment 369597 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jer Noble 2019-05-10 15:06:44 PDT
Created attachment 369604 [details]
Patch
Comment 4 Build Bot 2019-05-10 15:09:13 PDT
Attachment 369604 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Chris Dumez 2019-05-10 15:45:33 PDT
Comment on attachment 369604 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.h:795
> +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;

What makes sure this gets cleared properly in the event of a WebContent process crash?

> Source/WebKit/UIProcess/WebProcessPool.h:796
> +    std::unique_ptr<ProcessAssertion> m_uiProcessMediaPlaybackAssertion;

ditto.

> Source/WebKit/WebProcess/WebProcess.h:109
> +class WebPageProxy;

Something is wrong here. I do not think you meant to modify this file at all. There is no reason to change WebContent process code here, only UIProcess code.
Comment 6 Chris Dumez 2019-05-10 15:48:27 PDT
Comment on attachment 369604 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:2578
> +    RELEASE_LOG(ProcessSuspension, "Web process pid %u is now playing audible media", (unsigned)process->processIdentifier());

Why not %d ?

> Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> +    auto result = m_processesWithUploads.add(processID, nullptr);

m_processesWithUploads.add LOL bad copy paste.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2601
> +    if (m_processesWithUploads.isEmpty()) {

Bad copy paste.
Comment 7 Chris Dumez 2019-05-10 15:51:41 PDT
Comment on attachment 369604 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.h:795
>> +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;
> 
> What makes sure this gets cleared properly in the event of a WebContent process crash?

I have similar concerns when a process shutdown cleanly because it no longer has any pages. Do we properly notice that we stop playing audio if the page gets closed?
Comment 8 Jer Noble 2019-05-10 15:51:50 PDT
(In reply to Chris Dumez from comment #6)
> Comment on attachment 369604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369604&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:2578
> > +    RELEASE_LOG(ProcessSuspension, "Web process pid %u is now playing audible media", (unsigned)process->processIdentifier());
> 
> Why not %d ?

Probably because process identifiers are unsigned, not signed integers.

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> > +    auto result = m_processesWithUploads.add(processID, nullptr);
> 
> m_processesWithUploads.add LOL bad copy paste.

Oh no!

> > Source/WebKit/UIProcess/WebProcessPool.cpp:2601
> > +    if (m_processesWithUploads.isEmpty()) {
> 
> Bad copy paste.


Ditto. Will fix.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 369604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369604&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.h:795
> > +    HashMap<WebCore::ProcessIdentifier, std::unique_ptr<ProcessAssertion>> m_processesPlayingAudibleMedia;
> 
> What makes sure this gets cleared properly in the event of a WebContent
> process crash?

Good catch, I need to add a line to resetStateAfterProcessExited().

> > Source/WebKit/UIProcess/WebProcessPool.h:796
> > +    std::unique_ptr<ProcessAssertion> m_uiProcessMediaPlaybackAssertion;
> 
> ditto.
> 
> > Source/WebKit/WebProcess/WebProcess.h:109
> > +class WebPageProxy;
> 
> Something is wrong here. I do not think you meant to modify this file at
> all. There is no reason to change WebContent process code here, only
> UIProcess code.

You're right.
Comment 9 Jer Noble 2019-05-10 16:12:20 PDT
Created attachment 369612 [details]
Patch
Comment 10 Build Bot 2019-05-10 16:14:52 PDT
Attachment 369612 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Chris Dumez 2019-05-10 16:34:15 PDT
Comment on attachment 369612 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebProcessPool.cpp:2587
> +    auto result = m_processesPlayingAudibleMedia.add(processID, nullptr);

Could you ASSERT(!m_processesPlayingAudibleMedia.contains(process.coreProcessIdentifier())) in WebProcessPool::disconnectProcess() ? Just to be safe :) Maybe even RELEASE_ASSERT() since so don't often run debug on iOS.
Comment 12 Jer Noble 2019-05-13 13:26:33 PDT
Created attachment 369774 [details]
Patch for landing
Comment 13 Build Bot 2019-05-13 13:27:33 PDT
Attachment 369774 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Jer Noble 2019-05-13 13:33:46 PDT
Created attachment 369776 [details]
Patch for landing
Comment 15 Build Bot 2019-05-13 13:35:32 PDT
Attachment 369776 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Jer Noble 2019-05-13 13:49:41 PDT
Created attachment 369777 [details]
Patch for landing
Comment 17 Build Bot 2019-05-13 13:51:41 PDT
Attachment 369777 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessProxy.cpp:1215:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 WebKit Commit Bot 2019-05-13 15:01:18 PDT
Comment on attachment 369777 [details]
Patch for landing

Clearing flags on attachment: 369777

Committed r245255: <https://trac.webkit.org/changeset/245255>
Comment 19 WebKit Commit Bot 2019-05-13 15:01:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2019-05-13 15:02:18 PDT
<rdar://problem/50740551>