[EME] CDMProxy should not keep CDMInstanceSessions hard referenced
Created attachment 400909 [details] Patch
Created attachment 400922 [details] Patch
Comment on attachment 400922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400922&action=review I don't know about the IDL stuff so I only have a question about the test coverage of the null session purger. > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:319 > + return current == value; Why do you have to create an empty WeakPtr object to check if it has been cleared? It looks like WeakPtr::operator bool() would be good enough using removeIf? > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:466 > + return adoptRef(newSession); AFAICT, the test checks this alone (i.e., the double ref bump that existed before being fixed). But the meat of this patch is the WeakPtr and the null session purger, I don't understand how the test checks that?
(In reply to Charlie Turner from comment #3) > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:319 > > + return current == value; > > Why do you have to create an empty WeakPtr object to check if it has been > cleared? It looks like WeakPtr::operator bool() would be good enough using > removeIf? > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:466 > > + return adoptRef(newSession); > > AFAICT, the test checks this alone (i.e., the double ref bump that existed > before being fixed). But the meat of this patch is the WeakPtr and the null > session purger, I don't understand how the test checks that? The session purger is not a big deal, just an insurance but as we discussed in private it is not a big deal, so I am removing it. The rest is tested so I think we'll be good in the next patch.
Created attachment 401260 [details] Patch
(In reply to Xabier Rodríguez Calvar from comment #4) > (In reply to Charlie Turner from comment #3) > > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:319 > > > + return current == value; > > > > Why do you have to create an empty WeakPtr object to check if it has been > > cleared? It looks like WeakPtr::operator bool() would be good enough using > > removeIf? > > > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:466 > > > + return adoptRef(newSession); > > > > AFAICT, the test checks this alone (i.e., the double ref bump that existed > > before being fixed). But the meat of this patch is the WeakPtr and the null > > session purger, I don't understand how the test checks that? > > The session purger is not a big deal, just an insurance but as we discussed > in private it is not a big deal, so I am removing it. The rest is tested so > I think we'll be good in the next patch. OK, I defer the review of the internals stuff to someone who knows that, perhaps Jer could take a look?
Comment on attachment 401260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401260&action=review > Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:89 > + unsigned internalInstanceSessionObjectRefCount() { return m_instanceSession->refCount(); } const > Source/WebCore/Modules/encryptedmedia/MediaKeys.h:83 > + unsigned internalInstanceObjectRefCount() { return m_instance->refCount(); } const > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:316 > +void CDMInstanceProxy::trackSession(WeakPtr<CDMInstanceSessionProxy>&& session) It might be better to use trackSession(CDMInstanceSessionProxy& session). This way, we are sure session is not null. > Source/WebCore/platform/encryptedmedia/CDMProxy.h:201 > + Vector<WeakPtr<CDMInstanceSessionProxy>> m_sessions; Do we care of m_sessions order? Could we use a WeakHashSet here? Looking at the code, I do not see where m_sessions is used/read, the only case I see is trackSession. > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:464 > + CDMInstanceSessionClearKey* newSession = new CDMInstanceSessionClearKey(*this); auto* It might be a tad better to write it as: auto newSession = adoptRef(...) trackSession(newSession.get()); return newSession; This way, the adoptRef is happening sooner, just after the allocation. > Source/WebCore/testing/Internals.h:53 > +#include "MediaKeys.h" Can we forward declare MediaKeySession and MediaKeys? > Source/WebCore/testing/Internals.h:1032 > + unsigned mediaKeySessionInternalInstanceSessionObjectRefCount(MediaKeySession&); Could be const&
Created attachment 401333 [details] Patch (In reply to youenn fablet from comment #7) > > Source/WebCore/testing/Internals.h:53 > > +#include "MediaKeys.h" > > Can we forward declare MediaKeySession and MediaKeys? Nope, compiler complains. The rest is handled.
Comment on attachment 401333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401333&action=review > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:319 > + m_sessions.add(session); It seems m_sessions is only used here. Can we even go further and remove m_sessions?
(In reply to Xabier Rodríguez Calvar from comment #8) > Created attachment 401333 [details] > Patch > > (In reply to youenn fablet from comment #7) > > > Source/WebCore/testing/Internals.h:53 > > > +#include "MediaKeys.h" > > > > Can we forward declare MediaKeySession and MediaKeys? > > Nope, compiler complains. That is a bit sad. What is the compilation error?
(In reply to youenn fablet from comment #9) > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:319 > > + m_sessions.add(session); > > It seems m_sessions is only used here. > Can we even go further and remove m_sessions? Nope, it will be used later in other patches. (In reply to youenn fablet from comment #10) > (In reply to Xabier Rodríguez Calvar from comment #8) > > Created attachment 401333 [details] > > Patch > > > > (In reply to youenn fablet from comment #7) > > > > Source/WebCore/testing/Internals.h:53 > > > > +#include "MediaKeys.h" > > > > > > Can we forward declare MediaKeySession and MediaKeys? > > > > Nope, compiler complains. > > That is a bit sad. What is the compilation error? Easy: DerivedSources/WebCore/JSInternals.cpp:12612:102: error: no viable conversion from 'WebCore::MediaKeys' to incomplete type 'const MediaKeys' auto result = JSValue::encode(toJS<IDLUnsignedLong>(impl.mediaKeysInternalInstanceObjectRefCount(*mediaKeys))); ^~~~~~~~~~ /home/calvaris/gnome/WebKit/Source/WebCore/testing/Internals.h:53:7: note: forward declaration of 'MediaKeys' class MediaKeys; ^ /home/calvaris/gnome/WebKit/Source/WebCore/testing/Internals.h:1028:70: note: passing argument to parameter here unsigned mediaKeysInternalInstanceObjectRefCount(const MediaKeys&) const; ^
Committed r262781: <https://trac.webkit.org/changeset/262781> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401333 [details].
<rdar://problem/64154743>
<rdar://problem/64154744>