[webkit-reviews] review requested: [Bug 212695] [EME] Create CDMProxyFactory : [Attachment 401897] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has asked  for review:
Bug 212695: [EME] Create CDMProxyFactory
https://bugs.webkit.org/show_bug.cgi?id=212695

Attachment 401897: Patch

https://bugs.webkit.org/attachment.cgi?id=401897&action=review




--- Comment #22 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Created attachment 401897

  --> https://bugs.webkit.org/attachment.cgi?id=401897&action=review

Patch

(In reply to Xabier Rodríguez Calvar from comment #21)
> > > Source/WTF/wtf/LazilyInitialized.h:51
> > > +        , m_deinitializer(WTFMove(deinitializer)) { }
> > 
> > I am not really convinced by LazilyInitialized.
> > I think it would be best to introduce it as a separate follow-up patch.
> > And keep in this patch the usual approach of a class wrapper or a
> > CDMProxyClearKey getter like:
> > CDMProxyClearKey::cryptHandle()
> > {
> >	if (!m_handleInitialized) {
> >	    m_handleInitialized = true;
> >	    initializeGcrypt();
> >	}
> >	return m_gcryHandle;
> > }
> 
> If you have a strong opinion I'll do it, but I don't agree. I think this is
> a good solution.

When I think LazilyInitialized is a more elegant solution, I refactored as you
wanted.

> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:43
> > > +    static NeverDestroyed<Vector<CDMProxyFactory*>> factories;
> > 
> > Maybe this could be a WeakHashSet to prevent UAF.
> > If this is supposed to be keyed by keySystem, maybe it should be a HashMap?
> 
> It could but I don't follow why it should be a WeahHashSet. Who would own
> the factories then?

Finally, this can't be done. Things can't be keyed by key system because a
factory can support more than one system (well, it could be but I think the
alternative of adding the same factory more than once would be worse than
this).

> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:52
> > > +void CDMProxyFactory::registerFactory(CDMProxyFactory& factory)
> > 
> > It seems these methods are not called in this patch.
> > It would be safer to register/unregister in CDMProxyFactory
> > constructor/destructor if possible.
> 
> They are called for GStreamer.

I think now we would be ready for landing if you agree.