1360863 - EME prompt is not shown

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Steps to reproduce:

* Disable EME
* Attempt to play EME content on Netflix or YouTUbe

Expected results:

Drop down shown to enable DRM

Actual results:

No drop down. Unhelpful error message shown on Netflix/YouTube

Comment 1

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

A simpler test case is:

https://shaka-player-demo.appspot.com/demo/#asset=//yt-dash-mse-test.commondatastorage.googleapis.com/media/oops_cenc-20121114-signedlicenseurl-manifest.mpd;lang=en-US;play

This URL should play if "Play DRM Content" is checked, and show the "enable DRM" notification box if not.

I see this error logged in console:

*** Failed to format string emeNotifications.drmContentDisabled.message in bundle: chrome://browser/locale/browser.properties
JavaScript error: XStringBundle, line 37: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]

I bet something change in the front end.

Comment 2

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

This was regressed by https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a1222fd9f4 in Bug 1330001. So it shipped in 53. :(

Comment 3

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

This bug causes us to not show the "Enable DRM playback" notification box when users visit Netflix.com or any other site that tries to use DRM playback while DRM playback is disabled. We ship with DRM playback disabled by default on Linux (only). So this bug means that Linux users won't be prompted to enable DRM playback, and so sites trying to use DRM will not work and there will be no obvious way to make them work!

Comment 4

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

Attachment: Bug 1360863 - Unremove emeNotifications.drmContentDisabled.message.

This string was (accidentally I presume) removed in bug 1330001, but it's
needed for the "enable DRM" prompt that we show if we visit Netflix or another
site that uses EME and EME is disabled.

Without this, users on Linux (where we ship EME disabled by default) will never
be prompted to turn on EME, and so Netflix will likely just fail with no
obvious way to make it work!

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

Comment 5

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Once this lands, let's verify the fix and uplift to at least 54.

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 8863290 [details]
Bug 1360863 - Unremove emeNotifications.drmContentDisabled.message.

https://reviewboard.mozilla.org/r/135096/#review138512

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ec98c2a4824
Unremove emeNotifications.drmContentDisabled.message. r=dao

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ec98c2a4824

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Once this lands, let's verify the fix and uplift to at least 54.

The patch for uplift should have the string hard-coded. The error it's already in release, which means we lost that string all over the place.

Comment 11

[Sajedul Islam]

I have reproduced this bug with Nightly 55.0a1 (2017-04-29) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20170503030212
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170503]

Comment 12

[Gerry Chang [:gchang]]

Hi :cpearce,
Could you please nominate patch for uplift to Bet54?

Comment 13

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

Attachment: Beta rebase with string hardcoded

Comment 14

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

Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

(In reply to Francesco Lodolo [:flod] from comment #10)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> > Once this lands, let's verify the fix and uplift to at least 54.
> 
> The patch for uplift should have the string hard-coded. The error it's
> already in release, which means we lost that string all over the place.

Like this?

Comment 15

[Francesco Lodolo [:flod]]

I don't think I've ever seen a nested ternary operator, but yes, it should do the right: read the hard-coded string if the ID is emeNotifications.drmContentDisabled.message, otherwise fall back to the "proper" code path.

Comment 16

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

Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

I have tested this code and it works as expected, but would be good to get a once over from a front end reviewer before requesting uplift to beta.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8865382 [details] [diff] [review]
Beta rebase with string hardcoded

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

::: browser/base/content/browser-media.js
@@ +103,5 @@
>  
>      let msgPrefix = "emeNotifications." + notificationId + ".";
>      let msgId = msgPrefix + "message";
>  
> +    let message = (notificationId == "drmContentDisabled") ?

We should try to load the string from the .properties file first, and if that fails then fall back to the hard coded string. As this patch is written, locales won't be able to fix it if they want to.

Can we wrap the getFormattedString call in a try/catch and then if the msgId is "emeNotifications.drmContentDisabled.message" and an exception is thrown we will use the hard-coded string?

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> We should try to load the string from the .properties file first, and if
> that fails then fall back to the hard coded string. As this patch is
> written, locales won't be able to fix it if they want to.

This patch is only for Beta (maybe release), we don't have the string available there, so there's not much of a point in trying to load it.

Comment 19

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

Attachment: Beta rebase with string hardcoded

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

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

Thanks!

Comment 21

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

Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

Approval Request Comment

[Feature/Bug causing the regression]: DRM protected video playback (i.e. Netflix). This is a regression from Bug 1330001 which accidentally removed a string.

[User impact if declined]: Users who have "Play DRM Content" unchecked in Firefox's preferences (which is by default all Linux users) will see sites that use DRM playback like Netflix fail with no indication from our UI as to why they had failed.

[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No. I have verified with a local build. The fix to Nightly re-added the string. But because we can't take string changes to beta (right?), this patch hard codes the string in English. So it's a completely different patch to what landed on Nightly.

[Needs manual test from QE? If yes, steps to reproduce]: Yes please. Disable DRM playback in about:preferences, load https://shaka-player-demo.appspot.com/ and you should see a notification box drop down "You must enable DRM to play some audio or video on this page."

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No

[Why is the change risky/not risky?]: It's simple.

[String changes made/needed]: Yes, this patch hard codes a string was removed accidentally with an English string. This patch also checks to see whether the string is there first, so if our translators could re-add the fix we'd be able to gracefully start using the localized string.

Comment 22

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

We should really get a test for this... maybe a browser chrome mochitest? I thought we had one already for this feature...

Comment 23

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 24

[alex_mayorga]

¡Hola Gerry!

FWIW I do get the prompt.

Nombre 	Firefox
Versión 	55.0a1
Id. de compilación 	20170510183715

¡Gracias!
Alex

Comment 25

[alex_mayorga]

Attachment: Screen capture of the prompt to re-enable DRM.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8866018 [details] [diff] [review]
Beta rebase with string hardcoded

Fix an EME prompt issue and was verified. Beta54+. Should be in 54 beta 7.

Comment 27

[Brindusa Tot[:brindusat]]

Sorry, yesterday was a full day.

Verified as fixed on latest Nightly 55.0a1, Build ID 20170510030301 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

I have managed to reproduce this issue on Firefox 53 using STR from comment 0.

This is verified fixed on 54 beta 9 across platforms: Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.10.5.

Comment 30

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

I added a test for this feature in bug 1366167.