Closed Bug 1121332 Opened 5 years ago Closed 5 years ago

[EME] implement MediaKeySession.keyStatuses

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 14 obsolete files)

28.48 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
4.11 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
12.13 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
3.53 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
1.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
20.31 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
28.58 KB, patch
Details | Diff | Splinter Review
4.23 KB, patch
Details | Diff | Splinter Review
12.26 KB, patch
Details | Diff | Splinter Review
20.31 KB, patch
Details | Diff | Splinter Review
5.55 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
Details | Diff | Splinter Review
Blocks: 1083658
Assignee: nobody → jwwang
MediaKeyStatuses webidl.

MapClass is not implemented yet in bug 928114. Do we have some other way instead of rolling our own map-like interface. Also I don't have a clear idea how to declare/implement the rest of methods like entries, forEach, keys, values, and @@iterator.

Where can I find some clues?
Attachment #8548666 - Flags: feedback?(cpearce)
That would be a good question to ask bz... He'll need to review your patch anyway; I'm not a DOM peer. ;)

bz: How would we go about implementing a "readonly attribute maplike" in WebIDL, specifically for http://w3c.github.io/encrypted-media/#widl-MediaKeySession-keyStatuses
Blocks: 1032660, EME
Flags: needinfo?(bzbarsky)
Yeah, maplike is a new spec addition that we don't support yet.

What is the timeframe in which you need the maplike bits, and how high-fidelity do you want them to be?

If you want to emulate it using the IDL bits we already have implemented without making any changes at all to the codegen, I think you'd run into the following issues:

1)  Implementing "size" is not so bad, but you wouldn't be able to make it non-enumerable.
2)  Having "entries" be the same function value as "@@iterator" is not possible right
    now.  In fact, having a function called @@iterator is not really possible without some
    codegen changes right now.
3)  keys, values, get, has, clear, set,  you could implement via forwarding to the
    underlying Map (which you'd have to supply yourself).
4)  @@iterator you'd implement by forwarding to the underlying Map or explicit JSAPI for
    CreateMapIterator (which may need to be created).

I'm pretty willing to mentor you through doing codegen work to make all this Just Work if you want.  If you want someone else to do the codegen work, that's where the question about timeframes comes in.

Also note that you will need to store all your stuff in an actual JS Map.  I hope that's not untenable in this case...

That said, I'm not a huge fan of the spec here, because it lets the page control your ability to even store stuff in this map (e.g. by redefining Map.prototype.set).  Is that OK for this particular use case?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #3)
> What is the timeframe in which you need the maplike bits,
As far as I know, the schedule is tight. It could be next release or two. Chris should have a better idea.

> and how high-fidelity do you want them to be?
http://w3c.github.io/encrypted-media/#idl-def-MediaKeyStatusMap
We have to support entries, forEach, get, has, keys, values, @@iterator methods and a size getter.

> If you want to emulate it using the IDL bits we already have implemented
> without making any changes at all to the codegen,
I think I will take this approach as far as the tight schedule is concerned. Can you take a look at my WIP patch to see if I am going in the right direction? Thanks.

> I think you'd run into the following issues:
> 1)  Implementing "size" is not so bad, but you wouldn't be able to make it non-enumerable.
I don't quite understand this issue. Can you elobrate it?

> 2)  In fact, having a function called @@iterator is not really possible without some codegen changes right now.
The entries() method should serve the same purpose. I think our partner can do without the iterator for now.

> Also note that you will need to store all your stuff in an actual JS Map.  I
> hope that's not untenable in this case...
I don't understand this bit. Isn't it just a JS wrapper which access data stored on the native side?
 
> That said, I'm not a huge fan of the spec here, because it lets the page
> control your ability to even store stuff in this map (e.g. by redefining
> Map.prototype.set).  Is that OK for this particular use case?
MediaKeySession.keyStatuses doesn't include mutator methods like set(). I think it should be OK.
Flags: needinfo?(bzbarsky)
Attachment #8548666 - Flags: feedback?(cpearce) → feedback?(bzbarsky)
Flags: needinfo?(cpearce)
> As far as I know, the schedule is tight.

Tight as in "need the maplike bits in a matter of days" or as in "need the maplike bits before 38 goes to Aurora"?

> We have to support entries, forEach, get, has, keys, values, @@iterator methods and a
> size getter.

That doesn't answer my question.  How important are the things I listed as being hard-to-impossible without codegen changes in comment 3?

> I don't quite understand this issue.

Per spec, the "size" of a maplike is not enumerable (in the sense of whether for-in loops see it).  But if you just have a "readonly attribute size" in IDL, that will be enumerable.

> The entries() method should serve the same purpose.

Well, it won't work in a for-of loop.  But OK.  This is why I asked how high-fidelity you needed the implementation!  Sounds like "not very is OK for now", possibly.

You mention a partner.  Are we exposing this only to a restricted set of globals, or to anyone?

> Isn't it just a JS wrapper which access data stored on the native side?

Not per current IDL spec for maplike.  The IDL spec for maplike actually stores everything in a JS Map, and the only thing the IDL does is ensure that the types of the keys and values are correct.

This is back to the question of how closely you need to follow the spec here.

> MediaKeySession.keyStatuses doesn't include mutator methods like set().

OK, the page can also redefine Map.prototype.get.  ;) Of course it can also redefine the MediaKeyStatusMap.prototype.get... the difference mostly matters in terms of Xrays, but maybe Xrays would Xray the Map involved too.

But more importantly, by overriding Map.prototype.set the page can observe it when values are set in the Map backing the maplike.  Which means the specification needs to clearly define exactly when the Map is modified, because that's script-observable and can have side-effects....
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #5)
> > As far as I know, the schedule is tight.
> 
> Tight as in "need the maplike bits in a matter of days" or as in "need the
> maplike bits before 38 goes to Aurora"?

We want to uplift to 37 while its on Aurora. So less than 5 week ideally.


> > We have to support entries, forEach, get, has, keys, values, @@iterator methods and a
> > size getter.
> 
> That doesn't answer my question.  How important are the things I listed as
> being hard-to-impossible without codegen changes in comment 3?

We can get away with supporting a subset of that. We can defer implementing forEach and @@iterator and anything else complicated, provided there is *some way* to iterate over the maplike's contents.

 
> > I don't quite understand this issue.
> 
> Per spec, the "size" of a maplike is not enumerable (in the sense of whether
> for-in loops see it).  But if you just have a "readonly attribute size" in
> IDL, that will be enumerable.

The MediaKeyStatusMap is readonly. Does that address your concern?


> 
> > The entries() method should serve the same purpose.
> 
> Well, it won't work in a for-of loop.  But OK.  This is why I asked how
> high-fidelity you needed the implementation!  Sounds like "not very is OK
> for now", possibly.

Yes. Not perfect for now is OK. Resembling the spec is preferable.

> You mention a partner.  Are we exposing this only to a restricted set of
> globals, or to anyone?

We plan to expose this to a specific partner's site first, and then to the wider web later.
 
> > Isn't it just a JS wrapper which access data stored on the native side?
> 
> Not per current IDL spec for maplike.  The IDL spec for maplike actually
> stores everything in a JS Map, and the only thing the IDL does is ensure
> that the types of the keys and values are correct.
> 
> This is back to the question of how closely you need to follow the spec here.

Based on the spec churn recently, I'm guessing the spec will continue to change in future. We were planning on prefixing or some how versioning the entry point to EME, so that we can expose EME and allow specific partners to test it in the field, without shipping to the wider web as something that is the set-in-stone API that we must support forever.
Flags: needinfo?(cpearce)
(In reply to Boris Zbarsky [:bz] from comment #5)
> Per spec, the "size" of a maplike is not enumerable (in the sense of whether
> for-in loops see it).  But if you just have a "readonly attribute size" in
> IDL, that will be enumerable.

So if I change it to "long size()", it will be not enumerable, right?
(In reply to Chris Pearce (:cpearce) from comment #6)
> The MediaKeyStatusMap is readonly. Does that address your concern?

Does that mean a "readonly" keyword can prevent redefining the MediaKeyStatusMap.prototype.get?
(In reply to JW Wang [:jwwang] from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > The MediaKeyStatusMap is readonly. Does that address your concern?
> 
> Does that mean a "readonly" keyword can prevent redefining the
> MediaKeyStatusMap.prototype.get?

I don't know! I was hoping bz would know. ;)
> So if I change it to "long size()", it will be not enumerable, right?

It will be enumerable.  Gecko's Web IDL codegen right now does not allow creating non-enumerable things.

> Does that mean a "readonly" keyword can prevent redefining the
> MediaKeyStatusMap.prototype.get?

No, it doesn't prevent anything of the sort.
> We want to uplift to 37 while its on Aurora.

OK, I see.  So really, this is "ASAP".

> We can defer implementing forEach and @@iterator and anything else complicated, provided
> there is *some way* to iterate over the maplike's contents.

OK.  Implementing keys, values, and entries should not be too bad if you actually back your storage with a Map, since you just delegate all that to the Map.  If you don't back it with a Map, then you have to do more work, obviously...

> The MediaKeyStatusMap is readonly. Does that address your concern?

It's totally not relevant to the issue I was talking about.  But that issue is a much smaller one than missing @@iterator, so let's just not worry about it for now.

> We plan to expose this to a specific partner's site first, and then to the wider web
> later.

Alright.  That makes it much more OK to deviate from the (unstable, as you noticed) spec at first.

So given all that I think we have two options:

1)  Back the implementation with an ES Map, delegate all the bits that are easy to delegate, then see about adding @@iterator and forEach, which are a bit more work.

2)  Back the implementation with some C++ data structure and create our own iterator implementations.  As long as they implement the ES6 iteration protocol, the right thing will happen when people use them...

I expect #1 to actually be easier.
I decide to take #1. How should I get started? I can't find a way to create an ES Map in C++.
There isn't one.  Needs to be added.  Something similar to JS::NewWeakMapObject, basically, but for Map.
Comment on attachment 8548666 [details] [diff] [review]
1121332_part1_MediaKeySession_keyStatuses_webidl-wip.patch

This seems like a reasonable start.  Get/Has will need to take a BufferSource, not an ArrayBuffer.  And of course you don't want to return nullptr while claiming the return value is never null.  ;)
Attachment #8548666 - Flags: feedback?(bzbarsky) → feedback+
Part 1 - add media key status to gmp-api.
Attachment #8551020 - Flags: review?(cpearce)
Part 2 - expose media key status from CDMCaps.
This is required to implement MediaKeySession.keyStatuses.
Attachment #8548666 - Attachment is obsolete: true
Attachment #8551021 - Flags: review?(cpearce)
Part 3 - export MapObject from JS and implement maplike.
Attachment #8551022 - Flags: review?(bzbarsky)
Part 4 - implement MediaKeySession.keyStatuses and remove MediaKeySession.getUsableKeyIds.
Attachment #8551023 - Flags: review?(cpearce)
Attachment #8551023 - Flags: review?(bzbarsky)
Part 5 - update EME mochitests for webidl changes.
Attachment #8551024 - Flags: review?(cpearce)
Comment on attachment 8551020 [details] [diff] [review]
1121332_part1_add_media_key_status_to_gmp_api-v1.patch

Review of attachment 8551020 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good, but we need an extra enum value to denote that the CDM has removed a key from its usable set.

::: dom/media/eme/CDMCaps.cpp
@@ +97,5 @@
> +  KeyStatus key(aKeyId, aSessionId, aStatus);
> +  auto index = mData.mKeyStatuses.IndexOf(key);
> +
> +  if (index != mData.mKeyStatuses.NoIndex) {
> +    if (mData.mKeyStatuses[index].mStatus == aStatus) {

Remove element if aStatus == kGMPUnknown.

@@ +98,5 @@
> +  auto index = mData.mKeyStatuses.IndexOf(key);
> +
> +  if (index != mData.mKeyStatuses.NoIndex) {
> +    if (mData.mKeyStatuses[index].mStatus == aStatus) {
> +      return false;

You need to return true here if aStatus != mStatus, since the keys' state changed, and we need to dispatch an event to JavaScript.

::: dom/media/gmp/gmp-api/gmp-decryption.h
@@ +78,5 @@
> +enum GMPMediaKeyStatus {
> +  kGMPUsable = 0,
> +  kGMPExpired = 1,
> +  kGMPOutputNotAllowed = 2,
> +  kGMPMediaKeyStatusInvalid = 3 // Must always be last.

So I think we also need a way to mark a previously "known" key as "unknown", so we should also have something like a kGMPRemoved or kGMPUnknown value here too.

https://w3c.github.io/encrypted-media/#known-key

"Keys are considered known even after they become unusable, such as due to expiration. Keys only become unknown when they are explicitly removed from a session.
Note

For example, a key could become unknown if an update() call provides a new license that does not include the key and includes instructions to replace the license(s) that previously contained the key."

I also think that a CDM's implementation of MediaKeySession.remove() and close() should also make all keys on that session as unknown (i.e. remove them from the keysession map), so we need to provide a way for the CDM to explicitly remove a key too.

I filed this issue on the spec to track adding a note that remove() and close() change keys to unknown:
https://github.com/w3c/encrypted-media/issues/18

::: media/gmp-clearkey/0.1/ClearKeySessionManager.cpp
@@ +282,5 @@
>  
>      const string& sessionId = aSession->Id();
> +    mCallback->KeyStatusChanged(&sessionId[0], sessionId.size(),
> +                                &(*it)[0], it->size(),
> +                                kGMPExpired);

The description for "expired" in the EME spec is:

"The key is no longer usable to decrypt media data because its expiration time has passed.
The time represented by the expiration attribute MUST be earlier than the current time. All other keys in the session MUST have this status. "

So I think that's not appropriate here, since we didn't set a time limit on the key. We should instead mark the key as "unknown", which removes it from the key status map.
Attachment #8551020 - Flags: review?(cpearce) → review-
Attachment #8551021 - Flags: review?(cpearce) → review+
Attachment #8551023 - Flags: review?(cpearce) → review+
Comment on attachment 8551024 [details] [diff] [review]
1121332_part5_fix_test_cases-v1.patch

Review of attachment 8551024 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with nit addressed.

::: dom/media/test/test_eme_persistent_sessions.html
@@ -47,5 @@
>  function AwaitAllKeysNotUsable(session, token) {
>    return new Promise(function(resolve, reject) {
>      function listener(event) {
> -      session.getUsableKeyIds().then(function(usableKeyIds) {
> -        if (usableKeyIds.length == 0) {

We should go back to asserting that the previously usable keyIds are removed from the keyStatus' map. Just reverting this hunk should do it.
Attachment #8551024 - Flags: review?(cpearce) → review+
Comment on attachment 8551020 [details] [diff] [review]
1121332_part1_add_media_key_status_to_gmp_api-v1.patch

Review of attachment 8551020 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the quick review.

::: dom/media/eme/CDMCaps.cpp
@@ +98,5 @@
> +  auto index = mData.mKeyStatuses.IndexOf(key);
> +
> +  if (index != mData.mKeyStatuses.NoIndex) {
> +    if (mData.mKeyStatuses[index].mStatus == aStatus) {
> +      return false;

We return false here because mStatus == aStatus above. Did I miss something?

::: dom/media/gmp/gmp-api/gmp-decryption.h
@@ +78,5 @@
> +enum GMPMediaKeyStatus {
> +  kGMPUsable = 0,
> +  kGMPExpired = 1,
> +  kGMPOutputNotAllowed = 2,
> +  kGMPMediaKeyStatusInvalid = 3 // Must always be last.

I will add an enum value "kGMPUnknown" so we can remove the key from the keysession map. Since an unknown key is never visible to the script, there is no need to add an "unknown" state to MediaKeyStatus.
Comment on attachment 8551020 [details] [diff] [review]
1121332_part1_add_media_key_status_to_gmp_api-v1.patch

Review of attachment 8551020 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/eme/CDMCaps.cpp
@@ +98,5 @@
> +  auto index = mData.mKeyStatuses.IndexOf(key);
> +
> +  if (index != mData.mKeyStatuses.NoIndex) {
> +    if (mData.mKeyStatuses[index].mStatus == aStatus) {
> +      return false;

Whoops, no you didn't miss something. Sorry, you're right, forget about this.
Comment on attachment 8551024 [details] [diff] [review]
1121332_part5_fix_test_cases-v1.patch

Review of attachment 8551024 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/test/test_eme_persistent_sessions.html
@@ +38,5 @@
> +        bail(token + " failed to get usableKeyIds");
> +      }
> +      var usableKeyIds = [];
> +      map.forEach(function(val, key) {
> +        if (val == "usable") {

Should we assert |val == "usable"| for all keys? The keys in our test case should never expire, right?

@@ -47,5 @@
>  function AwaitAllKeysNotUsable(session, token) {
>    return new Promise(function(resolve, reject) {
>      function listener(event) {
> -      session.getUsableKeyIds().then(function(usableKeyIds) {
> -        if (usableKeyIds.length == 0) {

Since session.remove() will remove all known keys, map.size should be zero, right?
Comment on attachment 8551024 [details] [diff] [review]
1121332_part5_fix_test_cases-v1.patch

Review of attachment 8551024 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/test/test_eme_playback.html
@@ +26,5 @@
> +    var map;
> +    try {
> +      map = session.keyStatuses;
> +    } catch(e) {
> +      bail("Failed to get keyIds");

Just realize that bail() returns a function object. We should call |bail("Failed to get keyIds")()| instead.
address comment 20.
Attachment #8551020 - Attachment is obsolete: true
Comment on attachment 8551584 [details] [diff] [review]
1121332_part1_add_media_key_status_to_gmp_api-v2.patch

update patch part 1 per comment 20.
interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8551020&action=interdiff&newid=8551584&headers=1
Attachment #8551584 - Flags: review?(cpearce)
fix nits in comment 21.
Attachment #8551024 - Attachment is obsolete: true
Attachment #8551585 - Flags: review+
Attachment #8551584 - Flags: review?(cpearce) → review+
Hi bz,
Can you review my patches of part 3 and part 4? Thanks.
Flags: needinfo?(bzbarsky)
Yes, I'm working on it.  You asked for the reviews on Sunday, Monday was a holiday, and I didn't quite get to them so far today (Tuesday)...
Flags: needinfo?(bzbarsky)
Sorry I didn't notice Monday is a holiday. Please take your time. Thanks.
Comment on attachment 8551022 [details] [diff] [review]
1121332_part3_export_map_object-v1.patch

This is going to need review from a JS peer, but why do you need MapObject::readonlyMethods and createMapLike?  I don't think they should be needed...  You can just use a Map internally for the case of readonly maplikes, no?
Attachment #8551022 - Flags: review?(bzbarsky) → review?(jorendorff)
Comment on attachment 8551023 [details] [diff] [review]
1121332_part4_MediaKeySession.keyStatuses_webidl-v1.patch

No, this is returning a new object every time the getter is called.  What you want to do is to return the same object each time, no?

And that object should be a Web IDL object that has a Map internally that is uses to implement the maplike API...
Attachment #8551023 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8551023 [details] [diff] [review]
> 1121332_part4_MediaKeySession.keyStatuses_webidl-v1.patch
> 
> No, this is returning a new object every time the getter is called.  What
> you want to do is to return the same object each time, no?

Ideally I want to return the same object everytime. However it is just less handy to store the data in a JS map and you have to do the conversion between JS/native types. So I just go cheap as we did in https://hg.mozilla.org/mozilla-central/file/b8259a915b28/dom/webidl/HTMLMediaElement.webidl#l34.

> And that object should be a Web IDL object that has a Map internally that is
> uses to implement the maplike API...

That is right. But it would take much time for me to get the hang of JS and implement it in a correct way. So I just go cheap to get things work for our partner's development and hope JS fellows can implelemnt it someday.
(In reply to Boris Zbarsky [:bz] from comment #32)
> Comment on attachment 8551022 [details] [diff] [review]
> 1121332_part3_export_map_object-v1.patch
> 
> This is going to need review from a JS peer, but why do you need
> MapObject::readonlyMethods and createMapLike?  I don't think they should be
> needed...  You can just use a Map internally for the case of readonly
> maplikes, no?

Yes, that's what I intended to do. But it might take much time for me to get familiar with JS to get things right. So I take the quickest way to implement something like map-like and put a TODO comment for someone can take it in the future.
> However it is just less handy to store the data in a JS map

OK, so there's a question here about how much we want to do this quick-and-dirty and how much we want to try to match a prospective spec.  And a prospective spec would definitely always return the same object.

> But it would take much time for me to get the hang of JS and implement it in a correct
> way.

I'm not sure I see why.  

You need to have a get, has, keys, values, right?  It happens that your map is readonly so you don't need the IDL coercion stuff, so you could just forward those to the stored map via JSAPI just like you added an API for calling the canonical set.

If you need forEach, that would take more work, but the discussion above suggested that given keys/values we don't necessarily need forEach.

Did I misunderstand?  If not, it seems like just doing get/has/keys/values should be fairly straightforward...
(In reply to Boris Zbarsky [:bz] from comment #36)
> OK, so there's a question here about how much we want to do this
> quick-and-dirty and how much we want to try to match a prospective spec. 
> And a prospective spec would definitely always return the same object.

Another way is to clear all map entries and re-populate it everytime. Is that acceptable?

> Did I misunderstand?  If not, it seems like just doing get/has/keys/values
> should be fairly straightforward...

Ok, I will give it a try.
> Another way is to clear all map entries and re-populate it everytime. Is that acceptable?

Hmm.  Clear and repopulate when?  When the set of key statuses changes?
As lazy as MediaKeySession::GetKeyStatuses() is called. We clear the map and repopulate it instead of creating a new MapLike object. Then the code will be almost the same by replacing 'create' with 'clear'.
Why do you need to repopulate it at all?  Is the idea that the set of key statuses can change over time and you want consumers to see the current set?
Yes. https://w3c.github.io/encrypted-media/#widl-MediaKeySession-keyStatuses
The map entries will change upon load() or update() calls on a session.
OK, why not have the load() or update() calls update the map?
The key status data is stored in CDMCaps::mKeyStatuses as native types. I want it to be native types because it is handy to read/write/compare them upon load() or update() calls.

Since MediaKeySession::GetKeyStatuses() is not called as often as load() or update(), I take the approach of lazy synchronization between the map data and the native data stored in CDMCaps::mKeyStatuse.
Sorry, it won't work. The script can cache the result of session.keystatues and use it next time without calling session.keystatues again. So we have to update the map entries upon load() or update() calls everytime.
One other thing.  Presumably the spec defines all this, right?  Because entries in a map/maplike are ordered, so I assume the spec defines the order.
I can't find definition about the key order in the EME spec. I guess we can igore it for now.
implement readonly-maplike as suggested in comment 36.
Attachment #8551022 - Attachment is obsolete: true
Attachment #8551022 - Flags: review?(jorendorff)
Attachment #8553014 - Flags: review?(jorendorff)
Per discussion with bz, return the same object everytime MediaKeySession.keyStatuses is called.
Attachment #8551023 - Attachment is obsolete: true
Attachment #8553015 - Flags: review?(bzbarsky)
update test cases for patch part 3 doesn't fully implement readonly-maplike.
Attachment #8551585 - Attachment is obsolete: true
Attachment #8553016 - Flags: review+
Comment on attachment 8553014 [details] [diff] [review]
1121332_part3_export_map_object-v2.patch

Review of attachment 8553014 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know how WebIDL maplike works in detail, but I don't think this is a correct implementation, since it doesn't enforce the type constraints in the IDL.

I'm not opposed to the JS engine gaining special support for implementing maplike objects, but the implementation work probably needs to go a little deeper than this. For example: this patch defines the map methods anew on every maplike instance, which uses a lot of memory since every method is an object. Instead, you'd probably want them to inherit those methods from a shared prototype object, carried by the GlobalObject somehow.

Clearing the review flag for now. bz and I should talk this over.
Attachment #8553014 - Flags: review?(jorendorff)
Boris, ni?you because I don't know how much support for WebIDL maplike<> we already have or what plans we have; see my previous comment.
Flags: needinfo?(bzbarsky)
> I can't find definition about the key order in the EME spec.

Please file a spec bug!

> but I don't think this is a correct implementation

Agreed.  The "right" quick-and-hacky way to do this is to actually define the MediaKeyStatusMap interface, return an instance of that, and have _that_ object store a Map internally and communicate with it.  This will ensure that the right argument coercions happen.

> I'm not opposed to the JS engine gaining special support for implementing maplike objects

I don't think we need anything from the JS engine here except simple ways to invoke the canonical getter/mutator/etc methods on a Map instance.

The way this should work, both per spec and imo in our implementation, is that there is a Web IDL object that has an API defined in Web IDL.  The implementation of that API (after the IDL argument stuff has already happened) just sets/gets/whatever on a Map the IDL object owns.  I'd like these sets/gets/whatever to use the canonical methods instead of [[Get]] on the Map, because we don't want the page to be able to mess with operations on our internal Map.

Make sense?
Flags: needinfo?(bzbarsky)
Comment on attachment 8553015 [details] [diff] [review]
1121332_part4_MediaKeySession.keyStatuses_webidl-v2.patch

Please see comment 53.
Attachment #8553015 - Flags: review?(bzbarsky) → review-
So here is what to do:

1. remove map-like part from part 3.

2. define a interface MediaKeyStatusMap {
  MediaKeyStatus get(BufferSource key);
  boolean has(BufferSource key);
  object keys(); 
  object values();
  object entries();
}

and implement this interface using the map function exported from part 3.

Right?
(In reply to Boris Zbarsky [:bz] from comment #53)
> I'd like
> these sets/gets/whatever to use the canonical methods instead of [[Get]] on
> the Map, because we don't want the page to be able to mess with operations
> on our internal Map.

What is the different between canonical methods and [[Get]]?
> So here is what to do:

Yes, exactly.

> What is the different between canonical methods and [[Get]]?

Using canonical methods is as if in JS you did this before any page script got run:

  var has = Map.prototype.has;

and then when you want to do has() do:

  has.call(myMap, value);

Using [[Get]] is when you want to do has() you do:

  myMap.has(value);

They're equivalent if the page hasn't done this, say:

  Map.prototype.has = function() { return true; };

but if it has then the canonical method approach will keep working while the other will break, right?
Thanks for the explanation. But how do I do that in native code?

If the page do this:
WeakMap.prototype.get = function(key) { return 0; }

Will it affect the result of GetWeakMapEntry()?
No, it won't.  So the whole point is that you want to add methods like SetMapEntry and ClearMapEntry but also for Has and Get, and use those.
expose map methods from JS APIs.
Attachment #8553014 - Attachment is obsolete: true
Attachment #8554106 - Flags: review?(jorendorff)
implement MediaKeyStatusMap per comment 55.
Attachment #8553015 - Attachment is obsolete: true
Attachment #8554107 - Flags: review?(bzbarsky)
Comment on attachment 8554107 [details] [diff] [review]
1121332_part4_MediaKeySession.keyStatuses_webidl-v3.patch

This is looking much better!

>+++ b/dom/media/eme/MediaKeyStatusMap.cpp
>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap,
>+                                      mParent)

You need to trace mMap as well.  So you need to explode out NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE like so:

  NS_IMPL_CYCLE_COLLECTION_CLASS(MediaKeyStatusMap)
  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeyStatusMap)
    NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)
    NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
    tmp->mMap = nullptr;
  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeyStatusMap)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParent)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(MediaKeyStatusMap)
    NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mMap)
  NS_IMPL_CYCLE_COLLECTION_TRACE_END

>+  , mUpdateError(NS_OK)

Hmm.  So the place where we allocate the MediaKeyStatusMap has an ErrorResult.  Looks like that ErrorResult can in fact be used sanely to ensue that no one looks at the MediaKeySession object (though CreateSession should probably not add the session to mPendingSessions if aRv failed, no?).

Can we not just pass this ErrorResult to the MediaKeyStatusMap ctor so that if we fail to allocate the JS Map we just end up throwing on that ErrorResult?  We'd just have to check aRv and bail if failed before doing the MakePromise() bit in the MediaKeySession constructor.

Furthermore, if we flag MediaKeys::CreateSession as needing a JSContext in Bindings.conf, then we don't even need AutoJSAPI here: we can just pass the JSContext through via CreateSession and the MediaKeySession ctor.  Either way on this part, but I'd really prefer throwing from CreateSession to throwing from later maplike accessors... though I guess we have that anyway if UpdateInternal() fails, so maybe I shouldn't worry about it.

>+MediaKeyStatusMap::Get(JSContext* aCx,

Hmm.  So I think what we have here is a spec bug.  Per spec, this is a maplike taking a BufferSource... but then it's using that as a map key, just like your patch is.  That uses object identity, not the bytes, as the map key, no?  I really doubt that's what the spec intends here.  

Here's a simple pure-JS testcase showing the behavior:

  var map = new Map;
  var bytes = new Uint8Array(2);
  var bytes2 = new Uint8Array(bytes);
  map.set(bytes, 5);
  alert(map.get(bytes));  // alerts 5
  alert(map.get(bytes2)); // alerts undefined

I'm not actually quite sure what the right fix here is; there is no Map key that would match a given byte buffer, so you can't use JS Map at all to implement the behavior I _think_ the spec wants, and the "maplike" IDL in the spec is just bogus.  Or am I just misunderstanding what the spec is trying to do and it really _does_ want object-identity checking on the BufferSource?

I'm sorry it took me so long to notice this problem.  :(

Proceeding with the rest, just in case I did in fact misunderstand the goal.

>+ToJSString(JSContext* aCx, GMPMediaKeyStatus aStatus,
>+  auto str = JS_NewStringCopyN(aCx,

Better to have an explicit JSString* there.

>+  return str && (aResult.setString(str), true);

I think this is a lot more confusing than:

  if (!str) {
    return false;
  }
  aResult.setString(str);
  return true;

>+MediaKeyStatusMap::UpdateInternal(const nsTArray<CDMCaps::KeyStatus>& keys)
>+  if (!ClearMapEntry(cx, map)) {

That should be JS::ClearMapEntries (as in, with the namespace, and the name should make it clear that it clears all the entries; JS::ClearMap would be ok too), right?  How did this compile?  Is someone in this dir "using namespace JS"?  :(

I think in this method you should call jsapi.TakeOwnershipOfErrorReporting() t make sure it just reports any exceptions from updating the Map directly.

The code in this method makes me pretty sure I'm not misunderstanding the spec: it's creating brand-new array buffers to put stuff in the map with, so content will never be able to get anything out of the map via get/has.   We seem to have no tests for those bits.

Maybe we should just drop the get/has for now and leave only the iterator bits, until the spec is sorted out?

r=me modulo all that, but please don't land this without addressing the above.
Attachment #8554107 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #62)
> Hmm.  So the place where we allocate the MediaKeyStatusMap has an
> ErrorResult.  Looks like that ErrorResult can in fact be used sanely to
> ensue that no one looks at the MediaKeySession object (though CreateSession
> should probably not add the session to mPendingSessions if aRv failed, no?).
> 
> Can we not just pass this ErrorResult to the MediaKeyStatusMap ctor so that
> if we fail to allocate the JS Map we just end up throwing on that
> ErrorResult?  We'd just have to check aRv and bail if failed before doing
> the MakePromise() bit in the MediaKeySession constructor.
> 
> Furthermore, if we flag MediaKeys::CreateSession as needing a JSContext in
> Bindings.conf, then we don't even need AutoJSAPI here: we can just pass the
> JSContext through via CreateSession and the MediaKeySession ctor.  Either
> way on this part, but I'd really prefer throwing from CreateSession to
> throwing from later maplike accessors... though I guess we have that anyway
> if UpdateInternal() fails, so maybe I shouldn't worry about it.

Thanks for the review.

Yeah, I like this idea which simplify error handling. Since current patch doesn't make mistakes in error handling, I will file another bug to change code as suggested.
(In reply to Boris Zbarsky [:bz] from comment #62)
> That should be JS::ClearMapEntries (as in, with the namespace, and the name
> should make it clear that it clears all the entries; JS::ClearMap would be
> ok too), right?  How did this compile?  Is someone in this dir "using
> namespace JS"?  :(

It did compile... I guess it is a side effect from unified build. I will add the namespace to make it clear and use the name "JS::ClearMap".
(In reply to Boris Zbarsky [:bz] from comment #62)
> The code in this method makes me pretty sure I'm not misunderstanding the
> spec: it's creating brand-new array buffers to put stuff in the map with, so
> content will never be able to get anything out of the map via get/has.   We
> seem to have no tests for those bits.
> 
> Maybe we should just drop the get/has for now and leave only the iterator
> bits, until the spec is sorted out?

I agree to remove get/has support for now. Is that OK?

I think MediaKeySession.keyStatuses should just be a sequence of [BufferSource,MediaKeyStatus] pairs. However, I will leave it to the spec author to decide. Depending on how this issue is sorted out, the order of map-like keys might not matter anymore.
Flags: needinfo?(cpearce)
(In reply to Boris Zbarsky [:bz] from comment #62)
> You need to trace mMap as well.  So you need to explode out
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE like so:
> 
>   NS_IMPL_CYCLE_COLLECTION_CLASS(MediaKeyStatusMap)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeyStatusMap)
>     NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)
>     NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>     tmp->mMap = nullptr;
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeyStatusMap)
>     NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParent)
>     NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>   NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(MediaKeyStatusMap)
>     NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
>     NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mMap)
>   NS_IMPL_CYCLE_COLLECTION_TRACE_END

Change
NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap, mParentmMap) to
NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap, mParentmMap, mMap)
will do it, right?
(In reply to JW Wang [:jwwang] from comment #66)
> Change
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap, mParentmMap) to
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeyStatusMap, mParentmMap, mMap)
> will do it, right?
No... it fails to compile.
> I agree to remove get/has support for now. Is that OK?

Yes.  But please do make sure there is a spec issue raised!  If for some reason you're unable or unwilling to do so, let me know and I'll do it.

> I think MediaKeySession.keyStatuses should just be a sequence of [BufferSource,MediaKeyStatus] pairs.

OK.  That wouldn't match the API we're adding here, but as long as we're not enabling it for the world and as long as the partner in question is willing to deal with the API changing later, it's ok, I think.  I'd like us to verify the latter assumption.

> No... it fails to compile.

Right, because mMap is a JS thing, not a refcounted thing, so needs to go in the TRACE hook, not the TRAVERSE hook.
(In reply to JW Wang [:jwwang] from comment #65)
> I think MediaKeySession.keyStatuses should just be a sequence of
> [BufferSource,MediaKeyStatus] pairs. However, I will leave it to the spec
> author to decide. Depending on how this issue is sorted out, the order of
> map-like keys might not matter anymore.

So, if I'm understanding the comments here, the reason why we think MediaKeySession.keyStatuses should be a sequence rather than a maplike, is exemplified in bz's "simple pure-JS testcase showing the behavior" in comment 62; that is, the keys being used are objects, so the map must use object identity in its get/has, so those won't work as desired. Assuming I understood that correctly, then yes, I agree a sequence is better.

There is also the spec issue bz pointed out above about the ordering of the maplike (or sequence if we go that path).

Jw: Would you be able to file issues on the EME spec:
File an issue here: https://github.com/w3c/encrypted-media/issues
Corresponding spec now lives at https://w3c.github.io/encrypted-media/

Please file the issue on github, and post the link here. I'd ask you to CC me and bz, but I don't know how to make github do that...

When filing an issues, it's best to be explicit about the change you're suggesting (i.e. include updated text from the spec, with the proposed change, or proposed IDL), along with you reasoning for the change.
Flags: needinfo?(cpearce) → needinfo?(jwwang)
> so the map must use object identity in its get/has

If you want it to act like an ES Map.  You could obviously define an API which acts kinda like a Map but doesn't use object identity there, with separate iterator bits and whatnot; it's just more spec verbiage.
Issus filed:
https://github.com/w3c/encrypted-media/issues/24 <-- maplike key order
https://github.com/w3c/encrypted-media/issues/25 <-- maplike get/set methods

It looks like you have to suscribe notificatios so that you get mails when the bug is updated.
Flags: needinfo?(jwwang)
Jason: Can you review the patch here please? Our partners are blocked on this bug landing. Thanks.
Flags: needinfo?(jorendorff)
Comment on attachment 8554106 [details] [diff] [review]
1121332_part3_export_map_object-v3.patch

Review of attachment 8554106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r=me with a few changes noted below.

::: js/src/builtin/MapObject.cpp
@@ +1386,5 @@
>  {
>      MOZ_ASSERT(MapObject::is(args.thisv()));
>  
>      ValueMap &map = extract(args);
>      ARG0_KEY(cx, args, key);

To avoid adding redundant code, please delete ARG0_KEY and rewrite the body of get_impl() to look like this:

    {
        RootedObject obj(cx, &args.thisv().toObject());
        return get(cx, obj, args.get(0), args.rval());
    }

and the same for the other ones.

@@ +2119,5 @@
> +    return MapObject::create(cx);
> +}
> +
> +JS_PUBLIC_API(uint32_t)
> +JS::GetMapSize(JSContext *cx, HandleObject obj)

As module owner I have to bikeshed a little on the names. Let's make it really blindingly obvious that they're 1-1 to the methods:

JS::MapSize
JS::MapGet
JS::MapSet
JS::MapHas
JS::MapDelete
JS::MapClear
JS::MapKeys
JS::MapValues
JS::MapEntries

After all nobody will be using these functions who isn't already familiar with the corresponding standard JS methods.

JS::NewMapObject is fine. I'm sympathetic to your JS::MapGetSize(), since it's an accessor property, but JS::MapSize() is still better. (I note you also choose MapObject::size(), not getSize().)

I didn't see a MapDelete method here; I think you need it... right?
Comment on attachment 8554106 [details] [diff] [review]
1121332_part3_export_map_object-v3.patch

Review of attachment 8554106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r=me with a few changes noted below.

::: js/src/builtin/MapObject.cpp
@@ +1386,5 @@
>  {
>      MOZ_ASSERT(MapObject::is(args.thisv()));
>  
>      ValueMap &map = extract(args);
>      ARG0_KEY(cx, args, key);

To avoid adding redundant code, please delete ARG0_KEY and rewrite the body of get_impl() to look like this:

    {
        RootedObject obj(cx, &args.thisv().toObject());
        return get(cx, obj, args.get(0), args.rval());
    }

and the same for the other ones.

@@ +2119,5 @@
> +    return MapObject::create(cx);
> +}
> +
> +JS_PUBLIC_API(uint32_t)
> +JS::GetMapSize(JSContext *cx, HandleObject obj)

As module owner I have to bikeshed a little on the names. Let's make it really blindingly obvious that they're 1-1 to the methods:

JS::MapSize
JS::MapGet
JS::MapSet
JS::MapHas
JS::MapDelete
JS::MapClear
JS::MapKeys
JS::MapValues
JS::MapEntries

After all nobody will be using these functions who isn't already familiar with the corresponding standard JS methods.

JS::NewMapObject is fine. I'm sympathetic to your JS::MapGetSize(), since it's an accessor property, but JS::MapSize() is still better. (I note you also choose MapObject::size(), not getSize().)

I didn't see a MapDelete method here; I think you need it... right?
Attachment #8554106 - Flags: review?(jorendorff) → review+
rebase.
Attachment #8551584 - Attachment is obsolete: true
Attachment #8556863 - Flags: review+
rebase.
Attachment #8551021 - Attachment is obsolete: true
Attachment #8556867 - Flags: review+
fix naming.
Attachment #8554106 - Attachment is obsolete: true
Flags: needinfo?(jorendorff)
Attachment #8556869 - Flags: review+
rebase.
Attachment #8554107 - Attachment is obsolete: true
Attachment #8556870 - Flags: review+
fix ok/is usage.
Attachment #8553016 - Attachment is obsolete: true
Attachment #8556873 - Flags: review+
include MediaKeyStatusMap in test_interfaces.html.
Attachment #8556874 - Flags: review?(bzbarsky)
fix build break.
Attachment #8556870 - Attachment is obsolete: true
Attachment #8557010 - Flags: review+
Comment on attachment 8556874 [details] [diff] [review]
1121332_part6_fix_test_interfaces.html.patch

r=me
Attachment #8556874 - Flags: review?(bzbarsky) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #73)

> I didn't see a MapDelete method here; I think you need it... right?
We only need MapClear() for now. We won't implement MapDelete() until it is really needed to save the effort.
Blocks: 1128379
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Patch for beta branch as part of EME platform uplift.
Comment on attachment 8572329 [details] [diff] [review]
Patch 1 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572329 - Flags: approval-mozilla-beta?
Comment on attachment 8572330 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572330 - Flags: approval-mozilla-beta?
Comment on attachment 8572331 [details] [diff] [review]
Patch 3 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572331 - Flags: approval-mozilla-beta?
Comment on attachment 8572332 [details] [diff] [review]
Patch 4 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572332 - Flags: approval-mozilla-beta?
Comment on attachment 8572333 [details] [diff] [review]
Patch 5 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572333 - Flags: approval-mozilla-beta?
Comment on attachment 8572334 [details] [diff] [review]
Patch 6 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572334 - Flags: approval-mozilla-beta?
Comment on attachment 8572329 [details] [diff] [review]
Patch 1 - Beta patch

Approved for Beta as part of EME platform uplift.
Attachment #8572329 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572331 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572332 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572333 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572334 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.