1299694 - Don't try to download Widevine by default on Tier3 platforms as well

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jan Beich]

Attachment: default stub plugins N/A from vendors (screenshot)

+++ This bug was initially created as a clone of Bug #1295853 +++

Bug 1295853 failed to remove Widevine from about:addons#plugins.

Comment 1

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

Attachment: Bug 1299694 - Ensure we don't enable Widevine unintentionally.

On FreeBSD the target.kernel etc checks in enable_eme are failing,
but we're still falling through to |return value|, and so Widevine
is being enabled. If we remove the |return value| from enable_eme
we at least make Widevine disabled where it's not supposed to be
enabled.

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

Comment 2

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

Attached patch at least makes `./mach configure` work on FreeBSD, and will basically partially revert some of the changes I made to address the review comments for bug 1295853 comment 21.

Comment 3

[Jan Beich]

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

mozbuild.frontend.reader.BuildReaderError:
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    browser/app/moz.build

The error was triggered on line 45 of this file:

    for cdm in CONFIG['MOZ_EME_MODULES']:

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ["TypeError: 'NoneType' object is not iterable\n"]

*** Fix above errors and then restart with\
               "/usr/local/bin/gmake -f client.mk build"
gmake: *** [client.mk:377: configure] Error 1

Comment 4

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

(In reply to Jan Beich from comment #3)
> Comment on attachment 8788019 [details]
> Bug 1299694 - Ensure we don't enable Widevine unintentionally.
> 
> The error was triggered on line 45 of this file:
> 
>     for cdm in CONFIG['MOZ_EME_MODULES']:


Right. That needs to be:

def eme_modules(value):
    if value:
        for cdm in value:
            log.info('EME key system %s enabled' % cdm.upper())
    return value

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

https://reviewboard.mozilla.org/r/76554/#review74920

::: toolkit/moz.configure
(Diff revision 1)
>          target.os not in ('Android', 'iOS') and
>          target.cpu in ('x86', 'x86_64')):
>          return value
>      elif value and value.origin != 'default':
>          die('%s is not supported on %s' % (value.format('--enable-eme'), target.alias))
> -    return value

cf. subsequent comments, this doesn't work without modifying the eme_modules function. But it would be better not to touch it.

I'd suggest this:
    # Return the same type of OptionValue (Positive or Negative), with an empty tuple.
    return value.__class__(()).

Comment 6

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

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

Comment 7

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

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76554/diff/1-2/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

https://reviewboard.mozilla.org/r/76554/#review75294

Comment 9

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

Jan Beich: Does the above patch work for you?

Comment 10

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

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

Comment 11

[Jan Beich]

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

Widevine in about:addons#plugins and MOZ_EME_MODULES in obj/config/autoconf.mk disappeared as intended. Thanks.

Comment 12

[Jan Beich]

(In reply to Chris Pearce (:cpearce) from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=10696291b91d

Maybe use Automation -> Trigger a Try Build via MozReview (Reviews tab) next time. For a second I thought you've posted a different version.

Comment 13

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

With this patch the build fails on Android, as now we correctly don't enable Widevine on Android...

libdom_media_mediasource_gtest.a.desc
42:32.27 In file included from /Users/CPearce/src/mozilla/central/dom/media/gtest/TestGMPCrossOrigin.cpp:15:0,
42:32.27                  from /Users/CPearce/src/mozilla/central/objdir-android/dom/media/gtest/Unified_cpp_dom_media_gtest0.cpp:20:
42:32.27 /Users/CPearce/src/mozilla/central/dom/media/gmp/GMPDecryptorProxy.h:9:44: fatal error: mozilla/DecryptorProxyCallback.h: No such file or directory
42:32.27  #include "mozilla/DecryptorProxyCallback.h"
42:32.27                                             ^
42:32.38 compilation terminated.

These errors are reported in Bug 1299627.

This is a consequence of Bug 1290830, which is in Firefox 51, and should be fixed when Bug 1300654 lands.

So I can't land this patch until Bug 1300654 lands. :(

Comment 14

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

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

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7c7197e8ea6311df402e2bf32506f530f59399
Bug 1299694 - Ensure we don't enable Widevine unintentionally. r=glandium

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f7c7197e8ea

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Is it something that we want to uplift? If so, please submit the uplift request. Thanks

Comment 18

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

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

Approval Request Comment
[Feature/regressing bug #]: EME on tier 3 Firefox builds.
[User impact if declined]: Firefox on Tier 3 systems will show UI by default saying that EME plugins can be downloaded, but the download will never succeed, and indeed EME will never work there.
[Describe test coverage new/current, TreeHerder]: We've got EME test coverage, so we'll be sure we don't regress our Tier 1 builds.
[Risks and why]: Low; build system change.
[String/UUID change made/needed]: None.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788019 [details]
Bug 1299694 - Ensure we don't enable Widevine unintentionally.

We tried fixing this in bug 1295853, it seems we need more, Aurora51+, Beta50+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab649b462ad8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc0709bd50f1