Bug 212981

Summary: Catalyst WebKit apps continue to play audio after quitting
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ddkilzer, eric.carlson, ews-watchlist, jbedard, jer.noble, peng.liu6, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Jer Noble 2020-06-09 11:44:09 PDT
Catalyst WebKit apps continue to play audio after quitting
Comment 1 Jer Noble 2020-06-09 11:45:37 PDT
<rdar://problem/57089471>
Comment 2 Jer Noble 2020-06-09 11:56:43 PDT
Created attachment 401458 [details]
Patch
Comment 3 Jer Noble 2020-06-09 14:01:38 PDT
Created attachment 401472 [details]
Patch
Comment 4 Chris Dumez 2020-06-09 14:46:22 PDT
You are likely going to need to update WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/13/RunningBoardServices.framework/RunningBoardServices.tbd to fix the Open Source build.
Comment 5 Chris Dumez 2020-06-09 14:48:54 PDT
Comment on attachment 401472 [details]
Patch

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

> Source/WebKit/UIProcess/EndowmentStateTracker.h:61
> +    WeakHashSet<WebPageProxy> m_clients;

Would have been nice to use a Client interface for this instead of making this only usable by WebPageProxy.

> Source/WebKit/UIProcess/EndowmentStateTracker.h:62
> +    WTF::RetainPtr<RBSProcessMonitor> m_processMonitor;

WTF:: should not be needed.
Comment 6 Chris Dumez 2020-06-09 14:55:12 PDT
Comment on attachment 401472 [details]
Patch

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

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:38
> +static NSString* visibilityEndowment = @"com.apple.frontboard.visibility";

static const

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:39
> +static NSString* userfacingEndowment = @"com.apple.launchservices.userfacing";

ditto.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:76
> +bool EndowmentStateTracker::isApplicationForeground(pid_t pid)

I think I will be able to drop this one in a follow-up since ViewServices gets the visibility endowment and we don't need to check their host app.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:81
> +bool EndowmentStateTracker::isApplicationUserFacing(pid_t pid)

Is this really needed? If not, I propose we omit it.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:86
> +EndowmentStateTracker& EndowmentStateTracker::singleton()

Nit: Could this be at the top of the file?

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:137
> +    for (auto& client : m_clients)

It is risky to not copy m_clients before iterating.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:149
> +    for (auto& client : m_clients)

ditto.
Comment 7 Jer Noble 2020-06-09 17:57:51 PDT
Created attachment 401501 [details]
Patch
Comment 8 Jer Noble 2020-06-09 18:22:08 PDT
Created attachment 401502 [details]
Patch
Comment 9 Chris Dumez 2020-06-10 08:17:47 PDT
Comment on attachment 401502 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:129
> +    RELEASE_LOG(ViewState, "%p - EndowmentStateTracker::setIsUserFacing(%s)", this, isUserFacing ? "true" : "false");

%{public}s

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:143
> +    RELEASE_LOG(ViewState, "%p - EndowmentStateTracker::setIsVisible(%s)", this, isVisible ? "true" : "false");

%{public}s

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:151
> +

Extra blank line here.
Comment 10 Jer Noble 2020-06-10 09:14:21 PDT
Created attachment 401546 [details]
Patch for landing
Comment 11 EWS 2020-06-10 13:50:03 PDT
Committed r262858: <https://trac.webkit.org/changeset/262858>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401546 [details].
Comment 12 Darin Adler 2020-06-10 14:50:46 PDT
Comment on attachment 401546 [details]
Patch for landing

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

> Source/WTF/wtf/WeakHashSet.h:159
> +struct Mapper<MapFunction, const WeakHashSet<T> &, void> {

Strange space here before the "&".

> Source/WebKit/UIProcess/EndowmentStateTracker.h:49
> +        virtual void isUserFacingChanged(bool) { }
> +        virtual void isVisibleChanged(bool) { }

Is it helpful to pass the new values here instead of just giving a change notification?

Something about this seems unnecessarily complex. Kind of wish we just used change notification functions instead of an object. Those function could hold whatever, WeakPtr or RefPtr or whatever. And it could be a single function. Don’t need precise notifications, do we? As long as we have precise answers to isVisible and isUserFacing. Or if we do need precise notifications, what changed could be an argument to the function.

I guess the downside of adding a function is that it’s not as easy to remove. One downside of all these WeakPtr is that it’s ambiguous whether you need to call removeClient or not.

> Source/WebKit/UIProcess/EndowmentStateTracker.h:69
> +    bool m_isUserFacing;
> +    bool m_isVisible;

Should initialize these here.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:38
> +static NSString* visibilityEndowment = @"com.apple.frontboard.visibility";
> +static NSString* userfacingEndowment = @"com.apple.launchservices.userfacing";

Should be constexpr. Should format NSString * the way we do.

> Source/WebKit/UIProcess/EndowmentStateTracker.mm:148
> +    for (auto& client : copyToVector(m_clients)) {
> +        if (client)
> +            client->isVisibleChanged(m_isVisible);
> +    }

Seems overkill to implement this just so everybody can do ... nothing.
Comment 13 Jonathan Bedard 2020-06-10 15:11:43 PDT
Comment on attachment 401546 [details]
Patch for landing

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

> WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/13/RunningBoardServices.framework/RunningBoardServices.tbd:9
> +    objc-classes:    [ RBSAttribute, RBSDomainAttribute, RBSTarget, RBSAssertion, RBSProcessIdentifier, RBSProcessState, RBSProcessHandle, RBSProcessStateDescriptor, RBSProcessPredicate, RBSProcessMonitor ]

Not on EWS yet, but we now have watchOS and tvOS framework stubs too.
Comment 14 Jer Noble 2020-06-10 15:34:38 PDT
(In reply to Darin Adler from comment #12)
> Comment on attachment 401546 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401546&action=review
> 
> > Source/WTF/wtf/WeakHashSet.h:159
> > +struct Mapper<MapFunction, const WeakHashSet<T> &, void> {
> 
> Strange space here before the "&".

I think I copied these directly from a compiler message w.r.t. the type passed to the original Mapper functor, which is how I got the weird spacing.

> > Source/WebKit/UIProcess/EndowmentStateTracker.h:49
> > +        virtual void isUserFacingChanged(bool) { }
> > +        virtual void isVisibleChanged(bool) { }
> 
> Is it helpful to pass the new values here instead of just giving a change
> notification?

I figured it couldn't hurt; clients are most likely just going to turn around and ask for that value anyway.

> Something about this seems unnecessarily complex. Kind of wish we just used
> change notification functions instead of an object. Those function could
> hold whatever, WeakPtr or RefPtr or whatever. And it could be a single
> function. Don’t need precise notifications, do we? As long as we have
> precise answers to isVisible and isUserFacing. Or if we do need precise
> notifications, what changed could be an argument to the function.
> 
> I guess the downside of adding a function is that it’s not as easy to
> remove.

Yes; it works as a pattern for single-use callbacks. It's more difficult for notifications. Just like -[NSNotificationCenter addObserver:...], you'd need to do something like returning a token which could be used to remove the object later.

But indeed, something like this would let us do client callbacks without having to inherit from (and therefore expose to everyone else) the client class to do so. You could have something like:

```
class NotifyingClassClient : CanMakeWeakPtr<NotifyingClassClient> {
public:
    void setSomethingChangedCallback(Function<void(bool)>&& callback) { m_callback = WTFMove(callback);
private:
    friend class NotifyingClass;
    void somethingChanged(bool change) final { if (m_callback) m_callback(change); }
    Function<void(bool)> m_callback;
};

class NotifyingClass {
public:
    void addClient(Client&);
private:
    WeakHashSet<Client> m_clients;
};
```

Then in the subscriber's header file:

```
class NotifyingClassClient;

class ConcreteClass {
    Ref<NotifyingClassClient> m_client;
};
```

And in the implementation file:

```
#include "NotifyingClass"

ConcreteClass::ConcreteClass()
  : m_client(adoptRef(new NotifyingClassClient())
{
  m_client->setSomethingChangedFunction([this] (bool change) { handleSomethingChanged(change); });
};
```

Cancelling the callback would mean throwing away the m_client object. The ConcreteClass doesn't need to expose NotifyingClass.h in its own header. It can name the callback function whatever it wants.

> One downside of all these WeakPtr is that it’s ambiguous whether you
> need to call removeClient or not.

Seems like a style issue: 1) client objects should always be stored as WeakPtrs.

> > Source/WebKit/UIProcess/EndowmentStateTracker.h:69
> > +    bool m_isUserFacing;
> > +    bool m_isVisible;
> 
> Should initialize these here.

These get initialized by the only constructor, so they don't need initialization in the header.

> > Source/WebKit/UIProcess/EndowmentStateTracker.mm:38
> > +static NSString* visibilityEndowment = @"com.apple.frontboard.visibility";
> > +static NSString* userfacingEndowment = @"com.apple.launchservices.userfacing";
> 
> Should be constexpr. Should format NSString * the way we do.

`const NSString*` definitely caused problems passing these values into ObjC methods which take NSString*, but I can try constexpr.

> > Source/WebKit/UIProcess/EndowmentStateTracker.mm:148
> > +    for (auto& client : copyToVector(m_clients)) {
> > +        if (client)
> > +            client->isVisibleChanged(m_isVisible);
> > +    }
> 
> Seems overkill to implement this just so everybody can do ... nothing.

Chris is going to use this method in a follow up patch.
Comment 15 David Kilzer (:ddkilzer) 2020-06-10 15:39:07 PDT
(In reply to EWS from comment #11)
> Committed r262858: <https://trac.webkit.org/changeset/262858>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 401546 [details].

macOS build fix:
Committed r262866: <https://trac.webkit.org/changeset/262866>
Comment 16 Darin Adler 2020-06-10 16:15:02 PDT
Comment on attachment 401546 [details]
Patch for landing

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

>>> Source/WebKit/UIProcess/EndowmentStateTracker.mm:38
>>> +static NSString* userfacingEndowment = @"com.apple.launchservices.userfacing";
>> 
>> Should be constexpr. Should format NSString * the way we do.
> 
> `const NSString*` definitely caused problems passing these values into ObjC methods which take NSString*, but I can try constexpr.

Yes, "const NSString *" is wrong. "constexpr NSString *" is best. And if we want to do what we might think "const NSString *" does, we can do  "NSString * const", but that’s unnecessary if we use constexpr instead.
Comment 17 Jonathan Bedard 2020-06-10 17:04:21 PDT
Reopening to attach new patch.
Comment 18 Jonathan Bedard 2020-06-10 17:04:23 PDT
Created attachment 401607 [details]
Patch for landing
Comment 19 EWS 2020-06-10 17:46:56 PDT
Committed r262885: <https://trac.webkit.org/changeset/262885>

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