Cast License in Uint8Array #974

PaulRosset · 2021-06-22T13:06:57Z

We noticed a bug inside the RxPlayer on the FairPlay DRM environment where the update() EME method was expecting a strict Uint8Array type but sometimes it happens that the application for some reasons provides an ArrayBuffer.

For most of the CDM, it's not a problem, but for FairPlay on Safari, it waits for a strict UInt8Array.

In this PR, we are still allowing the ArrayBuffer as the rx-player doc was explicit about it:

In any case, if a license is provided by this function it should be under a BufferSource type (example: an Uint8Array or an ArrayBuffer). Link

But internally, we cast whatever we receive except null in an Uint8Array.

peaBerberian · 2021-06-22T15:11:41Z

As specified in the EME recommendation, update should take a BufferSource.

A BufferSource can be an ArrayBuffer or an ArrayBufferView (which includes Uint8Array) as specified here.

I think the issue we're having here is linked to the corresponding FairPlay implementation which is either incorrect or rely on another version of the specification. In that regard, are we sure we're not relying on a specific EME implementation in src/compat/eme on the browser where the problem can be reproduced?
(I would prefer to add that workaround only for the problematic environment)

PaulRosset · 2021-07-06T13:01:33Z

I think we could be sure that we are not relying on a specific EME implementation, I saw that the method compat, update, is waiting for an Uint8Array as expected, but the application is giving us an ArrayBuffer.

As u suggested, it could be nicer to only patch that issue only on the env that throw.
Do your think it is better to check the drm system name (reverse key system) or the browser (isSafari) ?
I would suggest checking the keySystem, what do you think?

lfaureyt · 2021-07-07T09:07:09Z

I guess that identifying the key system is a lot more reliable than guessing the browser although that doesn't answer the following question: "is this anomaly specific to the Safari browser or to the Fairplay key system?"

peaBerberian · 2021-07-07T13:44:59Z

is this anomaly specific to the Safari browser or to the Fairplay key system

From a quick check, it seems to have its source in Safari/WebKit:
https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp#L134

To note that it's only for the vendored WebKitMediaKeySession API, and not the MediaKeySession API, which takes a BufferSource, as expected:
https://github.com/WebKit/WebKit/blob/e98ff129bc8eba06ac17105f19c5f0e142aab853/Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp#L395

It seems that on Safari we rely on the former when available due to an issue with requestMediaKeysSystemAccess (which leads to the creation of a WebkitMediaKeys directly, without going through the requestMediaKeySystemAccess API).

So we do seem to rely on a specific EME implementation when the issue is triggered, defined in the RxPlayer in src/compat/eme/custom_media_keys/webkit_media_keys.ts.

What's weird is that we did consider that the license there should be a Uint8Array, so TypeScript is letting us do something we did not want here (still our fault but with a mitigating circumstance :p).

At first I thought it was linked to microsoft/TypeScript#31311 (in short, TypeScript only does structural typing and an Uint8Array has all properties an ArrayBuffer has) but here we're the other way around: we're giving an ArrayBuffer to a function awaiting an Uint8Array.

This should have been catched by TypeScript, so something else is also at play here.

peaBerberian · 2021-07-07T15:06:08Z

src/compat/eme/custom_media_keys/webkit_media_keys.ts

@@ -123,9 +123,12 @@ class WebkitMediaKeySession
          return reject("Unavailable WebKit key session.");
        }
        try {
+          const licenseTypedArray = license instanceof ArrayBuffer


We might want to re-define update as accepting a BufferSource if we're going down this path.

You also named it licenseTypedArray which seems to me a too precize name for not being not enough precize: if we name it like that, not only is it a TypedArray, it is a Uint8Array specifically, not any other one.

Also on the same point, what if the license given was a Uint16Array, or a Float64Array? Those are all valid BufferSource but still not a Uint8Array.

peaBerberian · 2021-07-07T15:09:48Z

src/core/eme/session_events_listener.ts

-        return (castToObservable(getLicense) as Observable<Uint8Array|null>)
-          .pipe(map(license => license !== null ? new Uint8Array(license) : null),
-                getLicenseTimeout >= 0 ? timeout(getLicenseTimeout) :
+        return (castToObservable(getLicense) as Observable<BufferSource|null>)


Why casting is necessary at all here? What is inferred if we do not?

After doing some research, I think Typescript can't infer the type cause of the function castToObservable and his type signature.

The current signature is this:

src/utils/cast_to_observable.ts

function castToObservable<T>(value? : T) : Observable<T>;

So in our case, the typescript infers the type with:
castToObservable<ArrayBufferView | ArrayBuffer | Promise<BufferSource | null> | null>(...)
And so, the same type will be returned while we have to return an Observable<BufferSource | null>

I managed to get it to work by editing the signature but it would mean refactoring elsewhere:

function castToObservable<T, J>(value? : T) : Observable<J>;

castToObservable<ArrayBufferView | ArrayBuffer | Promise<BufferSource | null> | null, ILicense | null>(getLicense)

peaBerberian · 2021-07-07T15:48:02Z

By the way, I always wondered where the bindings of MSE/EME APIs where in browsers (like, I can see where they are implemented on the browser engine-side, but how does the JS engine knows about them and call them, considering that those are part of w3c/WHATWG browser specs, not ECMAScript ones).
So I took a little time to document myself.

After doing some reading (it's very well documented actually!), it seems that it mostly done - at least in WebKit, Firefox and Chrome, through the special-purpose web IDL language.

The idea just seems to add an IDL file named the same way than the header file. Binding functions are then automatically generated by some kind of compiler or script (https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/blink/renderer/bindings/IDLCompiler.md). Those can then be called into by the JS engine (still curious how exactly, but that will be for another time).
So:
https://github.com/WebKit/WebKit/blob/e98ff129bc8eba06ac17105f19c5f0e142aab853/Source/WebCore/Modules/encryptedmedia/MediaKeySession.idl

Will be used to generate bindings to methods in:
https://github.com/WebKit/WebKit/blob/e98ff129bc8eba06ac17105f19c5f0e142aab853/Source/WebCore/Modules/encryptedmedia/MediaKeySession.h

Exact same way for Chromium (https://www.chromium.org/blink/webidl#TOC-File-organization):
IDL file:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/encryptedmedia/media_key_session.idl

header file:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/encryptedmedia/media_key_session.h

I did not look into firefox for now, but it seems they do it the same way: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#adding_webidl_bindings_to_a_class

Found it was interesting enough to share.

peaBerberian · 2021-07-08T10:45:19Z

As we discussed about it @PaulRosset, I post here the rest about what I found on JS engine -> browser APIs interactions.

I specifically searched for Chromium, mainly because I like their online code search tool :p.

V8 <-> blink interactions seems to be performed through a part of blink code called "V8 bindings", whose code is here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/

Inside it, a documentation page was particularly interesting including both the notion of the JS's context and more generally how workers and extensions are each sandboxed: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/V8BindingDesign.md

The directory in which it is contains a lot of code that seems to make it work.

Here begin speculation but I found idl_dictionary_base.h which implies that some files generated from IDL implement a toV8Impl function (we can see that method in some "jinja templates" used by the IDL->C++ generator: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/templates/dictionary_impl.h.tmpl?q=ToV8Impl&ss=chromium%2Fchromium%2Fsrc and https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/templates/dictionary_v8.cc.tmpl?q=ToV8Impl&ss=chromium%2Fchromium%2Fsrc).

ToV8Impl is called by a function called ToV8, also in the V8 bindings, amongst which to_v8_for_core.h.

Continuing deeper in the code, I switched to v8 and found The V8 public C++ API documentation to contain interesting bits. The part about API objects especially.

I stopped here (for now!).

peaBerberian · 2021-07-13T13:22:13Z

src/compat/eme/custom_media_keys/webkit_media_keys.ts

+            ? new Uint8Array(license)
+            : license instanceof Uint8Array
+              ? license
+              : new Uint8Array((license as ArrayBufferView).buffer);


Weird indent.

I guess that's OK, but I generally find it more readable that way:

const uInt8Arraylicense = license instanceof ArrayBuffer ? new Uint8Array(licence) : license instanceof Uint8Array ? license : new Uint8Array( (license as ArrayBufferView).buffer );

Also, casting as ArrayBufferView should not be needed. Why is that the case? If it is, it means that we forgot to handle other cases (as TypeScript should be smart enough to notice that this cannot be an ArrayBuffer)

Ho yes, thanks for the indent suggestion, since I'm using prettier, I was more used to write it like this, but it's totally fair that since we don't use prettier in this project we should use another indent style.

And yes, I forgot to edit the update method signature by this:

this.update = (license: Int8Array | ArrayBuffer) => {...}

So Typescript was not understanding cause we already checked that the license was a UInt8Array

peaBerberian · 2021-08-13T12:54:15Z

src/compat/eme/custom_media_keys/webkit_media_keys.ts

@@ -114,7 +114,7 @@ class WebkitMediaKeySession
    this.keyStatuses = new Map();
    this.expiration = NaN;

-    this.update = (license: Uint8Array) => {
+    this.update = (license: Int8Array | ArrayBuffer) => {


⚠️ Int8Array != Uint8Array
One contains signed integers the other not

Also, I'm under the impression that the current implementation allows every ArrayBuffer + ArrayBufferView so every BufferSource, no need to be picky here.

ho yes, VSCode autocompletion got me here :(

but yes, Typescript seems to be lost, because he doesn't know the type anymore after checking:

license instanceof Uint8Array

Actually, its true, what could be the type if the method waits for a Uint8Array but we check for it and it's not?

peaBerberian · 2021-08-24T08:26:15Z

The linter is not happy right now due to trailing spaces and 95 columns being reached.
Can you fix that?

peaBerberian · 2021-08-26T14:57:45Z

src/compat/eme/custom_media_keys/webkit_media_keys.ts

@@ -123,9 +123,15 @@ class WebkitMediaKeySession
          return reject("Unavailable WebKit key session.");
        }
        try {
+          const uInt8Arraylicense = license instanceof ArrayBuffer ?
+                                    new Uint8Array(license) :


Weird indentation now, and the nested ternary becomes just slightly less readable for it to be worth it.

Why not using regular readable conditions, indented at the start of the line, and be done with it?

implementation of EME update method is waiting for a uInt8Array, so it means that if we would pass an other type like an ArrayBuffer, webkit implementation would throw because he is waiting a strict uIn8Array (ttps://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp#L134). So, we are basically casting everything we receive into a uInt8Array to pass it to the CDM then.

peaBerberian added this to the 3.26.1 milestone Jun 23, 2021

peaBerberian added the DRM Relative to DRM (EncryptedMediaExtensions) label Jul 2, 2021

peaBerberian reviewed Jul 7, 2021

View reviewed changes

peaBerberian reviewed Jul 13, 2021

View reviewed changes

peaBerberian added bug This is an RxPlayer issue (unexpected result when comparing to the API) Compatibility Relative to the RxPlayer's compatibility with various devices and environments labels Aug 3, 2021

peaBerberian reviewed Aug 13, 2021

View reviewed changes

peaBerberian mentioned this pull request Aug 25, 2021

Release/v3.26.1 #996

Merged

17 tasks

peaBerberian reviewed Aug 26, 2021

View reviewed changes

PaulRosset force-pushed the fix/u8IntArray-license-casting branch 3 times, most recently from 7cb2c7a to 7f1410b Compare August 27, 2021 15:17

PaulRosset force-pushed the fix/u8IntArray-license-casting branch from 7f1410b to 5cb9aea Compare August 27, 2021 15:47

peaBerberian approved these changes Sep 1, 2021

View reviewed changes

peaBerberian merged commit a957bb0 into master Sep 1, 2021

peaBerberian deleted the fix/u8IntArray-license-casting branch August 22, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cast License in Uint8Array #974

Cast License in Uint8Array #974

PaulRosset commented Jun 22, 2021

peaBerberian commented Jun 22, 2021 •

edited

Loading

PaulRosset commented Jul 6, 2021

lfaureyt commented Jul 7, 2021

peaBerberian commented Jul 7, 2021

peaBerberian Jul 7, 2021 •

edited

Loading

peaBerberian Jul 7, 2021

PaulRosset Jul 9, 2021

peaBerberian commented Jul 7, 2021 •

edited

Loading

peaBerberian commented Jul 8, 2021

peaBerberian Jul 13, 2021 •

edited

Loading

PaulRosset Aug 13, 2021

peaBerberian Aug 13, 2021

PaulRosset Aug 13, 2021

peaBerberian commented Aug 24, 2021

peaBerberian Aug 26, 2021 •

edited

Loading

Cast License in Uint8Array #974

Cast License in Uint8Array #974

Conversation

PaulRosset commented Jun 22, 2021

peaBerberian commented Jun 22, 2021 • edited Loading

PaulRosset commented Jul 6, 2021

lfaureyt commented Jul 7, 2021

peaBerberian commented Jul 7, 2021

peaBerberian Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

peaBerberian Jul 7, 2021

Choose a reason for hiding this comment

PaulRosset Jul 9, 2021

Choose a reason for hiding this comment

peaBerberian commented Jul 7, 2021 • edited Loading

peaBerberian commented Jul 8, 2021

peaBerberian Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

PaulRosset Aug 13, 2021

Choose a reason for hiding this comment

peaBerberian Aug 13, 2021

Choose a reason for hiding this comment

PaulRosset Aug 13, 2021

Choose a reason for hiding this comment

peaBerberian commented Aug 24, 2021

peaBerberian Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

peaBerberian commented Jun 22, 2021 •

edited

Loading

peaBerberian Jul 7, 2021 •

edited

Loading

peaBerberian commented Jul 7, 2021 •

edited

Loading

peaBerberian Jul 13, 2021 •

edited

Loading

peaBerberian Aug 26, 2021 •

edited

Loading