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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: cpearce, Assigned: mshal)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments)

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
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.
Attachment #8742166 - Flags: review?(ted)
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.
Flags: needinfo?(mshal)
Assignee

Comment 3

3 years ago
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)?
Assignee: nobody → mshal
Flags: needinfo?(mshal) → needinfo?(cpearce)
(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)?
Flags: needinfo?(cpearce)
Assignee

Comment 5

3 years ago
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.
Flags: needinfo?(cpearce)
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.
Flags: needinfo?(cpearce)
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?
Flags: needinfo?(spohl.mozilla.bugs)
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
Flags: needinfo?(spohl.mozilla.bugs)
mshal: The consensus is that the voucher should go in Content/Resources. Please make it so.
Flags: needinfo?(mshal)
Assignee

Comment 10

3 years ago
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.
Flags: needinfo?(mshal) → needinfo?(cpearce)
That looks good to me.
Flags: needinfo?(cpearce)
Assignee

Comment 15

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

r?ted for the implementation bits.
Attachment #8745999 - Flags: review?(gerv) → review+
Attachment #8746000 - Flags: review?(gerv) → review+
Comment on attachment 8745999 [details]
MozReview Request: Bug 1265272 - Import macholib 1.7; r?ted,gerv

https://reviewboard.mozilla.org/r/49217/#review47049
Attachment #8745999 - Flags: review?(ted) → review+
Comment on attachment 8746000 [details]
MozReview Request: Bug 1265272 - Import altgraph 0.12; r?ted,gerv

https://reviewboard.mozilla.org/r/49219/#review47051
Attachment #8746000 - Flags: review?(ted) → review+
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.
Attachment #8746001 - Flags: review?(ted) → review+
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.
Attachment #8742166 - Flags: review?(ted) → review+
Assignee

Comment 20

3 years ago
Are these things we can or should update in our tree? Or do we need to send the feedback in #c19 to Adobe?
Flags: needinfo?(cpearce)

Comment 21

3 years ago
From the Adobe perspective -- feel free to make the changes. We can also make the changes, but it will take longer.
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.
Flags: needinfo?(cpearce)
Assignee

Comment 25

3 years ago
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.
Flags: needinfo?(cpearce)
It looks like the voucher is on disk as expected, and being found by Firefox. Thanks!
Flags: needinfo?(cpearce)
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.