Summary: | MediaSession.coordinator should not be optional, relying on coordinator state change instead | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kangil.han, kondapallykalyan, philipj, sergio, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 226459, 226460 | ||||||||||||||||||||||
Attachments: |
|
Description
Jean-Yves Avenard [:jya]
2021-05-27 06:27:02 PDT
Created attachment 429949 [details]
Patch
Created attachment 429971 [details]
Patch
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. 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) (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). 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. (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. 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. Created attachment 429979 [details]
Patch
Apply comments
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. 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. 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 :) Created attachment 429982 [details]
Patch
Apply comments part #2
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. Created attachment 429983 [details]
Patch
Apply comments part #3
Created attachment 430006 [details]
Patch
Update MediaSession IDL WPT test expectations
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. Created attachment 430186 [details]
Patch
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. Created attachment 430310 [details]
Patch
ChangeLog entry in Tools/ChangeLog contains OOPS!. Created attachment 430311 [details]
Patch
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]. |