Bug 212981 – Catalyst WebKit apps continue to play audio after quitting

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Catalyst WebKit apps continue to play audio after quitting

[Jer Noble]

<rdar://problem/57089471>

[Jer Noble]

Created attachment 401458 [details]
Patch

[Jer Noble]

Created attachment 401472 [details]
Patch

[Chris Dumez]

You are likely going to need to update WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/13/RunningBoardServices.framework/RunningBoardServices.tbd to fix the Open Source build.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Jer Noble]

Created attachment 401501 [details]
Patch

[Jer Noble]

Created attachment 401502 [details]
Patch

[Chris Dumez]

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.

[Jer Noble]

Created attachment 401546 [details]
Patch for landing

[EWS]

Committed r262858: <https://trac.webkit.org/changeset/262858>

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

[Darin Adler]

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.

[Jonathan Bedard]

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.

[Jer Noble]

(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.

[David Kilzer (:ddkilzer)]

(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>

[Darin Adler]

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.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 401607 [details]
Patch for landing

[EWS]

Committed r262885: <https://trac.webkit.org/changeset/262885>

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