Bug 226328 – MediaSession.coordinator should not be optional, relying on coordinator state change instead

• Component: New Bugs
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

MediaSession.coordinator should not be optional, relying on coordinator readyState change instead

[Jean-Yves Avenard [:jya]]

rdar://77461335

[Jean-Yves Avenard [:jya]]

Created attachment 429949 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 429971 [details]
Patch

[Chris Dumez]

Comment on attachment 429971 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:135
>      : ActiveDOMObject(navigator.scriptExecutionContext())

Does this still need to be an ActiveDOMObject? It doesn't seem to be firing events anymore so I doubt it.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:53
> +    return adoptRef(*new MediaSessionCoordinator(session, WTFMove(privateCoordinator)));

You are failing to call suspendIfNeeded() after constructing it here. This is mandatory for ActiveDOMObject and you'll likely hit assertions in debug.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:87
> +    return m_asyncEventQueue->hasPendingActivity() || m_session;

Please check for event listeners as I suggested. We really don't want to keep objects alive longer than they need to be.

Also, do we need to stay alive once the state is MediaSessionCoordinatorState::Closed? Can you ever go back to another state once the state becomes closed? Or does the session gets nulled out when the state becomes closed? I had a look at the code but the link between the state and m_session wasn't clear to me.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:76
> +    void refEventTarget() final { ref(); }

Should not be public.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:77
> +    void derefEventTarget() final { deref(); }

ditto.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:78
> +    EventTargetInterface eventTargetInterface() const final { return MediaSessionCoordinatorEventTargetInterfaceType; }

ditto.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:82
> +    const char* activeDOMObjectName() const final { return "MediaSessionCoordinator"; }

ditto.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:83
> +    bool virtualHasPendingActivity() const final;

ditto.

[Jean-Yves Avenard [:jya]]

Comment on attachment 429971 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:87
>> +    return m_asyncEventQueue->hasPendingActivity() || m_session;
> 
> Please check for event listeners as I suggested. We really don't want to keep objects alive longer than they need to be.
> 
> Also, do we need to stay alive once the state is MediaSessionCoordinatorState::Closed? Can you ever go back to another state once the state becomes closed? Or does the session gets nulled out when the state becomes closed? I had a look at the code but the link between the state and m_session wasn't clear to me.

In theory yes.

The state of the MediaSessionCoordinator is also controlled by the presence of a MediaSessionCoordinatorPrivate ; it is possible that one could be coming.

As for the present of event listener, ultimately, you don't need an event listener to be active to be able to use the MediaSessionCoordinator, JS could do without it in theory (like checking the state regularly).
It's also possible that the MediaSession isn't yet played with as we don't have a media element playing. The JS player would know that.

I can very well imagine conditions where the site is using a polyfill library that will only set up the listener once an external event has occurred. And from the assertion I'm seeing with the object being garbage collected super early on, we definitely need to keep it alive for as long as the media session itself is alive (which is for as long the navigator is alive)

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Comment on attachment 429971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429971&action=review
> 
> >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:87
> >> +    return m_asyncEventQueue->hasPendingActivity() || m_session;
> > 
> > Please check for event listeners as I suggested. We really don't want to keep objects alive longer than they need to be.
> > 
> > Also, do we need to stay alive once the state is MediaSessionCoordinatorState::Closed? Can you ever go back to another state once the state becomes closed? Or does the session gets nulled out when the state becomes closed? I had a look at the code but the link between the state and m_session wasn't clear to me.
> 
> In theory yes.
> 
> The state of the MediaSessionCoordinator is also controlled by the presence
> of a MediaSessionCoordinatorPrivate ; it is possible that one could be
> coming.
> 
> As for the present of event listener, ultimately, you don't need an event
> listener to be active to be able to use the MediaSessionCoordinator, JS
> could do without it in theory (like checking the state regularly).
> It's also possible that the MediaSession isn't yet played with as we don't
> have a media element playing. The JS player would know that.
> 
> I can very well imagine conditions where the site is using a polyfill
> library that will only set up the listener once an external event has
> occurred. And from the assertion I'm seeing with the object being garbage
> collected super early on, we definitely need to keep it alive for as long as
> the media session itself is alive (which is for as long the navigator is
> alive)

This makes no sense, the JS would need to be holding on to the MediaSessionCoordinator JS object in order to register an event listener on it. Therefore, there is no point in the implementation keeping the JS wrapper alive if there is no register event listener (for the only event we fire).

[Jean-Yves Avenard [:jya]]

Comment on attachment 429971 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSession.cpp:135
>>      : ActiveDOMObject(navigator.scriptExecutionContext())
> 
> Does this still need to be an ActiveDOMObject? It doesn't seem to be firing events anymore so I doubt it.

I looked into that.
The reason for keeping it is that MediaSessionCoordinator is an ActiveDOMObject. The MediaSessionCoordinator is created and owned by the MediaSession.
ActiveDOMObject constructor requires a ScriptExecutionContext ; it felt logical to use the MediaSession ones. 

I could make it a ContextDestructionObserver instead.

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 429971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429971&action=review
> 
> >> Source/WebCore/Modules/mediasession/MediaSession.cpp:135
> >>      : ActiveDOMObject(navigator.scriptExecutionContext())
> > 
> > Does this still need to be an ActiveDOMObject? It doesn't seem to be firing events anymore so I doubt it.
> 
> I looked into that.
> The reason for keeping it is that MediaSessionCoordinator is an
> ActiveDOMObject. The MediaSessionCoordinator is created and owned by the
> MediaSession.
> ActiveDOMObject constructor requires a ScriptExecutionContext ; it felt
> logical to use the MediaSession ones. 
> 
> I could make it a ContextDestructionObserver instead.

Yes, ContextDestructionObserver would be much better.

[Chris Dumez]

Comment on attachment 429971 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:154
>  void MediaSession::suspend(ReasonForSuspension reason)

Never mind, MediaSession overrides ActiveDOMObject::suspend() so you like do still need to subclass ActiveDOMObject (for the purpose of suspension).

>> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:53
>> +    return adoptRef(*new MediaSessionCoordinator(session, WTFMove(privateCoordinator)));
> 
> You are failing to call suspendIfNeeded() after constructing it here. This is mandatory for ActiveDOMObject and you'll likely hit assertions in debug.

To be clear, the correct pattern for ActiveDOMObjects is:
auto coordinator = adoptRef(*new MediaSessionCoordinator(WTFMove(privateCoordinator)));
coordinator->suspendIfNeeded();
return coordinator;

Please don't call suspendIfNeeded() in the constructor like some people do, it is subtly wrong and we hit assertions in some cases.

[Jean-Yves Avenard [:jya]]

Created attachment 429979 [details]
Patch

Apply comments

[Chris Dumez]

Comment on attachment 429979 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:87
> +    m_hasActiveListener = hasEventListeners(eventNames().coordinatorstatechangeEvent);

I think m_hasRelevantEventListener or m_hasCoordinatorsStateChangeEventListener would be a better name. It is unclear what an "active" event listener (does it mean the event listener is currently being invoked for e.g.? It could be misinterpreted).

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:94
> +    return m_asyncEventQueue->hasPendingActivity() || m_hasActiveListener;

This doesn't seem quite right. If you have pending events but NO event listener registered for it, then why keep the JS wrapper alive?

I think you may want something like:
return m_hasActiveListener && m_session;

As long as you have a session, you may fire events. And if you may fire event and there are registered event listeners for those events, then the wrapper needs to stay alive.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:53
> +    WEBCORE_EXPORT void setMediaSessionCoordinatorPrivate(Ref<MediaSessionCoordinatorPrivate>&&);

Maybe we should move this below so it isn't between the factory and the destructor.

[Jean-Yves Avenard [:jya]]

Comment on attachment 429979 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:94
>> +    return m_asyncEventQueue->hasPendingActivity() || m_hasActiveListener;
> 
> This doesn't seem quite right. If you have pending events but NO event listener registered for it, then why keep the JS wrapper alive?
> 
> I think you may want something like:
> return m_hasActiveListener && m_session;
> 
> As long as you have a session, you may fire events. And if you may fire event and there are registered event listeners for those events, then the wrapper needs to stay alive.

in practice m_session will always be true and will be for as long as the document/navigator is alive.

>> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:53
>> +    WEBCORE_EXPORT void setMediaSessionCoordinatorPrivate(Ref<MediaSessionCoordinatorPrivate>&&);
> 
> Maybe we should move this below so it isn't between the factory and the destructor.

I grouped the WEBCORE_EXPORT together.

[Chris Dumez]

Comment on attachment 429979 [details]
Patch

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

>>> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:53
>>> +    WEBCORE_EXPORT void setMediaSessionCoordinatorPrivate(Ref<MediaSessionCoordinatorPrivate>&&);
>> 
>> Maybe we should move this below so it isn't between the factory and the destructor.
> 
> I grouped the WEBCORE_EXPORT together.

Sure, you can do that and still move it after the destructor :)

[Jean-Yves Avenard [:jya]]

Created attachment 429982 [details]
Patch

Apply comments part #2

[Chris Dumez]

Comment on attachment 429982 [details]
Patch

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

The ActiveDOMObject stuff looks good but I'll let someone else review the media code :)

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:92
> +    // Need to keep the object alive as long as it may still fire events in the future.

nit: s/object/wrapper

By "object", it sounds like you are referring to this MediaSessionCoordinator implementation object but this function is about the JS wrapper's lifetime.

[Jean-Yves Avenard [:jya]]

Created attachment 429983 [details]
Patch

Apply comments part #3

[Jean-Yves Avenard [:jya]]

Created attachment 430006 [details]
Patch

Update MediaSession IDL WPT test expectations

[Eric Carlson]

Comment on attachment 430006 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:203
> +    m_coordinator->setMediaSession(this);

This shouldn't be necessary as you pass the session to `MediaSessionCoordinator::create`.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:59
> +MediaSessionCoordinator::MediaSessionCoordinator(MediaSession& session, RefPtr<MediaSessionCoordinatorPrivate>&& privateCoordinator)
> +    : ActiveDOMObject(session.scriptExecutionContext())

The only use of `session` here is to get the scriptExecutionContext, so you could either just pass that directly or also call `setMediaSession()` and skip the explicit call from MediaSession::createCoordinator.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:71
> +    if (privateCoordinator) {
> +        m_privateCoordinator = WTFMove(privateCoordinator);
> +        m_privateCoordinator->setLogger(m_logger.copyRef(), m_logIdentifier);
> +        m_privateCoordinator->setClient(makeWeakPtr(this));
> +        coordinatorStateChanged(MediaSessionCoordinatorState::Waiting);
> +    }

I would put all of this logic in `setMediaSessionCoordinatorPrivate` and call it from here.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:93
> +    return m_hasCoordinatorsStateChangeEventListener;

I don't think we should fire events if there is no MediaSession (see below).

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:420
> +    if (m_hasCoordinatorsStateChangeEventListener)

I don't think there is any reason to fire an event if there is no MediaSession, so this should also check `m_session`. Also, you might put the test into a method and also call it from `virtualHasPendingActivity` so we're sure they use the same logic if something changes.

[Jean-Yves Avenard [:jya]]

Created attachment 430186 [details]
Patch

[Eric Carlson]

Comment on attachment 430186 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:68
> +    if (privateCoordinator) {
> +        setMediaSessionCoordinatorPrivate(*privateCoordinator);
> +    }

Nit: WebKit style is to omit braces on a single line condition.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:413
> +bool MediaSessionCoordinator::shouldFireEvents() const {

Nit: opening brace for a method should be on a new line.

[Jean-Yves Avenard [:jya]]

Created attachment 430310 [details]
Patch

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Jean-Yves Avenard [:jya]]

Created attachment 430311 [details]
Patch

[EWS]

Committed r278336 (238369@main): <https://commits.webkit.org/238369@main>

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