1016162 - [EME] Encrypted Media Extensions base DOM API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

This bug covers the DOM changes required to get a basic implementation of Encrypted Media Extensions, as per:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-media.html#extensions

The spec is still in flux, and I don't intend to do a verbatim implementation at this time. I want to get the basic DOM APIs implemented, and follow up after the spec stabilizes a bit more.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1: EME DOM APIs

First cut at EME DOM APIs.

The DOM APIs work, but there's no CDM plugged into to them. Instead there's a CDMProxy class that pretends to conform to the API I roughly expect to use to communicate with the CDM via IPC. So there's no need to review the functions in CDMProxy.cpp with a "// TODO: Dispatch task to do X via IPC" thoroughly, they are just placeholders which I'll replace in future.

MediaKeyError is implemented here as inheriting Event rather than inheriting DOMException since I'm not sure what code and NSResult to pass to DOMException's constructor, and MediaKeyError may be removed from the spec anyway. So I'll fix it up once the spec stabilizes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8429001 [details] [diff] [review]
Patch v1: EME DOM APIs

I'm requesting a bunch of spec bugs be filed below.  Please cc me on those bugs.

>+++ b/content/html/content/public/HTMLMediaElement.h
>+  already_AddRefed<MediaKeys> GetKeys() const;

The current spec draft names this attribute "mediaKeys", not "keys", right?

>+    // TODO: "If candidate has a keySystem attribute whose value represents a

File a bug (blocking turning this on, presumably), refer to the bug# here.

>+already_AddRefed<MediaKeys>
>+HTMLMediaElement::GetKeys() const
>+{
>+  nsRefPtr<MediaKeys> keys(mMediaKeys);
>+  return keys.forget();

  MediaKeys*
  HTMLMediaElement::GetKeys() const
  {
    return mMediaKeys;
  }

>+HTMLMediaElement::SetMediaKeys(mozilla::dom::MediaKeys* aMediaKeys,
>+  // TODO: Need to shutdown existing MediaKeys instance?

File bug, bug# here, etc.

>+++ b/content/html/content/src/HTMLSourceElement.h
>+  void GetKeySystem(nsString& aKeySystem) const

So... the spec draft doesn't actually define the behavior of the IDL attribute here.  It should specify that it reflects the content attribute, right?  Please raise a spec issue.

>+++ b/content/media/eme/CDMProxy.cpp
>+#include "CDMProxy.h"

Should probably be mozilla/CDMProxy.h and we should export into mozilla/, right?

>+CDMProxy::Init(PromiseId aPromiseId)

Why the integer handle instead of just holding a reference to the Promise?  Is the worry about releasing it on the right thread?  But you have that problem anyway with mKeys, I'd think...

At the very least, this handle setup needs documentation.

>+CDMProxy::CreateSession(dom::SessionType aSessionType,
>+                        PromiseId aPromiseId,
>+                        const nsAString& aInitDataType,
>+                        uint8_t* aInitData,
>+                        uint32_t aInitDataSize)

If the caller always has a dom::Uint8Array, can we pass that through to here?  Getting the data/length as late as possible is a good idea.

Also note you'll need a ComputeLengthAndData() before getting .Data() and .Length(), now that https://hg.mozilla.org/integration/mozilla-inbound/rev/2c631967ab9e has landed.

>+CDMProxy::UpdateSession(const nsAString& aSessionId,
>+                        PromiseId aPromiseId,
>+                        const uint8_t* response,
>+                        int response_size)

Again, pass in the dom::Uint8Array.  Once we have an actual async section here, we'll need to be really careful with that array....

>+  // aCode == NS_OK will reject promise with "undefined".

Why would one ever reject with undefined?  That doesn't really make sense, and we shouldn't support it.

>+  void ResolvePromise(PromiseId aId);

Resolves with undefined, right?

>+  // WARNING: Non-owning reference! Cleared by MediaKeys destructor.
>+  //          Always nullcheck before using!
>+  // WARNING: dom::MediaKeys is main thread only, including its refcounting!
>+  dom::MediaKeys* mKeys;

So the point is that mKeys should only ever be touched on main thread, right?  Null-checking it on another thread is definitely a bug.

I wonder whether it makes sense to wrap this in a "smart-pointer" class with a conversion operator to dom::MediaKeys* which asserts that the code doing the access is mainthread-only or something.

>+++ b/content/media/eme/EMELog.h
>+#ifndef GetEMEVerboseLog

EME_VERBOSE_LOG, you mean?

Why are we using two different log modules instead of just the existing different log levels?

>+++ b/content/media/eme/MediaKeyError.cpp

I'm mostly ignoring this file as long as we don't ship it in the state it's in, per comments in the bug, and because I think the spec here is wrong (filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25896 ).  That said, subclassing DOMException would be really easy.  And why do you need to subclass Event at all?

>+++ b/content/media/eme/MediaKeyError.h
>+struct JSContext;

js/TypeDecls.h, please.

>+class HTMLMediaElement;

Unused here?  Did you mean EventTarget?

>\ No newline at end of file

Please fix.

>diff --git a/content/media/eme/MediaKeyMessageEvent.cpp b/content/media/eme/MediaKeyMessageEvent.cpp

Is there a reason we can't just use event codegen here?

>+MediaKeyMessageEvent::Constructor(const GlobalObject& aGlobal,
>+  aRv = e->InitEvent(aType, false, false);

This is wrong.  It should be using the values from the dictionary.

>+  e->SetTrusted(true);

This is also wrong.  Why should this event be trusted?  I'd think it _shouldn't_.

What about "message" and "destinationURL"?  Then again, the spec doesn't define how those should behave anyway; that needs to be raised as a spec issue.  The spec is simply not implementable as written.

>+MediaKeyMessageEvent::InitEvent(const nsTArray<uint8_t>& aMessage,
>+                                const nsString& aURL)

_This_ function is where you should be doing SetTrusted(true).

>+  InitEvent(NS_LITERAL_STRING("keymessage"), false, false);

Spec seems to say "message", not "keymessage", no?

>+JSObject*
>+MediaKeyMessageEvent::Message(JSContext* aCx)
>+{
>+  return Uint8Array::Create(aCx, this, mMessage.Length(), mMessage.Elements());

This seems pretty clearly wrong to me.  It will return a new object on every single get.  You probably want to be storing the actual JSObject* and creating it only once (possibly in the event init code).

>+++ b/content/media/eme/MediaKeyNeededEvent.cpp
>+MediaKeyNeededEvent::Constructor(const GlobalObject& aGlobal,

Again, the spec is totally underdefined here.  Please file a spec issue.

>+  aRv = e->InitEvent(aType, false, false);
>+  e->SetTrusted(true);

Again, these are both wrong.

>+MediaKeyNeededEvent::GetInitData(JSContext* aCx)
>+  return Uint8Array::Create(aCx, this, mInitData.Length(), mInitData.Elements());

Again, returning a new object on each get is probably wrong.

>+MediaKeyNeededEvent::InitEvent(const nsAString& aEventType,

This function seems to be unused.  I didn't review it very carefully, since we should just remove it until we have uses.

>+++ b/content/media/eme/MediaKeySession.cpp
>+#include "CDMProxy.h"

mozilla/CDMProxy.h

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MediaKeySession,

This one is not doing anything interesting.  Why can we not just use NS_IMPL_CYCLE_COLLECTION_INHERITED(MediaKeySession, DOMEventTargetHelper, members)?

>+MediaKeySession::MediaKeySession(nsISupports* aParent,
>+  : mParent(aParent)

Why is mParent needed?  Seems like we should make aParent be nsPIDOMWindow and just call teh corresponding DOMEventTargetHelper ctor.  That would also handle calling SetIsDOMBinding() for us.

>+  MOZ_COUNT_CTOR(MediaKeySession);
>+  MOZ_COUNT_DTOR(MediaKeySession);

Doesn't really make sense for a refcounted class.

>+already_AddRefed<MediaKeyError>
>+MediaKeySession::GetError() const

Just return a MediaKeyError*.

>+MediaKeySession::GetParentObject() const

And you could drop this, since DOMEventTargetHelper has a perfectly good GetParentObject() already if you pass it a parent as a ctor argument.

>+Date
>+MediaKeySession::Expiration() const
>+{
>+  // TODO: Return undefined if date is not set. How to do that?

By filing a spec bug.  The prose here flat-out contradicts the IDL.

The right IDL here is to return a numeric timestamp, in ms from the epoch or something.  Probably with NaN representing no timestamp.  In any case, spec bug.

>+already_AddRefed<Promise>
>+MediaKeySession::Closed() const

Just return Promise*?

>+MediaKeySession::Update(const Uint8Array& aResponse)
>+    promise->MaybeReject("InvalidStateError");

This isn't following the spec.  You want something more like:

  promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);

and even then your message won't be right per what the spec says.  If you want that, you should add a new line to dom/base/domerr.msg and a corresponding new nsresult.  But I think what this spec says for the message is bogus.  Afaict, the DOM spec defines that the message just depends on the name for DOMException, so can't vary based on what specs want to do. Please file a spec bug and make sure to cc annevk (and me) on it.

>+MediaKeySession::Remove()
>+    promise->MaybeReject("InvalidAccessError");

See comments for Update().

>+    promise->MaybeReject("InvalidStateError");

And here.  This time you want NS_ERROR_DOM_INVALID_STATE_ERR.

>+class AsyncEventRunner MOZ_FINAL: public nsRunnable {

Is there a reason we're reinventing mozilla::AsyncEventDispatcher?  We seem to have several other instances of such reinvention in the tree too.  :(

>+++ b/content/media/eme/MediaKeySession.h
>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<MediaKeyError> GetError() const;

You can return raw pointers here.

>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<Promise> Closed() const;

Likewise.

>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<Promise> Update(const Uint8Array& response);

Remove that comment.

Also, please annotate the method as [NewObject], since it is?  The spec should do that too -- needs a spec issue

>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<Promise> Close();

>+  // Mark this as resultNotAddRefed to return raw pointers
>+  already_AddRefed<Promise> Remove();

Likewise for both of these.

>+++ b/content/media/eme/MediaKeys.cpp
>+#include "CDMProxy.h"

mozilla/CDMProxy ?

>+TraverseKeySessions(const nsAString& aKey,
>+TraversePromiseMap(const uint32_t& aKey, dom::Promise* aPromise, void* aArg)
>+TraversePendingPromises(const uint32_t& aKey, MediaKeySession* aSession, void* aArg)

You shouldnt need those.

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeys)
>+  tmp->mKeySessions.Clear();

NS_IMPL_CYCLE_COLLECTION_UNLINK(mKeySessions) should work fine.  Same for mPromises and mPendingSessions.

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeys)
>+  tmp->mKeySessions.EnumerateRead(TraverseKeySessions, &cb);

NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mKeySessions) should work fine.  Same for mPromises and mPendingSessions.

So I think you should be able to do just:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeys, mParent, mKeySessions, mPromises, mPendingSessions);

>+MediaKeys::MakePromise()
>+    NS_WARNING("Passed non-global to MediaKeys::MakePromise!");

To MediaKeys ctor, no?

>+MediaKeys::StorePromise(Promise* aPromise)
>+  mPromises.Put(id, aPromise);

Do we ever remove things from mPromises?  This seems like a leak to me...  Once we've resolved a promise and have no plans to get at it again, we should be letting it die as needed.

>+MediaKeys::RetreivePromise(PromiseId aId)

"Retrieve"

>+MediaKeys::RejectPromise(PromiseId aId, nsresult aCode)
>+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");

s/resovle/reject/?

>+    mPendingSessions.Remove(aId);

Document why, please.

>+  if (aCode == NS_OK) {

Like I said, this should not be supported.

>+    nsRefPtr<DOMException> ex(DOMException::Create(aCode));
>+    promise->MaybeReject(ex);

  promise->MaybeReject(aCode);

will do the right thing.

>+MediaKeys::ResolvePromise(PromiseId aId)
>+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");

s/resovle/resolve/

>+  nsRefPtr<CDMProxy> proxy(new CDMProxy(keys, aKeySystem));
>+  keys->mProxy = proxy.forget();

Why not:

  keys->mProxy = new CDMProxy(keys, aKeySystem);

?

>+MediaKeys::OnCDMCreated(PromiseId aId)
>+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");

"resolve"

>+MediaKeys::LoadSession(const nsAString& aSessionId)
>+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));

See above about rejecting with InvalidAccessError.

>+  // TODO: The spec doesn't specify what to do in this case...

Please raise a spec issue!

>+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));

See above about rejecting promises with InvalidAccessError.

>+MediaKeys::OnSessionActivated(PromiseId aId, const nsAString& aSessionId)
>+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");

"resolve"

>+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));

The usual.

>+MediaKeys::GetSesssion(const nsAString& aSessionId)

"GetSession", no?  No extra s.

>+  // Removes promise from mPromises, and returns it.
>+  already_AddRefed<Promise> RetreivePromise(PromiseId aId);

It's not removing anything from mPromises afaict.

>+++ b/dom/events/EventNameList.h
>+EVENT(needkey,
>+      NS_NEED_KEY,
>+      EventNameType_HTML,
>+      NS_EVENT)

Hmm.  This is adding an event handler attribute for all HTML elements, as far as I can tell.  The spec doesn't seem to say to do this.  Does the spec need fixing, or this code?

>+++ b/dom/interfaces/core/nsIInlineEventHandlers.idl
>+  [implicit_jscontext] attribute jsval onneedkey;

I don't think we need this here.

>+++ b/dom/webidl/EventHandler.webidl
>+           attribute EventHandler onneedkey;

Likewise.  This is putting a new event handler attribute on everything in sight, whereas the spec just says to put it on HTMLMediaElement.

>+//EME extensions
>+[Pref="media.eme.enabled"]
>+partial interface HTMLMediaElement {

That [Pref] annotation on a partial interface is a no-op.  We should make codegen fail when people do that....

You want it on all the methods/attributes you're adding.

>+  readonly attribute MediaKeys? keys;

This is not nullable in the spec.  That's clearly a spec bug, which you should file.

>+++ b/dom/webidl/HTMLSourceElement.webidl
>+  attribute DOMString keySystem;

Needs to be behind the pref.

>\ No newline at end of file

Fix, please.

>+++ b/dom/webidl/MediaError.webidl
>+  const unsigned short MEDIA_ERR_5 = ENCRYPTED;

Needs to be behind the pref.  Also, I don't see this in the EME spec.  Am I just missing it?  https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/C3h1LApSVx8/G2s2OV5azvgJ suggests not, and that it's been removed.

>+++ b/dom/webidl/MediaKeySession.webidl
>+  readonly attribute Date expiration;

Like I said above, using Date, especially as an attribute, is an antipattern that we shouldn't propagate.  Spec issue needed.

r=me with those issues addressed, though I'd like to see an interdiff...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Also, you should probably send an intent to implement mail.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for quick review! Comments inline below.

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8429001 [details] [diff] [review]
> Patch v1: EME DOM APIs
> 
> I'm requesting a bunch of spec bugs be filed below.  Please cc me on those
> bugs.
> 
> >+++ b/content/html/content/public/HTMLMediaElement.h
> >+  already_AddRefed<MediaKeys> GetKeys() const;
> 
> The current spec draft names this attribute "mediaKeys", not "keys", right?
> 
> >+    // TODO: "If candidate has a keySystem attribute whose value represents a
> 
> File a bug (blocking turning this on, presumably), refer to the bug# here.

Bug 1016707

> 
> >+already_AddRefed<MediaKeys>
> >+HTMLMediaElement::GetKeys() const
> >+{
> >+  nsRefPtr<MediaKeys> keys(mMediaKeys);
> >+  return keys.forget();
> 
>   MediaKeys*
>   HTMLMediaElement::GetKeys() const
>   {
>     return mMediaKeys;
>   }
> 
> >+HTMLMediaElement::SetMediaKeys(mozilla::dom::MediaKeys* aMediaKeys,
> >+  // TODO: Need to shutdown existing MediaKeys instance?
> 
> File bug, bug# here, etc.

bug 1016709.


> 
> >+++ b/content/html/content/src/HTMLSourceElement.h
> >+  void GetKeySystem(nsString& aKeySystem) const
> 
> So... the spec draft doesn't actually define the behavior of the IDL
> attribute here.  It should specify that it reflects the content attribute,
> right?  Please raise a spec issue.
> 
> >+++ b/content/media/eme/CDMProxy.cpp
> >+#include "CDMProxy.h"
> 
> Should probably be mozilla/CDMProxy.h and we should export into mozilla/,
> right?
> 
> >+CDMProxy::Init(PromiseId aPromiseId)
> 
> Why the integer handle instead of just holding a reference to the Promise?

Because we send the integer handle via IPC across to the CDM, to allow the CDM to choose to resolve/reject promises by their ID's.

Basically we're implementing an API that will be equivalent to the Host_5 and ContentDecryptionModule_5 interfaces here:

https://chromium.googlesource.com/chromium/cdm/+/master/content_decryption_module.h


> Is the worry about releasing it on the right thread?  But you have that
> problem anyway with mKeys, I'd think...

Resolving or rejecting it on the main thread is an issue which I tried to sovle by wrapping the release inside CDMProxy::{Resolve,Reject}Promise().

> At the very least, this handle setup needs documentation.

OK.


> >+CDMProxy::CreateSession(dom::SessionType aSessionType,
> >+                        PromiseId aPromiseId,
> >+                        const nsAString& aInitDataType,
> >+                        uint8_t* aInitData,
> >+                        uint32_t aInitDataSize)
> 
> If the caller always has a dom::Uint8Array, can we pass that through to
> here?  Getting the data/length as late as possible is a good idea.

Sure. :)

> Also note you'll need a ComputeLengthAndData() before getting .Data() and
> .Length(), now that
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2c631967ab9e has
> landed.

Thanks!

> >+CDMProxy::UpdateSession(const nsAString& aSessionId,
> >+                        PromiseId aPromiseId,
> >+                        const uint8_t* response,
> >+                        int response_size)
> 
> Again, pass in the dom::Uint8Array.  Once we have an actual async section
> here, we'll need to be really careful with that array....

The array is copied when it gets sent over IPC I believe. We'll probably have to copy it again here when we dispatch it to the GMP thread, as we don't want to be hanging on to a reference to a JS object's data and using it off main thread...

> 
> >+  // aCode == NS_OK will reject promise with "undefined".
> 
> Why would one ever reject with undefined?  That doesn't really make sense,
> and we shouldn't support it.

Ah yes, the spec never requires this for promise rejection. Thanks!


> 
> >+  void ResolvePromise(PromiseId aId);
> 
> Resolves with undefined, right?
> 
> >+  // WARNING: Non-owning reference! Cleared by MediaKeys destructor.
> >+  //          Always nullcheck before using!
> >+  // WARNING: dom::MediaKeys is main thread only, including its refcounting!
> >+  dom::MediaKeys* mKeys;
> 
> So the point is that mKeys should only ever be touched on main thread,
> right?

Yes.

>  Null-checking it on another thread is definitely a bug.
> 
> I wonder whether it makes sense to wrap this in a "smart-pointer" class with
> a conversion operator to dom::MediaKeys* which asserts that the code doing
> the access is mainthread-only or something.

Sounds reasonable and easy enough.

> 
> >+++ b/content/media/eme/EMELog.h
> >+#ifndef GetEMEVerboseLog
> 
> EME_VERBOSE_LOG, you mean?
> 
> Why are we using two different log modules instead of just the existing
> different log levels?


Because logging something on every video and audio sample results in hundreds of log messages per second, and bug 999898 isn't fixed yet.

> 
> >+++ b/content/media/eme/MediaKeyError.cpp
> 
> I'm mostly ignoring this file as long as we don't ship it in the state it's
> in, per comments in the bug, and because I think the spec here is wrong
> (filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25896 ).  That said,
> subclassing DOMException would be really easy.  And why do you need to
> subclass Event at all?

At the time I first implemented this spec, this object subclassed Event.

I filed bug 1016711 to clean this up.


> 
> >+++ b/content/media/eme/MediaKeyError.h
> >+struct JSContext;
> 
> js/TypeDecls.h, please.
> 
> >+class HTMLMediaElement;
> 
> Unused here?  Did you mean EventTarget?
> 
> >\ No newline at end of file
> 
> Please fix.
> 
> >diff --git a/content/media/eme/MediaKeyMessageEvent.cpp b/content/media/eme/MediaKeyMessageEvent.cpp
> 
> Is there a reason we can't just use event codegen here?

Only my ignorance of event codegen. Fortunately that's something we can fix. ;)

Presumably we can still instantiate MediaKeyMessageEvent from C++ if it's implemented this way? The initData and destinationURL come from C++.

And I presume that most of these comments below will be redundant if I re-implement this via event codegen?
 

> >+MediaKeyMessageEvent::Constructor(const GlobalObject& aGlobal,
> >+  aRv = e->InitEvent(aType, false, false);
> 
> This is wrong.  It should be using the values from the dictionary.
> 
> >+  e->SetTrusted(true);
> 
> This is also wrong.  Why should this event be trusted?  I'd think it
> _shouldn't_.
> 

Whoops!


> What about "message" and "destinationURL"?  

Whoops, looks like I forgot them.

> Then again, the spec doesn't
> define how those should behave anyway; that needs to be raised as a spec
> issue.  The spec is simply not implementable as written.

I'm not sure what the issue is here. Could you elaborate please?

I think the message and destinationURL members are keysystem specific.

For example their contents are defined for the ClearKey CDM here:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-media.html#clear-key

Is that sufficient specification, or should we file another spec bug here?


> >+MediaKeyMessageEvent::InitEvent(const nsTArray<uint8_t>& aMessage,
> >+                                const nsString& aURL)
> 
> _This_ function is where you should be doing SetTrusted(true).
> 
> >+  InitEvent(NS_LITERAL_STRING("keymessage"), false, false);
> 
> Spec seems to say "message", not "keymessage", no?
> 
> >+JSObject*
> >+MediaKeyMessageEvent::Message(JSContext* aCx)
> >+{
> >+  return Uint8Array::Create(aCx, this, mMessage.Length(), mMessage.Elements());
> 
> This seems pretty clearly wrong to me.  It will return a new object on every
> single get.  You probably want to be storing the actual JSObject* and
> creating it only once (possibly in the event init code).
> 
> >+++ b/content/media/eme/MediaKeyNeededEvent.cpp
> >+MediaKeyNeededEvent::Constructor(const GlobalObject& aGlobal,
> 
> Again, the spec is totally underdefined here.  Please file a spec issue.

Do you mean that what is in the init data is undefined? There's a registry where it's defined for various formats here:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/initdata-format-registry.html

Is that sufficient, or were you looking for something else?

> 
> >+  aRv = e->InitEvent(aType, false, false);
> >+  e->SetTrusted(true);
> 
> Again, these are both wrong.
> 
> >+MediaKeyNeededEvent::GetInitData(JSContext* aCx)
> >+  return Uint8Array::Create(aCx, this, mInitData.Length(), mInitData.Elements());
> 
> Again, returning a new object on each get is probably wrong.
> 
> >+MediaKeyNeededEvent::InitEvent(const nsAString& aEventType,
> 
> This function seems to be unused.  I didn't review it very carefully, since
> we should just remove it until we have uses.
> 
> >+++ b/content/media/eme/MediaKeySession.cpp
> >+#include "CDMProxy.h"
> 
> mozilla/CDMProxy.h
> 
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MediaKeySession,
> 
> This one is not doing anything interesting.  Why can we not just use
> NS_IMPL_CYCLE_COLLECTION_INHERITED(MediaKeySession, DOMEventTargetHelper,
> members)?
> 
> >+MediaKeySession::MediaKeySession(nsISupports* aParent,
> >+  : mParent(aParent)
> 
> Why is mParent needed?  Seems like we should make aParent be nsPIDOMWindow
> and just call teh corresponding DOMEventTargetHelper ctor.  That would also
> handle calling SetIsDOMBinding() for us.
> 
> >+  MOZ_COUNT_CTOR(MediaKeySession);
> >+  MOZ_COUNT_DTOR(MediaKeySession);
> 
> Doesn't really make sense for a refcounted class.
> 
> >+already_AddRefed<MediaKeyError>
> >+MediaKeySession::GetError() const
> 
> Just return a MediaKeyError*.
> 
> >+MediaKeySession::GetParentObject() const
> 
> And you could drop this, since DOMEventTargetHelper has a perfectly good
> GetParentObject() already if you pass it a parent as a ctor argument.
> 
> >+Date
> >+MediaKeySession::Expiration() const
> >+{
> >+  // TODO: Return undefined if date is not set. How to do that?
> 
> By filing a spec bug.  The prose here flat-out contradicts the IDL.
> 
> The right IDL here is to return a numeric timestamp, in ms from the epoch or
> something.  Probably with NaN representing no timestamp.  In any case, spec
> bug.

You mean, we should propose that this returns an integer number of milliseconds, rather than a Date object?

How about we propose a nullable Date, i.e.:

  readonly attribute? Date expired;

Then it's semantically still a "wall clock timestamp", i.e. a Date.  
  
I don't work much on the DOM or specs, so I don't understand why using a Date object here is bad.
  
  
> >+already_AddRefed<Promise>
> >+MediaKeySession::Closed() const
> 
> Just return Promise*?
> 
> >+MediaKeySession::Update(const Uint8Array& aResponse)
> >+    promise->MaybeReject("InvalidStateError");
> 
> This isn't following the spec.  You want something more like:
> 
>   promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> 
> and even then your message won't be right per what the spec says.  If you
> want that, you should add a new line to dom/base/domerr.msg and a
> corresponding new nsresult.  But I think what this spec says for the message
> is bogus.  Afaict, the DOM spec defines that the message just depends on the
> name for DOMException, so can't vary based on what specs want to do. Please
> file a spec bug and make sure to cc annevk (and me) on it.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25899

 
> >+MediaKeySession::Remove()
> >+    promise->MaybeReject("InvalidAccessError");
> 
> See comments for Update().
> 
> >+    promise->MaybeReject("InvalidStateError");
> 
> And here.  This time you want NS_ERROR_DOM_INVALID_STATE_ERR.
> 
> >+class AsyncEventRunner MOZ_FINAL: public nsRunnable {
> 
> Is there a reason we're reinventing mozilla::AsyncEventDispatcher?  We seem
> to have several other instances of such reinvention in the tree too.  :(
> 
> >+++ b/content/media/eme/MediaKeySession.h
> >+  // Mark this as resultNotAddRefed to return raw pointers
> >+  already_AddRefed<MediaKeyError> GetError() const;
> 
> You can return raw pointers here.
> 
> >+  // Mark this as resultNotAddRefed to return raw pointers
> >+  already_AddRefed<Promise> Closed() const;
> 
> Likewise.
> 
> >+  // Mark this as resultNotAddRefed to return raw pointers
> >+  already_AddRefed<Promise> Update(const Uint8Array& response);
> 
> Remove that comment.
> 
> Also, please annotate the method as [NewObject], since it is?  The spec
> should do that too -- needs a spec issue
> 
> >+  // Mark this as resultNotAddRefed to return raw pointers
> >+  already_AddRefed<Promise> Close();
> 
> >+  // Mark this as resultNotAddRefed to return raw pointers
> >+  already_AddRefed<Promise> Remove();
> 
> Likewise for both of these.
> 
> >+++ b/content/media/eme/MediaKeys.cpp
> >+#include "CDMProxy.h"
> 
> mozilla/CDMProxy ?
> 
> >+TraverseKeySessions(const nsAString& aKey,
> >+TraversePromiseMap(const uint32_t& aKey, dom::Promise* aPromise, void* aArg)
> >+TraversePendingPromises(const uint32_t& aKey, MediaKeySession* aSession, void* aArg)
> 
> You shouldnt need those.
> 
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeys)
> >+  tmp->mKeySessions.Clear();
> 
> NS_IMPL_CYCLE_COLLECTION_UNLINK(mKeySessions) should work fine.  Same for
> mPromises and mPendingSessions.
> 
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeys)
> >+  tmp->mKeySessions.EnumerateRead(TraverseKeySessions, &cb);
> 
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mKeySessions) should work fine.  Same for
> mPromises and mPendingSessions.
> 
> So I think you should be able to do just:
> 
>   NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeys, mParent, mKeySessions,
> mPromises, mPendingSessions);
> 
> >+MediaKeys::MakePromise()
> >+    NS_WARNING("Passed non-global to MediaKeys::MakePromise!");
> 
> To MediaKeys ctor, no?
> 
> >+MediaKeys::StorePromise(Promise* aPromise)
> >+  mPromises.Put(id, aPromise);
> 
> Do we ever remove things from mPromises?  This seems like a leak to me... 
> Once we've resolved a promise and have no plans to get at it again, we
> should be letting it die as needed.

Oops, RetrievePromise was supposed to remove it. I must have lost this while cleaning up.

> >+MediaKeys::RetreivePromise(PromiseId aId)
> 
> "Retrieve"
> 
> >+MediaKeys::RejectPromise(PromiseId aId, nsresult aCode)
> >+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");
> 
> s/resovle/reject/?
> 
> >+    mPendingSessions.Remove(aId);
> 
> Document why, please.
> 
> >+  if (aCode == NS_OK) {
> 
> Like I said, this should not be supported.
> 
> >+    nsRefPtr<DOMException> ex(DOMException::Create(aCode));
> >+    promise->MaybeReject(ex);
> 
>   promise->MaybeReject(aCode);
> 
> will do the right thing.
> 
> >+MediaKeys::ResolvePromise(PromiseId aId)
> >+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");
> 
> s/resovle/resolve/
> 
> >+  nsRefPtr<CDMProxy> proxy(new CDMProxy(keys, aKeySystem));
> >+  keys->mProxy = proxy.forget();
> 
> Why not:
> 
>   keys->mProxy = new CDMProxy(keys, aKeySystem);
> 
> ?
> 
> >+MediaKeys::OnCDMCreated(PromiseId aId)
> >+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");
> 
> "resolve"
> 
> >+MediaKeys::LoadSession(const nsAString& aSessionId)
> >+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));
> 
> See above about rejecting with InvalidAccessError.
> 
> >+  // TODO: The spec doesn't specify what to do in this case...
> 
> Please raise a spec issue!

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25900


> 
> >+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));
> 
> See above about rejecting promises with InvalidAccessError.
> 
> >+MediaKeys::OnSessionActivated(PromiseId aId, const nsAString& aSessionId)
> >+    NS_WARNING("MediaKeys tried to resovle a non-existent promise");
> 
> "resolve"
> 
> >+    promise->MaybeReject(NS_LITERAL_STRING("InvalidAccessError"));
> 
> The usual.
> 
> >+MediaKeys::GetSesssion(const nsAString& aSessionId)
> 
> "GetSession", no?  No extra s.
> 
> >+  // Removes promise from mPromises, and returns it.
> >+  already_AddRefed<Promise> RetreivePromise(PromiseId aId);
> 
> It's not removing anything from mPromises afaict.
> 
> >+++ b/dom/events/EventNameList.h
> >+EVENT(needkey,
> >+      NS_NEED_KEY,
> >+      EventNameType_HTML,
> >+      NS_EVENT)
> 
> Hmm.  This is adding an event handler attribute for all HTML elements, as
> far as I can tell.  The spec doesn't seem to say to do this.  Does the spec
> need fixing, or this code?

I'm not sure. I don't think it makes sense for all HTML elements to have a needkey event handler attribute. We don't want EME on non media elements!

I just cargo-culted the stuff that made media element's "loadedmetadata" event handler work. All the media elements EventHandler attributes (oncanplay, oncanplaythrough, etc) are defined in the GlobalEventHandlers:
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#globaleventhandlers

Maybe that's a bug too?

How do I make onneedkey only appear on the media element? If I remove the onneedkey stuff I added to dom/interfaces/core/nsIInlineEventHandlers.idl and dom/webidl/EventHandler.webidl (but not dom/events/EventNameList.h) as you suggested, I get build errors like so:

 0:54.42 c:\Users\cpearce\src\mozilla\purple\objdir\dist\include\mozilla/EventNameList.h(299) : error C2511: 'nsresult nsGlobalWindow::GetOnneedkey(JSContext *,
JS::MutableHandle<T>)' : overloaded member function not found in 'nsGlobalWindow'
 0:54.42         with
 0:54.42         [
 0:54.42             T=JS::Value
 0:54.42         ]
 0:54.42         c:\users\cpearce\src\mozilla\purple\dom\base\nsGlobalWindow.h(317) : see declaration of 'nsGlobalWindow'
 0:54.42 c:\Users\cpearce\src\mozilla\purple\objdir\dist\include\mozilla/EventNameList.h(299) : error C2511: 'nsresult nsGlobalWindow::SetOnneedkey(JSContext *,
JS::Handle<T>)' : overloaded member function not found in 'nsGlobalWindow'
 0:54.43         with
 0:54.43         [
 0:54.43             T=JS::Value
 0:54.43         ]
 0:54.43         c:\users\cpearce\src\mozilla\purple\dom\base\nsGlobalWindow.h(317) : see declaration of 'nsGlobalWindow'
 0:54.43
 
 

> >+++ b/dom/interfaces/core/nsIInlineEventHandlers.idl
> >+  [implicit_jscontext] attribute jsval onneedkey;
> 
> I don't think we need this here.
> 
> >+++ b/dom/webidl/EventHandler.webidl
> >+           attribute EventHandler onneedkey;
> 
> Likewise.  This is putting a new event handler attribute on everything in
> sight, whereas the spec just says to put it on HTMLMediaElement.
> 
> >+//EME extensions
> >+[Pref="media.eme.enabled"]
> >+partial interface HTMLMediaElement {
> 
> That [Pref] annotation on a partial interface is a no-op.  We should make
> codegen fail when people do that....
> 
> You want it on all the methods/attributes you're adding.
> 
> >+  readonly attribute MediaKeys? keys;
> 
> This is not nullable in the spec.  That's clearly a spec bug, which you
> should file.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25901


> >+++ b/dom/webidl/HTMLSourceElement.webidl
> >+  attribute DOMString keySystem;
> 
> Needs to be behind the pref.
> 
> >\ No newline at end of file
> 
> Fix, please.
> 
> >+++ b/dom/webidl/MediaError.webidl
> >+  const unsigned short MEDIA_ERR_5 = ENCRYPTED;
> 
> Needs to be behind the pref.  Also, I don't see this in the EME spec.  Am I
> just missing it? 
> https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/C3h1LApSVx8/
> G2s2OV5azvgJ suggests not, and that it's been removed.
> 
> >+++ b/dom/webidl/MediaKeySession.webidl
> >+  readonly attribute Date expiration;
> 
> Like I said above, using Date, especially as an attribute, is an antipattern
> that we shouldn't propagate.  Spec issue needed.
> 
> r=me with those issues addressed, though I'd like to see an interdiff...

I will get an interdiff to you.

I don't fully understand the Date issue, so I haven't filed a spec bug for that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Bug 1016707
> bug 1016709.

To be clear, there is no need to block initial landing on those.  Just flipping of the pref.

> Because we send the integer handle via IPC across to the CDM

Aha!  That makes sense.

> The array is copied when it gets sent over IPC I believe. 

Good.  Actually, the EME spec needs to say exactly when the data gets snapshotted; if it doesn't right now (and I bet it does not), we need a spec issue for that too.

> And I presume that most of these comments below will be redundant if I
> re-implement this via event codegen?

I think so, yes.

> I'm not sure what the issue is here. Could you elaborate please?

Sure.  Let's say my script does this:

  var ev = new MediaKeyMessageEvent("something", {});

What will ev.message be and why?  The spec doesn't actually define what happens when the init dictionary members are missing for these event constructors.

It should probably (assuming that's the desired behavior) define that the initData member of MediaKeyNeededEventInit and destinationURL member of MediaKeyMessageEventInit have a default value of null and that the initDataType member of MediaKeyNeededEventInit has a default value of "".  But there is no default value for the Uint8Array type, so you have to just go ahead and define in prose what happens when the member is missing.

> Is that sufficient specification, or should we file another spec bug here?

We need another spec bug.

> Do you mean that what is in the init data is undefined?

No, I mean what the members of the event should be set to based on the various values that one can pass in the init data is undefined.

> You mean, we should propose that this returns an integer number of
> milliseconds, rather than a Date object?

Yes.

> so I don't understand why using a Date object here is bad.

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22824 and the thread linked from it.  But the short of it is that if you have a Date-valued attribute it will return a new Date object every single time you do the get.  This means that you get things like:

  if (foo.expired == foo.expired) // Always tests false!

Basically, Date as defined in WebIDL right now is an attractive nuisance.  :(

> I just cargo-culted the stuff that made media element's "loadedmetadata" event
> handler work.
...
> Maybe that's a bug too?

The HTML spec explicitly puts those attributes and event handlers on all elements (and on documents and windows).

I'm not super-convinced that's a good thing to do, but in any case that's not what the EME spec is doing.  So in the end either the EME spec needs to align with HTML for this, or we need to implement what the EME spec specifies.

I just realized: this event handler stuff needs to be conditioned on the pref as well.

> How do I make onneedkey only appear on the media element?
...
> I get build errors like so

Hrm.  I think the right thing to do is to not add it to EventNameList at all, and then implement the relevant getter/setter on HTMLMediaElement (cribbing the bits we do on nsINode.h/cpp right now).  And also override IsEventAttributeName on HTMLMediaElement to return true for this atom.

Alternately, we could add a NODE_ONLY_EVENT (or MEDIA_ELEMENT_ONLY) thing in EventNameList and another type of event name (similar to the existing body/frameset stuff).  That's starting to get complicated, though.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and:

> Presumably we can still instantiate MediaKeyMessageEvent from C++ if it's
> implemented this way?

Yes, by just invoking its constructor (with the corresponding dictionary struct).  If we need something simpler, Olli, is there something else?

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Also, you should probably send an intent to implement mail.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/cCCUDt90uWs

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=25902 for the MediaKeySession.expiration Date issue.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > The array is copied when it gets sent over IPC I believe. 
> 
> Good.  Actually, the EME spec needs to say exactly when the data gets
> snapshotted; if it doesn't right now (and I bet it does not), we need a spec
> issue for that too.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25903

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, simpler setup for event construction? I think the new setup where
one initializes the needed properties of the struct and passes it to Constructor method is pretty simple.

I'm not sure codegen supports Uint8Array. We certainly don't have any other event interface using it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I'm not sure codegen supports Uint8Array.

I just tested, and it sort of tries to but doesn't actually succeed.  This is simple enough to fix, and I'm happy to do that once we know what actually needs to be supported.  That is, what the actual behavior the spec wants for the Uint8Array bit is.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and in terms of construction, I hadn't realized we had a version of Constructor that takes an EventTarget instead of a GlobalObject.  That does make things pretty simple.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8429001 [details] [diff] [review]
> Patch v1: EME DOM APIs

> >+++ b/content/html/content/src/HTMLSourceElement.h
> >+  void GetKeySystem(nsString& aKeySystem) const
> 
> So... the spec draft doesn't actually define the behavior of the IDL
> attribute here.  It should specify that it reflects the content attribute,
> right?  Please raise a spec issue.
 
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25911

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 EME DOM APIs

Review comments addressed. I will attach a patch which is the changes I made on top of the previous patch and f? you on that for easier review.

Note: I copied the output of event codegen for MediaKeyNeededEvent and MediaKeyMessageEvent and hacked that to work for my purposes. Hopefully with sensible defaults. Once we get the spec made more explicit, we can switch to using event codegen properly.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: interdiff

Patch from my patch queue on top of previously reviews patch; the changes I made to address review comments.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8431316 [details] [diff] [review]
Patch v2 EME DOM APIs

>+++ b/content/html/content/src/HTMLMediaElement.cpp
>+HTMLMediaElement::GetOnneedkey() {

Curly on next line, please.

>+++ b/content/media/eme/CDMProxy.h
>+  // Helper to enforce that a raw pointer is only be accessed on the main thread.

s/be accessed/accessed/

>+++ b/content/media/eme/MediaKeyMessageEvent.cpp
>+#include "mozilla/dom/MediaKeyMessageEvent.h"

This is included twice.  Drop one of those.

> MediaKeyMessageEvent::~MediaKeyMessageEvent()
>+  mozilla::DropJSObjects(this);

Where's the corresponding HoldJSObjects?  I'd think it should be in the constructor, right?  It looks like there are some calls in Constructor methods, but why there and not in the C++ ctor?

>+MediaKeyMessageEvent::Constructor(const GlobalObject& aGlobal,
>+  nsCOMPtr<mozilla::dom::EventTarget> owner = do_QueryInterface(aGlobal.GetAsSupports());

You shouldn't need the mozilla::dom bit here.

>+    const auto& a = aEventInitDict.mMessage.Value();
>+    e->mMessage = Uint8Array::Create(aGlobal.GetContext(), owner, a.Length(), a.Data());

Is "a" a DOM typed array type that you're trying to clone, basically?

If so, you need to ComputeLengthAndData on it first.

Also, if Create returns null, need to throw on aRv, since we have OOM and a pending exception on the JSContext.  Same for the other Create call in this method.

>+  if (aEventInitDict.mDestinationURL.WasPassed()) {
>+    e->mDestinationURL = aEventInitDict.mDestinationURL.Value();
>+  }

And otherwise it's empty, right?  Just set it to = "" in the dictionary and then you don't need to worry about the !WasPassed() case here.

>+MediaKeyMessageEvent::Message(JSContext* cx)
>+  if (mRawMessage.Length()) {
>+    mMessage = Uint8Array::Create(cx,
>+                                  nullptr,

That should be "this", not nullptr, I'd think, to create in the right compartment.

Also, it looks to me like this function can return null (on OOM from Create, or if mRawMessage.Length() == 0).  In the former case it really needs to communicate an exception out to binding code or at least clear it from the JSContext.  But in the latter case, we'll end up with asserts in debug builds and bad things (like security bugs) in opt builds, since "message" is not declared nullable on this event type.  We need to either make sure we never return null without error or make the attribute nullable.

>+++ b/content/media/eme/MediaKeyMessageEvent.h
>-class MediaKeyMessageEvent MOZ_FINAL : public Event
>+class MediaKeyMessageEvent : public Event

Leave the MOZ_FINAL, please.

>+++ b/content/media/eme/MediaKeyNeededEvent.cpp
> #include "MediaKeyNeededEvent.h"

mozilla/dom/MediaKeyMessageEvent.h

> MediaKeyNeededEvent::~MediaKeyNeededEvent()
>+  mozilla::DropJSObjects(this);

Again, why is the HoldJSObjects() not in the ctor?

>+MediaKeyNeededEvent::Constructor(const GlobalObject& aGlobal,
>+    e->mInitData = Uint8Array::Create(aGlobal.GetContext(), owner, a.Length(), a.Data());

If this returns null, need to throw on aRv.

And also, need ComputeLengthAndData.

The initDataType member in the dictionary here should probably have "" as default value.

>+MediaKeyNeededEvent::GetInitData(JSContext* cx)
>+    mInitData = Uint8Array::Create(cx,

If this returns null, need to throw.

>+++ b/content/media/eme/MediaKeyNeededEvent.h
>-class MediaKeyNeededEvent MOZ_FINAL : public Event

Should still be MOZ_FINAL.

>+++ b/content/media/eme/MediaKeySession.cpp
> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(MediaKeySession,

Why is this bit still needed?  Maybe check with mccr8 to see whether it really is?

> MediaKeySession::Expiration() const
>+  return std::numeric_limits<double>::quiet_NaN();

Or JS::GenericNaN().  Not sure which is clearer in this context...

> MediaKeySession::Update(const Uint8Array& aResponse)
>+      !aResponse.Length()) {

You need a aResponse.ComputeLengthAndData() before that check, no?

>+++ b/content/media/eme/MediaKeys.cpp
>+MediaKeys::RetrievePromise(PromiseId aId)
>   mPromises.Get(aId, getter_AddRefs(promise));
>+  mPromises.Remove(aId);

  mPromises.Remove(aId, getter_AddRefs(promise));

instead of those two lines.

>+MediaKeys::RejectPromise(PromiseId aId, nsresult aExceptionCode)
>+    NS_WARNING("MediaKeys tried to reject  a non-existent promise");

Extra space.

>+++ b/content/media/eme/moz.build
>-UNIFIED_SOURCES += [
>+SOURCES += [

Why this change?

r=me based on the interdiff with the above addressed (though again I'd like to see aa new interdiff).

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

On 6/1/2014 3:35 PM, Bugzilla@Mozilla wrote:
> Boris Zbarsky [:bz] <[e-mail]> has granted Chris Pearce (:cpearce)
> <[e-mail]>'s request for review:
> Bug 1016162: [EME] Encrypted Media Extensions base DOM API
> https://bugzilla.mozilla.org/show_bug.cgi?id=1016162
>

>> +++ b/content/media/eme/MediaKeySession.cpp
>> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(MediaKeySession,
>
> Why is this bit still needed?  Maybe check with mccr8 to see whether it really
> is?

Without this, I get the following linker error:

 0:14.31 LINK : warning LNK4098: defaultlib 'MSVCRTD' conflicts with use of other libs; use /NODEFAULTLIB:library
 0:14.31
 0:14.31 MediaKeySession.obj : error LNK2001: unresolved external symbol "public: virtual void __stdcall mozilla::dom::MediaKeySession::cycleCollection::Trace(v
oid *,struct TraceCallbacks const &,void *)" (?Trace@cycleCollection@MediaKeySession@dom@mozilla@@UAGXPAXABUTraceCallbacks@@0@Z)
 0:14.31
 0:14.31 xul.dll : fatal error LNK1120: 1 unresolved externals

This was because I was using NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED in MediaKeySession.h. If I change to NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED (which other classes that inherit from DOMEventTargetHelper do), then I no longer need the NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED.

`./mach webidl-example MediaKeySession` generated MediaKeySession-example.h with NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED in it.

When should we use NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED instead of NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED? Which should I use here?

Comment 18

[Andrew McCreight (out of office until 8/21) [:mccr8]]

If the class has a field that is a JS pointer that it does not inherit from a parent CCed class (this include inheriting immediately from nsWrapperCache), then it needs to be SCRIPT_HOLDER, so that it can declare a TRACE method, and tell the CC (and GC) about that JS pointer.

From the patch, it looks like MediaKeySession declares no additional JS pointers, and inherits its wrapper cache via DETH, which I assume traces the wrapper cache, so it does not need to be a SCRIPT_HOLDER.

The example generator is probably assuming that you are immediately inheriting from nsWrapperCache.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 EME DOM APIs

Interdiff to follow.

Addressed review comments. I moved the ComputeLengthAndData() calls out from CDMProxy, and define that the caller is responsible for calling them. We assert if we call ComputeLengthAndData() twice, and we sometimes need to use the Uint8Array in the caller, so to be consistent, always require the caller of CDMProxy methods that have Uint8Array methods to call ComputeLengthAndData() before sending it to the CDMProxy.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: review-comments2.patch

Interdiff.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here.  I needed to carefully re-audit all the typed array bits.

The point is that we must guarantee that no JS runs between when ComputeLengthAndData() is called and when we finish using the pointer and length it returns.  I _think_ we're safe here in that the CDMProxy members can't call into JS, right?  It feels very fragile to be doing that bit up front.... :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432966 [details] [diff] [review]
Patch v3 EME DOM APIs

r=me

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://tbpl.mozilla.org/?tree=Try&rev=34d8a89eef18

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b9d289514f

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2b9d289514f

Comment 26

[kevink9876543]

Please add a (non-default) mozconfig option such that if set, the application being built will not contain any support for HTML5 DRM at all.  For the reasons why I would like that change, see bug 1024763.

(Sorry if I've got the wrong bug, but after looking at that linked m-c changeset, sounds like gps suggested I redirect my request from bug 1024763 here.)