1100450 - GMP Plugin should be packaged under Contents/MacOS, not Contents/Resources

Status: NEW

(Firefox Build System :: General, defect)

Description

[Stephen A Pohl [:spohl]]

New dynamic libraries should not be placed under Contents/Resources, as this may break Apple's v2 signatures on the .app bundle in the future. Bug 1075182 introduced Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib, which should be placed under Contents/MacOS. The client will most likely need to be updated to look for this library under Contents/MacOS as well. NS_GRE_BIN_DIR can be used to retrieve the proper directory in Firefox.

The general rule is:
NS_GRE_BIN_DIR - Contents/MacOS     - compiled code only
NS_GRE_DIR     - Contents/Resources - non-compiled code only

All existing dynamic libraries are currently being moved out of Contents/Resources, and new ones should be placed under Contents/MacOS.

Comment 1

[Stephen A Pohl [:spohl]]

Edwin, will you be able to tackle this? I'm happy to move the dylib myself, but I'm pretty sure that this will break the client (since it can't find the dylib anymore).

Comment 2

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

Sure.

Comment 3

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

Attachment: bug1100450.patch

Comment 4

[Stephen A Pohl [:spohl]]

Comment on attachment 8525039 [details] [diff] [review]
bug1100450.patch

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

Unfortunately, this won't work. In order for us to be able to sign our bundles with Apple's v2 signatures, the dylib has to live under Contents/MacOS directly, without a subdirectory or .info file next to it. Subdirectories under Contents/MacOS are treated as app bundles, which 'gmp-clearkey' obviously isn't and .info files aren't compiled code, so have to go in Contents/Resources. Subdirectories are fine under Contents/Resources, if needed.

::: browser/app/macbuild/Contents/MacOS-files.in
@@ +6,1 @@
>  /gtest/***

It may seem at first that /gtest/*** is being packaged into our .app bundles, hence /gmp-clearkey/*** should be fine as well. However, /gtest/*** is only being included in the .app bundle on unpackaged and unsigned builds in order to run our gtests. Once the build is packaged, the gtest directory is stripped and not included in the final bundle (see the omission of the gtest directory in package-manifest.in). Since gmp-clearkey is supposed to ship as part of the packaged and signed bundle, we can't use this trick here.

::: browser/installer/package-manifest.in
@@ +929,5 @@
>  
>  ; media
>  #ifdef MOZ_EME
> +#ifdef XP_MACOSX
> +@APPNAME@/Contents/MacOS/gmp-clearkey/0.1/@DLL_PREFIX@clearkey@DLL_SUFFIX@

- As of the landing of bug 1096494 today, please use @BINPATH@ (instead of @APPNAME@/Contents/MacOS) to refer to Contents/MacOS and @RESPATH@ to refer to Contents/Resources.
- Subdirectories aren't permissible under Contents/MacOS, unless it's an app bundle.

@@ +930,5 @@
>  ; media
>  #ifdef MOZ_EME
> +#ifdef XP_MACOSX
> +@APPNAME@/Contents/MacOS/gmp-clearkey/0.1/@DLL_PREFIX@clearkey@DLL_SUFFIX@
> +@APPNAME@/Contents/MacOS/gmp-clearkey/0.1/clearkey.info

- clearkey.info needs to go in Contents/Resources.
- Subdirectories are permissible under Contents/Resources, if needed.

Comment 5

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

(In reply to Stephen Pohl [:spohl] from comment #4)
> Unfortunately, this won't work. In order for us to be able to sign our
> bundles with Apple's v2 signatures, the dylib has to live under
> Contents/MacOS directly, without a subdirectory or .info file next to it.
> Subdirectories under Contents/MacOS are treated as app bundles, which
> 'gmp-clearkey' obviously isn't and .info files aren't compiled code, so have
> to go in Contents/Resources. Subdirectories are fine under
> Contents/Resources, if needed.

We require a specific directory structure for GMPs: gmp-[name]/[version]/, where both [name] and [version] are arbitrary text.

Would it be kosher to add a symlink from Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib back to Contents/MacOS/libclearkey.dylib?

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #5)
> Would it be kosher to add a symlink from
> Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib back to
> Contents/MacOS/libclearkey.dylib?

Unfortunately, we don't support symlinks in packaged builds. I forget what the technical reasons were, but I discussed this in #releng when I was trying to reduce the number of duplicates in our app bundles and was told that symlinks are being resolved during packaging. In other words, Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib would no longer be a symlink, but a full copy of the dylib.

Comment 7

[Stephen A Pohl [:spohl]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #5)
> We require a specific directory structure for GMPs: gmp-[name]/[version]/,
> where both [name] and [version] are arbitrary text.

Does this structure need to be represented by a directory structure, or would it be enough to incorporate this structure in the dylib's filename? The [name] part is already there, so we'd only need to add the [version] part. For example "libclearkey-0.1.dylib"?

Comment 8

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

(In reply to Stephen Pohl [:spohl] from comment #7)
> Does this structure need to be represented by a directory structure, or
> would it be enough to incorporate this structure in the dylib's filename?
> The [name] part is already there, so we'd only need to add the [version]
> part. For example "libclearkey-0.1.dylib"?

It has to be represented by the directory structure; further, the .dylib and .info should be in the same directory.

Is there any technical reason we can't have the .dylib in Resources/, or is it just that it's a Bad Idea™?

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> (In reply to Stephen Pohl [:spohl] from comment #7)
> > Does this structure need to be represented by a directory structure, or
> > would it be enough to incorporate this structure in the dylib's filename?
> > The [name] part is already there, so we'd only need to add the [version]
> > part. For example "libclearkey-0.1.dylib"?
> 
> It has to be represented by the directory structure; further, the .dylib and
> .info should be in the same directory.

Can you elaborate why these restrictions are in place?

> Is there any technical reason we can't have the .dylib in Resources/, or is
> it just that it's a Bad Idea™?

Currently, we're lucky that this is still possible or we wouldn't be able to sign with v2 signatures. However, it is pretty clear from Apple's documentation that they expect a proper bundle structure and they may strengthen the enforcement of their rules at any time. This could break GMP, trigger prompts to the end-user that the app can't be launched (because it doesn't have a valid signature or bundle structure) or several other things.

I'm planning to move all the remaining compiled code from Contents/Resources to the proper place to make us future proof against this. My hope is that new features, such as GMP, can be designed in such a way that it doesn't break the recommended bundle structure.

Fire drills like the v2 signing changes aren't fun. Let's avoid them when we can.

Comment 10

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

(In reply to Stephen Pohl [:spohl] from comment #9)
> (In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> > (In reply to Stephen Pohl [:spohl] from comment #7)
> > > Does this structure need to be represented by a directory structure, or
> > > would it be enough to incorporate this structure in the dylib's filename?
> > > The [name] part is already there, so we'd only need to add the [version]
> > > part. For example "libclearkey-0.1.dylib"?
> > 
> > It has to be represented by the directory structure; further, the .dylib and
> > .info should be in the same directory.
> 
> Can you elaborate why these restrictions are in place?

It's basically because we need a structured way to find the metadata associated with a shared library, and we may need to have multiple copies of the same shared library with different versions on disk at once. It's an arbitrary convention.

Why doesn't this affect OpenH264 as well? We use the exact same layout as OpenH264?

Comment 11

[Stephen A Pohl [:spohl]]

(In reply to Chris Pearce (:cpearce) from comment #10)
> (In reply to Stephen Pohl [:spohl] from comment #9)
> > Can you elaborate why these restrictions are in place?
> 
> It's basically because we need a structured way to find the metadata
> associated with a shared library, and we may need to have multiple copies of
> the same shared library with different versions on disk at once. It's an
> arbitrary convention.

Wouldn't a versioned filename do the trick though? It seems like it'd be straightforward to infer the path to a dylib's metadata based on its filename. What am I missing?

> Why doesn't this affect OpenH264 as well? We use the exact same layout as
> OpenH264?

Could you point me to the OpenH264 dylib in the current nightly? I can't seem to find it...

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Stephen Pohl [:spohl] from comment #11)
> (In reply to Chris Pearce (:cpearce) from comment #10)
> > Why doesn't this affect OpenH264 as well? We use the exact same layout as
> > OpenH264?
> 
> Could you point me to the OpenH264 dylib in the current nightly? I can't
> seem to find it...

Also on IRC last night: OpenH264 lives in the profile, and is downloaded after the browser starts, so it's not part of our executable package.

Comment 13

[Stephen A Pohl [:spohl]]

[Tracking Requested - why for this release]:
Bug 1075182 introduced a new dylib under Contents/Resources. Bug 1075182 is tracking for FF36, so I'm setting the tracking flag for Firefox 36. I'm not sure how strongly we want to enforce not having new dylibs being placed under Contents/Resources, so I'll let someone else make the call. As mentioned in previous comments, there is nothing that breaks *at the moment* by having this dylib under Contents/Resources. But since there's an effort to make our .app bundles more Apple compliant, and that all dylibs are being moved out of Contents/Resources, it seems to make sense that new features abide by this rule as well. Apple may choose to enforce stricter rules at any time. The v2 signing (bug 1047584) was one example for that, which caused a decent fire drill to get it fixed for FF 34.

I understand that the gmp-clearkey plugin can't be installed anywhere as a simple binary. Its directory structure is defined by a standard (more or less) -- https://wiki.mozilla.org/GeckoMediaPlugins. And though it might be changed, it can't be eliminated entirely. But this means that, if Apple's rules about signatures evolve as we expect, no GMP plugin will be able to live anywhere in the Firefox bundle. So any Gecko Media Plugins will ultimately need to be moved somewhere else -- for example to the profile.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Tracking because the recent Apple changes caused a lot of pain for 32 & 33...

Comment 15

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

We discussed this in Portland. On MacOSX We could package gmp-clearkey into a zip file, and ship that. On first run, we can unzip the plugin into some "sensible" place.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Chris, are you planning this for 36?
Thanks

Comment 17

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

No. This is unrelated to MSE, which is targeted at 36.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

ok, not tracking anymore then!

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

This seems to have stalled. I don't think this is pressing for 36 or 37 but agree with the sentiment that we want to fix this before we have to.

Edwin - Can you pick this back up?

Comment 20

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

Sure. Should have time this week or next.

Comment 21

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

Stephen: You're working on the new GMP loading stuff. Is there any particular way to do this that would play nice with that? It seems a bit hacky to zip and unzip this just to play these signing games.

Last I heard, we were going to have EME GMPs packaged in XPIs which would have worked well here, but IIUC we're not going that way anymore?

Comment 22

[Stephen A Pohl [:spohl]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #21)
> Stephen: You're working on the new GMP loading stuff. Is there any
> particular way to do this that would play nice with that? It seems a bit
> hacky to zip and unzip this just to play these signing games.

We could make this just another downloadable GMP, since we'll have the ability to download any number of GMPs advertised via our XML file (see bug 1089867). But Chris Pearce made me aware of the fact that Linux prefers having this bundled. I don't have a perfect answer, but I could see one of these two solutions:
1. Package as a zip, unzip at runtime. Hacky, but should be fairly straightforward to do.
2. Make this a downloadable GMP for OSX only. This would require hosting the ClearKey CDM on our server and updating the XML file for OSX. However, we may not like the fact that OSX diverges from other platforms so much.

All things considered, I'm still leaning towards zipping it up for OSX.

> Last I heard, we were going to have EME GMPs packaged in XPIs which would
> have worked well here, but IIUC we're not going that way anymore?

I've never heard of this, but I haven't been on the GMP project for very long. I don't know if this would be a solution here.