Bug 212689 – [EME] CDMProxyInstance should not keep CDMInstanceSessions hard referenced

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[EME] CDMProxy should not keep CDMInstanceSessions hard referenced

[Xabier Rodríguez Calvar]

Created attachment 400909 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400922 [details]
Patch

[Charlie Turner]

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?

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

Created attachment 401260 [details]
Patch

[Charlie Turner]

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

[youenn fablet]

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&

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

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?

[youenn fablet]

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

[Xabier Rodríguez Calvar]

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

[EWS]

Committed r262781: <https://trac.webkit.org/changeset/262781>

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

[Radar WebKit Bug Importer]

<rdar://problem/64154743>

[Radar WebKit Bug Importer]

<rdar://problem/64154744>