Multiple calls to suspend media playback for the same page may result in resuming media playback too soon

[Kate Cheney]

It’s possible that multiple calls to _suspendAllMediaPlayback() could occur in the same WKWebView with different expectations of when to resume. The current implementation will resume after one call to _resumeAllMediaPlayback(). Instead, we should make sure that all calls to _suspendAllMediaPlayback() have been resolved with _resumeAllMediaPlayback() before actually resuming media content. Adding a counter to track how many calls to suspend have been made and only resuming when the counter is at 0 is a potential solution.

[Kate Cheney]

rdar://problem/69709346

[Kate Cheney]

Created attachment 410330 [details]
Patch

[Kate Cheney]

Created attachment 410366 [details]
Patch

[Eric Carlson]

Comment on attachment 410366 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1897
> +    return (NSInteger *)(static_cast<long>(_page->suspendMediaPlaybackCounter()));

can you `return @(_page->suspendMediaPlaybackCounter())` ?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:345
> +- (NSInteger *)_suspendMediaPlaybackCounter WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This should probably be WKWebViewPrivateForTesting.h

> Source/WebKit/UIProcess/WebPageProxy.cpp:6045
> +    if (!m_mediaPlaybackIsSuspended || m_suspendMediaPlaybackCounter > 0)

if (!m_mediaPlaybackIsSuspended || m_suspendMediaPlaybackCounter)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StopSuspendResumeAllMedia.mm:115
> +        auto counter = [webView _suspendMediaPlaybackCounter];
> +        if (counter >= (NSInteger *)(static_cast<long>(2)))

if ([webView _suspendMediaPlaybackCounter] <= @(2))

> Tools/TestWebKitAPI/Tests/WebKitCocoa/StopSuspendResumeAllMedia.mm:123
> +        auto counter = [webView _suspendMediaPlaybackCounter];
> +        if (counter <= (NSInteger *)(static_cast<long>(1)))

if ([webView _suspendMediaPlaybackCounter] <= @(1))

[Kate Cheney]

(In reply to Eric Carlson from comment #4)
> Comment on attachment 410366 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410366&action=review
> 

Thanks for the review!

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1897
> > +    return (NSInteger *)(static_cast<long>(_page->suspendMediaPlaybackCounter()));
> 
> can you `return @(_page->suspendMediaPlaybackCounter())` ?
> 

Yep, will change.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:345
> > +- (NSInteger *)_suspendMediaPlaybackCounter WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This should probably be WKWebViewPrivateForTesting.h
> 

Cool, didn't know about this, I'll move it.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:6045
> > +    if (!m_mediaPlaybackIsSuspended || m_suspendMediaPlaybackCounter > 0)
> 
> if (!m_mediaPlaybackIsSuspended || m_suspendMediaPlaybackCounter)
> 

Will fix.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/StopSuspendResumeAllMedia.mm:115
> > +        auto counter = [webView _suspendMediaPlaybackCounter];
> > +        if (counter >= (NSInteger *)(static_cast<long>(2)))
> 
> if ([webView _suspendMediaPlaybackCounter] <= @(2))
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/StopSuspendResumeAllMedia.mm:123
> > +        auto counter = [webView _suspendMediaPlaybackCounter];
> > +        if (counter <= (NSInteger *)(static_cast<long>(1)))
> 
> if ([webView _suspendMediaPlaybackCounter] <= @(1))

I got 'error: direct comparison of a numeric literal has undefined behavior' with this change, but I can change it to if ([webView _suspendMediaPlaybackCounter].intValue <= 1) to avoid the casting.

[Kate Cheney]

Created attachment 410382 [details]
Patch for landing

[EWS]

Committed r267899: <https://trac.webkit.org/changeset/267899>

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

[Radar WebKit Bug Importer]

<rdar://problem/69901158>