firefox: warn users that drmSupport=false argument does nothing

[ghost]

Description of changes

When nixpkgs drmSupport?true option is set to false, it causes firefox to compile with --disable-eme. This flag has been silently ignored by Firefox since September of 2016.

We should not mislead users into thinking that it is still possible to turn off EME at compile time.

Since firefox no longer provides any way for nixpkgs to implement drmSupport==false, this commit asserts drmSupport==true and prints an explanation of the situation if that assertion fails. I recommend we leave this assertion for one release cycle to warn anybody who might have been using it, and then drop the option in the following release.

I only noticed because some recent change caused --disable-eme on aarch64 to cause a build failure (shown below).

 0:11.70(B Traceback (most recent call last):(B
 0:11.70(B   File "/build/firefox-91.8.0/configure.py", line 226, in <module>(B
 0:11.71(B     sys.exit(main(sys.argv))(B
 0:11.71(B   File "/build/firefox-91.8.0/configure.py", line 50, in main(B
 0:11.71(B     sandbox.run(os.path.join(os.path.dirname(__file__), "moz.configure"))(B
 0:11.71(B   File "/build/firefox-91.8.0/python/mozbuild/mozbuild/configure/__init__.py", line 507, in run(B
 0:11.71(B     self._value_for(option)(B
 0:11.71(B   File "/build/firefox-91.8.0/python/mozbuild/mozbuild/configure/__init__.py", line 612, in _value_for(B
 0:11.71(B     return self._value_for_option(obj)(B
 0:11.71(B   File "/build/firefox-91.8.0/python/mozbuild/mozbuild/util.py", line 1050, in method_call(B
 0:11.72(B     cache[args] = self.func(instance, *args)(B
 0:11.72(B   File "/build/firefox-91.8.0/python/mozbuild/mozbuild/configure/__init__.py", line 679, in _value_for_option(B
 0:11.72(B     raise InvalidOptionError((B
 0:11.72(B mozbuild.configure.options.InvalidOptionError: --disable-eme is not available in this configuration(B
Error running mach:

    ['configure', '--prefix=/nix/store/4s2hn4hnm96frmdh6bd6qzxsphvsdf1y-firefox-esr-unwrapped-91.8.0esr', '--disable-tests', '--disable-updater', '--enable-application=browser', '--enable-default-toolkit=cairo-gtk3-wayland', '--enable-system-pixman', '--with-libclang-path=/nix/store/cggszyf7mjlzrp9vxrlffvdr9nqjjd13-clang-14.0.1-lib/lib', '--with-system-ffi', '--with-system-icu', '--disable-dbus', '--disable-accessibility', '--with-system-jpeg', '--with-system-libevent', '--with-system-libvpx', '--with-system-nspr', '--with-system-nss', '--with-system-png', '--with-system-webp', '--with-system-zlib', '--enable-lto=cross', '--enable-linker=lld', '--enable-alsa', '--disable-pulseaudio', '--enable-ffmpeg', '--enable-jemalloc', '--disable-necko-wifi', '--enable-negotiateauth', '--disable-webrtc', '--disable-crashreporter', '--disable-eme', '--disable-debug', '--enable-optimize', '--enable-release', '--enable-debug-symbols', '--disable-strip', '--disable-install-strip', '--enable-official-branding']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file configure| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Process executed with non-0 exit code 1: ['/nix/store/s6lwpx258l9ypq8phghzdhrnidnfgjk3-python3-3.9.12/bin/python3', '/build/firefox-91.8.0/configure.py', '--prefix=/nix/store/4s2hn4hnm96frmdh6bd6qzxsphvsdf1y-firefox-esr-unwrapped-91.8.0esr', '--disable-tests', '--disable-updater', '--enable-application=browser', '--enable-default-toolkit=cairo-gtk3-wayland', '--enable-system-pixman', '--with-libclang-path=/nix/store/cggszyf7mjlzrp9vxrlffvdr9nqjjd13-clang-14.0.1-lib/lib', '--with-system-ffi', '--with-system-icu', '--disable-dbus', '--disable-accessibility', '--with-system-jpeg', '--with-system-libevent', '--with-system-libvpx', '--with-system-nspr', '--with-system-nss', '--with-system-png', '--with-system-webp', '--with-system-zlib', '--enable-lto=cross', '--enable-linker=lld', '--enable-alsa', '--disable-pulseaudio', '--enable-ffmpeg', '--enable-jemalloc', '--disable-necko-wifi', '--enable-negotiateauth', '--disable-webrtc', '--disable-crashreporter', '--disable-eme', '--disable-debug', '--enable-optimize', '--enable-release', '--enable-debug-symbols', '--disable-strip', '--disable-install-strip', '--enable-official-branding']

  File "/build/firefox-91.8.0/python/mozbuild/mozbuild/build_commands.py", line 185, in configure
    return driver.configure(
  File "/build/firefox-91.8.0/python/mozbuild/mozbuild/controller/building.py", line 1528, in configure
    status = self._run_command_in_objdir(
  File "/build/firefox-91.8.0/python/mozbuild/mozbuild/base.py", line 845, in _run_command_in_objdir
    return self.run_process(cwd=self.topobjdir, **args)
  File "/build/firefox-91.8.0/python/mach/mach/mixin/process.py", line 176, in run_process
    raise Exception(
note: keeping build directory '/tmp/nix-build-firefox-esr-unwrapped-91.8.0esr.drv-1'

Shame on Mozilla for silently ignoring this flag instead of, at minimum, warning users that they have decided to stop responding to it.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[mweinelt]

Hm, I'm a bit surprised, as that would mean I misunderstood the config options. I thought that if they offer an --enable-eme option, then you could also invert that to --disable-eme. But I guess that depends on which config option system the flag is using.

mozilla/gecko-dev@3635c71

[mweinelt]

I think the problem is that EME is just not supported on aarch64 on Linux, and that's why setting the option at all fails for you.

https://github.com/mozilla/gecko-dev/blob/master/toolkit/moz.configure#L767-L778

https://github.com/mozilla/gecko-dev/blob/master/toolkit/moz.configure#L795

[mweinelt]

I can build w and w/o drmSupport on x86_64-linux, so this is just a target specific limitation that has existed for a long time.

See e.g. https://bugs.gentoo.org/show_bug.cgi?id=684674.

[K900]

The message feels a bit too wordy to me. Maybe just keep the last part about the policy file?

[ghost]

I think the problem is that EME is just not supported on aarch64 on Linux, and that's why setting the option at all fails for you.

I can build w and w/o drmSupport on x86_64-linux, so this is just a target specific limitation that has existed for a long time.

Let's set aarch64 aside for a second, I regret mentioning it. I only brought that up because it's how I noticed this problem in the first place.

[mweinelt]

What's left here when we exclude the aarch64 problem?

I reject your assertion message based on testing done here and in #166078 on x86_64-linux.

Also shaming someone publicly should be done with great care.

[ghost]

I can build w and w/o drmSupport on x86_64-linux

Of course. But you still got libclearkey.so in the closure, didn't you?

[mweinelt]

I can build w and w/o drmSupport on x86_64-linux

Of course. But you still got libclearkey.so in the closure, didn't you?

On mobile currently, so can't check, but I understand the flag only sets the default runtime configuration these days, which can be overridden again from within about: config.

[ghost]

I understand the flag only sets the default runtime configuration these days, which can be overridden again from within about: config.

This is the whole problem.

drmSupport/--disable-eme is a compile-time setting. When it was originally introduced, setting it to false gave you a firefox which lacked the ability to download DRM binary blobs.

Mozilla then removed the ability to do that.

Instead of alerting users with an error message indicating that they had removed this ability, they silently redefined what it means. I absolutely think that this is shameful and am not afraid to say it. However in a certain sense the commit messages in the nixpkgs repo speak for all the contributors, and I shouldn't ascribe my own take on the situation to the other contributors. I have removed the text "Shame on Mozilla for silently ignoring this flag instead of, at minimum, warning users that they have decided to stop responding to it." from the commit message.

In any event, I don't think nixpkgs should do the same thing Mozilla did.

Besides, what's the point of having a compile-time option in that case? A compile-time flag to set the default value of a runtime option? Really? I can't think of any other examples of those anywhere in nixpkgs... I'd be shocked if there were any of them in packages as widely-used as a web browser.

And it's totally inconsistent with the nixpkgs naming scheme, where fooSupport means compiled with support for foo. Which is not surprising, because when drmSupport was introduced, that's what it meant.

[ghost]

The message feels a bit too wordy to me. Maybe just keep the last part about the policy file?

Ok, how does this look?

I really want to keep a link to the commit because it really is the best and only objective evidence that a silent and radical behavior change was made. Otherwise we get long discussions and hemming and hawing about "oh, I forgot about that, hey, it never worked anyway" etc etc.

Frankly if the Nixpkgs Dieties declared that this throw had to be a single line of text, I'd throw that hg.mozilla.org url; there's no shorter or better explanation of the problem. The commit really speaks for itself.

[ghost]

Of course. But you still got libclearkey.so in the closure, didn't you?

On mobile currently, so can't check

Please let us know when you get back to your desk and take a look.

And although it is strictly tangential to this particular PR, you'll notice that libclearkey.so is present in the aarch64 closures as well, which is pretty baffling since EME is supposedly not supported on aarch64 linux.

[mweinelt]

Besides, what's the point of having a compile-time option in that case? A compile-time flag to set the default value of a runtime option? Really? I can't think of any other examples of those anywhere in nixpkgs... I'd be shocked if there were any of them in packages as widely-used as a web browser.

To make it a preference that is still user-overridable without having to recompile. This is all about user choice!

I have other duties to attend to, so I'll reply to other comments later.

[ghost]

Besides, what's the point of having a compile-time option in that case?

To make it a preference that is still user-overridable without having to recompile.

I think we're in violent agreement here: DRM is no longer a compile-time choice for firefox.

Therefore, it makes no sense for nixpkgs to have a compile-time option for this functionality -- especially because the option now does nothing (like the goggles).

This is all about user choice!

::rolls eyes::

[ghost]

Torbrowser noticed the same thing, and found a way to restore the old drmSupport=false behavior, which they've been shipping for the last five years:

https://gitweb.torproject.org/tor-browser.git/commit/?h=geckoview-96.0.3-11.0-1&id=952eef1f393989d027fc1c4364bb6dc59d8b1f1a

Bug 16285: Exclude ClearKey system for now

commit e948ae5d404321a1ed0316ffb97baf45ee0163a5
Author: Georg Koppen <gk@torproject.org>
Date:   Mon May 22 12:44:40 2017 +0000

In the past the ClearKey system had not been compiled when specifying
--disable-eme. But that changed and it is even bundled nowadays (see:
Mozilla's bug 1300654). We don't want to ship it right now as the use
case for it is not really visible while the code had security
vulnerabilities in the past.

[ghost]

With the benefit of hindsight, the tone of this PR was... less than tactful. When I have more time available I will try again using a more diplomatic approach.