1150437 - [EME] Relicense gmp-clearkey to only depend on Apache2 code

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

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

To make it easier for partners to reuse parts of our code in proprietary plugins, gmp-clearkey should only contain and use/#include Apache 2 licensed code.

Comment 1

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

Attachment: Patch

Relicense MPL to Apache 2 in gmp-clearkey, and remove dependency on MFBT.

Will take the first review I get...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72ce6f4fee6f

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=292694368966

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23bcbd7e470c

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe8838682af

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8587317 [details] [diff] [review]
Patch

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

Define NDEBUG for release builds

Comment 6

[Chris Peterson [:cpeterson]]

We should fix this soon, but we don't need to uplift the license change.

Comment 7

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

(In reply to Edwin Flores [:eflores] [:edwin] from comment #5)
> Comment on attachment 8587317 [details] [diff] [review]
> Patch
> 
> Review of attachment 8587317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Define NDEBUG for release builds

I tested, and we do define NDEBUG in --disable-debug builds and don't in --enable-debug. So we're good as is.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f4f036b611

Comment 9

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

Comment on attachment 8587317 [details] [diff] [review]
Patch

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

::: media/gmp-clearkey/0.1/RefCounted.h
@@ +57,5 @@
> +  T* operator->() const { return mPtr; }
> +
> +private:
> +  void Assign(T* aPtr) {
> +    if (mPtr) {

There's a foot-gun here. If we assign a RefPtr to itself, we'll we'll deref mPtr before we addref it, so we'll end up destroying the thing pointed to if that's the last ref.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8587317 [details] [diff] [review]
Patch

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

::: media/gmp-clearkey/0.1/RefCounted.h
@@ +57,5 @@
> +  T* operator->() const { return mPtr; }
> +
> +private:
> +  void Assign(T* aPtr) {
> +    if (mPtr) {

I noticed and concluded it was safe because Assign() is private and there is no assign operator and RefPtr is not convertible to T*. What call path could result in self-assignment?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38f4f036b611