1223980 - [EME] MediaKeyStatusMap has() and get() aren't exposed via WebIDL

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

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

MediaKeyStatusMap.h has a Has() and Get() which are similar to the maplike functions, but they're not listed in MediaKeyStatusMap.webidl, so they're not available to JS.

Comment 1

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

Attachment: Patch: Add has() and get() to MediaKeyStatusMap.webidl, and test it.

* Add has() and get() to MediaKeyStatusMap.webidl, so that the existing implementation in MediaKeyStatusMap.cpp is exposed to JS and actually works.
* Test that calls to MediaKeyStatusMap.has() and MediaKeyStatusMap.get() work in  test_eme_playback.
* Add calls to MediaKeyStatusMap.keys() and MediaKeyStatusMap.values() to test_eme_playback, and validate that their output corresponds to MediaKeyStatusMap.entries().

Based on my reading of https://github.com/w3c/encrypted-media/issues/75 the intent is for MediaKeyStatusMap to be like a Map. This moves towards that.

The spec has a forEach method, but we should wait until issue 75 is resolved before implementing that I think.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686268 [details] [diff] [review]
Patch: Add has() and get() to MediaKeyStatusMap.webidl, and test it.

I think this implementation fails in exactly the way that just using maplike<> in the IDL would fail.

As I understand the desired behavior in the spec, the key should be a chunk of data, not a JS object.  In other words, doing this:

  for (var [key, val] of map.entries()) {
    // key is an ArrayBuffer here, right?
    key = key.slice();
    ok(map.has(key), "MediaKeyStatusMap.has() should work.");
  }

should work.  But I expect that with this patch it doesn't, since we're using an actual ES Map as the backing store.

What I suspect needs to happen here is the following:

1)  We change out backing store from an ES Map to something keyed on the data instead.  A hashtable would almost work except that I expect we need to provide consistent enumeration order _and_ we need indexed access into our list for the iterator from item 2.  So we may need to think about the data structure a bit.

2)  We kill off our existing entries/keys/values stuff in the IDL and replace it with an iterable<ArrayBuffer, MediaKeyStatus> declaration, and implement the bits that IterableIterator needs.  For this, we need to decide whether we keep returning the same objects as keys or new ones each time; that's something the spec should figure out.  New ones are probably somewhat better because otherwise someone could detach (née neuter) one of our key buffers....

3)  We implement has/get in terms of extracting the data from the incoming buffer and using that as the key.

Comment 3

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

Attachment: WIP Patch 1: Move CopyArrayBufferViewOrArrayBufferData into EMEUtis

WIP patch, part 1. Move helper function to make easier to access.

Comment 4

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

Attachment: WIP Patch 2: Add iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl

OK, I added iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl, and changed the backing store from an ESMap to a simple nsTArray. But I'm getting errors when this code is run.

"TypeError: map.entries(...)[Symbol.iterator] is not a function"

STR:
1. Apply patches, build.
2. Open http://people.mozilla.org/~cpearce/mse-clearkey/
3. Open webconsole, observe "TypeError: map.entries(...)[Symbol.iterator] is not a function" error.

The failing code is my "keystatuseschange" event listener:

function KeysChange(event) {
  var session = event.target;
  log("keystatuseschange event on session" + session.sessionId);
  var map = session.keyStatuses;
  for (var entry of map.entries()) {  //  <-- Exception thrown on this line.
    var keyId = entry[0];
    var status = entry[1];
    var base64KeyId = Base64ToHex(window.btoa(ArrayBufferToString(keyId)));
    log("SessionId=" + session.sessionId + " keyId=" + base64KeyId + " status=" + status);
  }
}

https://github.com/cpearce/mse-eme/blob/master/eme.js#L114

bz: Any idea what's going on here? Am I missing something?

Also, the webidl codegen didn't handle the iterable<> returning an enum, so I had to write code to return the MediaKeyStatus as a string manually in MediaKeyStatusMap::GetValueAtIndex(). Not a big deal, but would be nice if codegen handled this somehow.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> bz: Any idea what's going on here? Am I missing something?

You're not.  This is a bug in the patch for 1085293.  I filed bug 1225392 on this.

> Also, the webidl codegen didn't handle the iterable<> returning an enum

Yeah, there's no ToJSValue implementation for enums...  We could try to add one, now that we have an actual consumer.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8688251 [details] [diff] [review]
WIP Patch 2: Add iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl

This seems generally sane.  Some comments:

1)  I assume mStatuses is generally pretty small, so the linear search on it (as compared to using a hashtable) is ok.  Might be worth documenting explicitly that this is assumed.

2)  Having to copy the key into an nsTArray is rather annoying.  Especially since as far as I can tell this is doing an infallible allocation on page-provided data.  It might be worth being a bit careful but using the provided buffer (in its uint8_t* form) as-is.

3)  I don't think GetKeyAtIndex does the right thing here.  It's returning an nsTArray, which when run through ToJSValue will produce a JS Array, not a typed array or arraybuffer, right?  Should add tests to that effect.  I assume you actually want an ArrayBuffer here.  If so, returning TypedArrayCreator<ArrayBuffer> should do the right thing; it's constructor takes a reference to a "const nsTArray<uint8_t>", which is exactly your situation.

4)  We now have at least two consumers who'd like ToJSValue on enums, so we should just do that.  Filed bug 1225603 on that.

5)  The "Copy chromium and compare keys' bytes." thing presumably needs to be raised to a spec issue if the spec doesn't already require that behavior, right?

Comment 7

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

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8688251 [details] [diff] [review]
> WIP Patch 2: Add iterable<ArrayBuffer,MediaKeyStatus> to
> MediaKeyStatusMap.webidl
> 
> This seems generally sane.  Some comments:

Thanks for your comments, I'll address them. And thanks for patches for the codegen!

> 5)  The "Copy chromium and compare keys' bytes." thing presumably needs to
> be raised to a spec issue if the spec doesn't already require that behavior,
> right?

Relevant spec issues are:

"Define key ID sort order"
https://github.com/w3c/encrypted-media/issues/69

... which is "blocked" on:

"MediaKeyStatusMap: iterable declaration has unexpected behavior; ForEachCallback definition is incorrect"
https://github.com/w3c/encrypted-media/issues/75

Comment 8

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

Attachment: Patch 1 v2: Move CopyArrayBufferViewOrArrayBufferData into EMEUtis

* Add GetArrayBufferViewOrArrayBufferData() to extract pointers to data in a ArrayBufferViewOrArrayBuffer, and define CopyArrayBufferViewOrArrayBufferData() in terms of it. Will use it in the next patch.
* Move GetArrayBufferViewOrArrayBufferData() to EMEUtis.

Comment 9

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

Attachment: Patch 2 v2: Add iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl

Address comments; use TypedArrayCreator<ArrayBuffer> and test that we get an ArrayBuffer as a key when enumerating the MediaKeyStatusMap.

Based on top of the codegen changes patch, this works.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8688766 [details] [diff] [review]
Patch 1 v2: Move CopyArrayBufferViewOrArrayBufferData into EMEUtis

>+  const uint8_t* data = nullptr;
>+  size_t length = 0;

These are unused.

>+#include "mozilla/dom/UnionTypes.h"

Might be better to only include this in the .cpp and forward-declare in the header instead.  UnionTypes.h pulls in all sorts of junk, so including it in headers is suboptimal.

>+CopyArrayBufferViewOrArrayBufferData(const dom::ArrayBufferViewOrArrayBuffer& aBufferOrView,

Need to document the return value.  Can it ever be false in practice?  I guess for a detached arraybuffer it can....  But note that it didn't use to (used to only return false on bogus state of the ArrayBufferViewOrArrayBuffer), so we're changing the semantics of this function.  Was that intentional?

>+// Be careful to only use returned ArrayData inside the lifetime
>+// of the ArrayBufferViewOrArrayBuffer; this does not make a copy!

Oh, the requirement is a LOT more stringent than that.  You have to make sure that you don't invoke any JS anything while an ArrayData is live.  No calls into JS, no calls into JS-implemented webidl or XPIDL, nothing.  Please document this very clearly.

r=me with the above fixed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8688767 [details] [diff] [review]
Patch 2 v2: Add iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl

Now that you don't have to worry about mMap, you could probably just replace all the unlink/traverse/trace/NS_IMPL_CYCLE_COLLECTION_CLASS stuff with:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap, mParaent)

Either way is fine with me.

> MediaKeyStatusMap::MediaKeyStatusMap(JSContext* aCx,

This no longer needs the aCx or aRv arguments, right?

> MediaKeyStatusMap::Get(JSContext* aCx,

This no longer needs aCx, so you can get rid of the implicitJSContext annotation for it in Bindings.conf.  In fact, you can get rid of the entire chunk for MediaKeyStatusMap in Bindings.conf.  Also, looks like you don't need aRv, so can remove the [Throws] from the IDL, right?

> MediaKeyStatusMap::Has(JSContext* aCx,

Again, no need for aCx.

>+    aRv.Throw(NS_ERROR_FAILURE);

This doesn't seem right, given the current definition of IsValid().  Just because someone passed in a zero-length arraybuffer here doesn't mean we should throw, right?  Just returning false from there is fine.  That's assuming that we need this check at all and can't just have it fail all the compares to mKeyId and return false the normal way.  Then we can nix aRv and [Throws].

>+nsString
>+MediaKeyStatusMap::GetValueAtIndex(uint32_t aIndex) const

You should be able to have this return mStatuses[aIndex].mStatus directly, on top of the codegen changes, right?

If those land before this does, please do that.  Otherwise, I'll just make that change in the codegen patch.

> MediaKeyStatusMap::GetSize(JSContext* aCx, ErrorResult& aRv) const

Certainly doesn't need aCx or aRv.  ;)

r=me with those bits fixed.

Comment 12

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

Attachment: Patch 1v3: Move CopyArrayBufferViewOrArrayBufferData into EMEUtis

* Assert in GetArrayBufferViewOrArrayBufferData() that what's passed in is an ArrayBuffer or ArrayuBfferView, i.e. initialized.
* Make CopyArrayBufferViewOrArrayBufferData() infallible, and return an empty array when there's no data in the incoming buffer.
* Handle CopyArrayBufferViewOrArrayBufferData returning an empty array in call sites, returning appropriate exception to JS as required by spec.
* Address other review comments.

Will upload interdiff to make it easier to see changes.

Comment 13

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

Attachment: Patch: Interdiff between Patch 1v2 and Patch 1v3.

Comment 14

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

Attachment: Patch 2v3: Add iterable<ArrayBuffer,MediaKeyStatus> to MediaKeyStatusMap.webidl

Address review comments. It's a much less code now now. :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8688865 [details] [diff] [review]
Patch 1v3: Move CopyArrayBufferViewOrArrayBufferData into EMEUtis

r=me

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efbb3d9ac473
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64de8faad45

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/efbb3d9ac473
https://hg.mozilla.org/mozilla-central/rev/a64de8faad45