1265272 - [EME] Update gen-eme-voucher.py to generate voucher for MacOSX binaries

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

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

In order to support Adobe Primetime EME on MacOSX, we need gen-eme-voucher.py [1] to handle generating vouchers for our MacOSX plugin-container binary.

We also need to ensure we run the voucher generation tool on MacOSX, and that the voucher is actually packaged on MacOSX.

See Also bug 1091668 which added the gen-eme-voucher.py for Windows.

[1] http://mxr.mozilla.org/mozilla-central/source/python/eme/gen-eme-voucher.py

Comment 1

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

Attachment: Patch: Update gen-eme-voucher.py to handle MacOSX binaries

Uploading this patch on behalf of Adobe...

Update gen-eme-voucher.py to handle MacOSX binaries.

Note: this requires the macholib package to be installed.

Comment 2

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

Michael: You did the initial work in bug 1091668 to make the gen-eme-voucher.py script generate a voucher on Windows, can you do the same thing here for MacOSX? The MacOSX version uses the macholib package, so we'll need that to ensure we install that too.

Comment 3

[Michael Shal [:mshal]]

Sure, I can take a look. Hopefully it's easier than the Windows one was :).

Do we know for sure that macholib works cross-platform? Or do we need a per-platform version of gen-eme-voucher.py?

Where should the voucher.bin file end up in OSX? $(STAGEPATH)$(MOZ_PKG_DIR)? Or do we need it in under $(_RESPATH)?

Comment 4

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

(In reply to Michael Shal [:mshal] from comment #3)
> Sure, I can take a look. Hopefully it's easier than the Windows one was :).
> 
> Do we know for sure that macholib works cross-platform? Or do we need a
> per-platform version of gen-eme-voucher.py?

I was able to install macholib and run this script on my Windows machine. So I don't think we'll need per-platform versions of this script, but we will need to install macholib on our Windows machines for the script to run, even though it shouldn't hit the macholib path.
 
> Where should the voucher.bin file end up in OSX? $(STAGEPATH)$(MOZ_PKG_DIR)?
> Or do we need it in under $(_RESPATH)?

Gecko expects to find the plugin-container.voucher file in NS_GRE_DIR, which the code says "On OSX, this typically points to Contents/Resources in the app bundle." Ah... so maybe that is $(_RESPATH)?

Comment 5

[Michael Shal [:mshal]]

Are you able to check if this try build puts the voucher in the right place?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0145da0b97f8&selectedJob=19860377

The staged plugin-container actually goes into a somewhat funky location on OSX, which in make-land corresponds to:

$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/$(MOZ_CHILD_PROCESS_NAME).app/Contents/MacOS/

Where STAGEPATH can be "universal" or blank, MOZ_PKG_DIR is "firefox", _BINPATH is something like Nightly.app/Contents/MacOS, and MOZ_CHILD_PROCESS_NAME is plugin_container.

I put the voucher.bin in the same location as the plugin-container binary, which is what we do on Windows. If it turns out we do need the voucher in _RESPATH rather than with the binary, just let me know.

Comment 6

[Stephen A Pohl [:spohl]]

If voucher.bin is a binary file, it should live under Contents/MacOS. Any other files should live under Contents/Resources. Based on comment 5, it appears that this is put in the right place.

Comment 7

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

spohl: the voucher file isn't an executable, it's a binary data file. Should binary data files be in Resources or in Contents/MacOS?

Comment 8

[Stephen A Pohl [:spohl]]

If it's not an executable you're correct that it should live under Contents/Resources. The Apple docs don't call out binary data files specifically, but they do say that Contents/MacOS typically only contains the main executable and sometimes other, standalone executables[1].

I believe both locations would still be accepted by codesign, but since this is a new file we may as well put it in the right place from the start, i.e. Contents/Resources.

[1] https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html#//apple_ref/doc/uid/10000123i-CH101-SW19

Comment 9

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

mshal: The consensus is that the voucher should go in Content/Resources. Please make it so.

Comment 10

[Michael Shal [:mshal]]

Sounds good. Can you test out this build with voucher.bin in Contents/Resources?

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

If that looks good I can publish it for review.

Comment 11

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

That looks good to me.

Comment 12

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1265272 - Import macholib 1.7; r?ted,gerv

Review commit: https://reviewboard.mozilla.org/r/49217/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49217/

Comment 13

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1265272 - Import altgraph 0.12; r?ted,gerv

Review commit: https://reviewboard.mozilla.org/r/49219/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49219/

Comment 14

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1265272 - Generate EME voucher for MacOSX; r?ted

Review commit: https://reviewboard.mozilla.org/r/49221/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49221/

Comment 15

[Michael Shal [:mshal]]

r?gerv for the licensing of macholib and altgraph. Both should be MIT.

r?ted for the implementation bits.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 8745999 [details]
MozReview Request: Bug 1265272 - Import macholib 1.7; r?ted,gerv

https://reviewboard.mozilla.org/r/49217/#review47049

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8746000 [details]
MozReview Request: Bug 1265272 - Import altgraph 0.12; r?ted,gerv

https://reviewboard.mozilla.org/r/49219/#review47051

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8746001 [details]
MozReview Request: Bug 1265272 - Generate EME voucher for MacOSX; r?ted

https://reviewboard.mozilla.org/r/49221/#review47057

Terrible, but not really worse than what's already there.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8742166 [details] [diff] [review]
Patch: Update gen-eme-voucher.py to handle MacOSX binaries

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

Some nit-picky stuff, but as usual I'm not terribly invested in making this code match existing Mozilla standards.

::: python/eme/gen-eme-voucher.py
@@ +168,5 @@
> +FAT_CIGAM = 0xbebafeca
> +FAT_MAGIC =	0xcafebabe
> +
> +LC_SEGMENT = 0x1
> +LC_SEGMENT_64	= 0x19	# 64-bit segment of this file to be

All of these constants are defined in the `macholib.mach_o` module:
```
>>> import macholib.mach_o
>>> hex(macholib.mach_o.MH_MAGIC)
'0xfeedface'
```

Just use those instead of redefining them.

@@ +432,5 @@
> +
> +		arch_digest.setComponentByName('cpuType', CPUType(header.header.cputype))
> +		arch_digest.setComponentByName('cpuSubType', CPUSubType(header.header.cpusubtype))
> +
> +		if header.header.cputype == 0x1000007:

If there isn't a constant defined in macholib somewhere it'd be good to define one for this.

@@ +438,5 @@
> +
> +
> +
> +		segment_commands = list(filter(lambda x: x[0].cmd == lc_segment, header.commands))
> +		text_segment_commands = list(filter(lambda x: x[1].segname.decode("utf-8").startswith("__TEXT"), segment_commands))

I would probably write these as list comprehensions, but I'm not that hung up on it:
```
segment_commands = [x for x in header.commands if x[0].cmd == lc_segment]
```

@@ +454,5 @@
> +			set_of_digest = SetOfCodeSectionDigest()
> +			for section in text_command[2]:
> +				digester = hashlib.sha256()
> +				digester.update(section.section_data)
> +				digest = digester.digest()

You can make this one line: `hashlib.sha256(section.section_data).digest()`

@@ +581,5 @@
> +
> +	dict = processCOFFBinary(stream)
> +
> +	if dict['result'] == False:
> +		dict = processMachoBinary(app_args.input)

It might be nicer if you had an argument to pass in the expected binary type, and default to the right thing depending on sys.platform.

Comment 20

[Michael Shal [:mshal]]

Are these things we can or should update in our tree? Or do we need to send the feedback in #c19 to Adobe?

Comment 21

[Joe Steele]

From the Adobe perspective -- feel free to make the changes. We can also make the changes, but it will take longer.

Comment 22

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

The main benefit of review here is ensuring the script does what we expect it to do. I think we don't need to be too hung up on making it match our coding style. I think we could just land it as is.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb5f44527d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/41433eda78b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/521b33eb32f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e68d0261d7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bb5f44527d1
https://hg.mozilla.org/mozilla-central/rev/41433eda78b3
https://hg.mozilla.org/mozilla-central/rev/521b33eb32f7
https://hg.mozilla.org/mozilla-central/rev/41e68d0261d7

Comment 25

[Michael Shal [:mshal]]

Have you had a chance to see if the voucher is working properly here? It looks like it is present in nightly builds, but I wanted to make sure it was usable.

Comment 26

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

It looks like the voucher is on disk as expected, and being found by Firefox. Thanks!

Comment 27

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1265272 - Mark JSClass as a MOZ_STATIC_CLASS r?jandem

Turns out it's legal to have these at the end as well:

Type Attributes

An attribute specifier list may appear as part of a struct, union or enum
specifier. It may go either immediately after the struct, union or enum
keyword, or after the closing brace. The former syntax is preferred. Where
attribute specifiers follow the closing brace, they are considered to relate
to the structure, union or enumerated type defined, not to any enclosing
declaration the type specifier appears in, and the type defined is not
complete until after the attribute specifiers.

from https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html

Comment 28

[Phabricator Automation]

Comment on attachment 9398421 [details]
Bug 1265272 - Mark JSClass as a MOZ_STATIC_CLASS r?jandem

Revision D208519 was moved to bug 1265271. Setting attachment 9398421 [details] to obsolete.